Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR: Remove Deadcode in NioTransport CORS #34324

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ public final class NioCorsConfig {
private final long maxAge;
private final Set<HttpMethod> allowedRequestMethods;
private final Set<String> allowedRequestHeaders;
private final boolean allowNullOrigin;
private final Map<CharSequence, Callable<?>> preflightHeaders;
private final boolean shortCircuit;

Expand All @@ -61,7 +60,6 @@ public final class NioCorsConfig {
maxAge = builder.maxAge;
allowedRequestMethods = builder.requestMethods;
allowedRequestHeaders = builder.requestHeaders;
allowNullOrigin = builder.allowNullOrigin;
preflightHeaders = builder.preflightHeaders;
shortCircuit = builder.shortCircuit;
}
Expand Down Expand Up @@ -108,19 +106,6 @@ public boolean isOriginAllowed(final String origin) {
return false;
}

/**
* Web browsers may set the 'Origin' request header to 'null' if a resource is loaded
* from the local file system.
*
* If isNullOriginAllowed is true then the server will response with the wildcard for the
* the CORS response header 'Access-Control-Allow-Origin'.
*
* @return {@code true} if a 'null' origin should be supported.
*/
public boolean isNullOriginAllowed() {
return allowNullOrigin;
}

/**
* Determines if credentials are supported for CORS requests.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,19 +49,6 @@ public static NioCorsConfigBuilder forAnyOrigin() {
return new NioCorsConfigBuilder();
}

/**
* Creates a {@link NioCorsConfigBuilder} instance with the specified origin.
*
* @return {@link NioCorsConfigBuilder} to support method chaining.
*/
public static NioCorsConfigBuilder forOrigin(final String origin) {
if ("*".equals(origin)) {
return new NioCorsConfigBuilder();
}
return new NioCorsConfigBuilder(origin);
}


/**
* Create a {@link NioCorsConfigBuilder} instance with the specified pattern origin.
*
Expand All @@ -87,14 +74,12 @@ public static NioCorsConfigBuilder forOrigins(final String... origins) {
Optional<Set<String>> origins;
Optional<Pattern> pattern;
final boolean anyOrigin;
boolean allowNullOrigin;
boolean enabled = true;
boolean allowCredentials;
long maxAge;
final Set<HttpMethod> requestMethods = new HashSet<>();
final Set<String> requestHeaders = new HashSet<>();
final Map<CharSequence, Callable<?>> preflightHeaders = new HashMap<>();
private boolean noPreflightHeaders;
boolean shortCircuit;

/**
Expand Down Expand Up @@ -130,18 +115,6 @@ public static NioCorsConfigBuilder forOrigins(final String... origins) {
anyOrigin = false;
}

/**
* Web browsers may set the 'Origin' request header to 'null' if a resource is loaded
* from the local file system. Calling this method will enable a successful CORS response
* with a wildcard for the CORS response header 'Access-Control-Allow-Origin'.
*
* @return {@link NioCorsConfigBuilder} to support method chaining.
*/
NioCorsConfigBuilder allowNullOrigin() {
allowNullOrigin = true;
return this;
}

/**
* Disables CORS support.
*
Expand Down Expand Up @@ -219,71 +192,6 @@ public NioCorsConfigBuilder allowedRequestHeaders(final String... headers) {
return this;
}

/**
* Returns HTTP response headers that should be added to a CORS preflight response.
*
* An intermediary like a load balancer might require that a CORS preflight request
* have certain headers set. This enables such headers to be added.
*
* @param name the name of the HTTP header.
* @param values the values for the HTTP header.
* @return {@link NioCorsConfigBuilder} to support method chaining.
*/
public NioCorsConfigBuilder preflightResponseHeader(final CharSequence name, final Object... values) {
if (values.length == 1) {
preflightHeaders.put(name, new ConstantValueGenerator(values[0]));
} else {
preflightResponseHeader(name, Arrays.asList(values));
}
return this;
}

/**
* Returns HTTP response headers that should be added to a CORS preflight response.
*
* An intermediary like a load balancer might require that a CORS preflight request
* have certain headers set. This enables such headers to be added.
*
* @param name the name of the HTTP header.
* @param value the values for the HTTP header.
* @param <T> the type of values that the Iterable contains.
* @return {@link NioCorsConfigBuilder} to support method chaining.
*/
public <T> NioCorsConfigBuilder preflightResponseHeader(final CharSequence name, final Iterable<T> value) {
preflightHeaders.put(name, new ConstantValueGenerator(value));
return this;
}

/**
* Returns HTTP response headers that should be added to a CORS preflight response.
*
* An intermediary like a load balancer might require that a CORS preflight request
* have certain headers set. This enables such headers to be added.
*
* Some values must be dynamically created when the HTTP response is created, for
* example the 'Date' response header. This can be accomplished by using a Callable
* which will have its 'call' method invoked when the HTTP response is created.
*
* @param name the name of the HTTP header.
* @param valueGenerator a Callable which will be invoked at HTTP response creation.
* @param <T> the type of the value that the Callable can return.
* @return {@link NioCorsConfigBuilder} to support method chaining.
*/
public <T> NioCorsConfigBuilder preflightResponseHeader(final CharSequence name, final Callable<T> valueGenerator) {
preflightHeaders.put(name, valueGenerator);
return this;
}

/**
* Specifies that no preflight response headers should be added to a preflight response.
*
* @return {@link NioCorsConfigBuilder} to support method chaining.
*/
public NioCorsConfigBuilder noPreflightResponseHeaders() {
noPreflightHeaders = true;
return this;
}

/**
* Specifies that a CORS request should be rejected if it's invalid before being
* further processing.
Expand All @@ -305,7 +213,7 @@ public NioCorsConfigBuilder shortCircuit() {
* @return {@link NioCorsConfig} the configured CorsConfig instance.
*/
public NioCorsConfig build() {
if (preflightHeaders.isEmpty() && !noPreflightHeaders) {
if (preflightHeaders.isEmpty()) {
preflightHeaders.put("date", DateValueGenerator.INSTANCE);
preflightHeaders.put("content-length", new ConstantValueGenerator("0"));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,6 @@ private void setPreflightHeaders(final HttpResponse response) {
private boolean setOrigin(final HttpResponse response) {
final String origin = request.headers().get(HttpHeaderNames.ORIGIN);
if (!Strings.isNullOrEmpty(origin)) {
if ("null".equals(origin) && config.isNullOriginAllowed()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems needed? Or is this handled in a different way?

setAnyOrigin(response);
return true;
}

if (config.isAnyOriginSupported()) {
if (config.isCredentialsAllowed()) {
echoRequestOrigin(response);
Expand Down Expand Up @@ -201,10 +196,6 @@ private boolean validateOrigin() {
return true;
}

if ("null".equals(origin) && config.isNullOriginAllowed()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems needed? Or is this handled in a different way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielmitterdorfer actually this is always false (there's no code assigning anything to that field in the builder) so this must be handled a different way or we have a bug on our hands ... (looking into that)
Looking at the Netty code it's unused there as well ... I guess I could drop this logic there also?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears unused indeed and I think we don't want/need it but maybe @tbrooks8 or @jasontedor can chime in here?

Copy link
Member Author

@original-brownbear original-brownbear Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find a single usage of allowNullOrigin() (which would set the return of isNullOriginAllowed to true) in git in any revision. I also don't see why we'd want to support "null" for the origin really? (plus it seems this was never set to true before anyway)

return true;
}

// if the origin is the same as the host of the request, then allow
if (isSameOrigin(origin, request.headers().get(HttpHeaderNames.HOST))) {
return true;
Expand Down