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

Remove lenient URL parameter parsing #20722

Merged
merged 17 commits into from Oct 4, 2016

Conversation

Projects
None yet
5 participants
@jasontedor
Copy link
Member

commented Oct 3, 2016

Today when parsing a request, Elasticsearch silently ignores incorrect
(including parameters with typos) or unused parameters. This is bad as
it leads to requests having unintended behavior (e.g., if a user hits
the _analyze API and misspell the "tokenizer" then Elasticsearch will
just use the standard analyzer, completely against intentions).

This commit removes lenient URL parameter parsing. The strategy is
simple: when a request is handled and a parameter is touched, we mark it
as such. Before the request is actually executed, we check to ensure
that all parameters have been consumed. If there are remaining
parameters yet to be consumed, we fail the request with a list of the
unconsumed parameters. An exception has to be made for parameters that
format the response (as opposed to controlling the request); for this
case, handlers are able to provide a list of parameters that should be
excluded from tripping the unconsumed parameters check because those
parameters will be used in formatting the response.

Additionally, some inconsistencies between the parameters in the code
and in the docs are corrected.

Closes #14719

Remove lenient URL parameter parsing
Today when parsing a request, Elasticsearch silently ignores incorrect
(including parameters with typos) or unused parameters. This is bad as
it leads to requests having unintended behavior (e.g., if a user hits
the _analyze API and misspell the "tokenizer" then Elasticsearch will
just use the standard analyzer, completely against intentions).

This commit removes lenient URL parameter parsing. The strategy is
simple: when a request is handled and a parameter is touched, we mark it
as such. Before the request is actually executed, we check to ensure
that all parameters have been consumed. If there are remaining
parameters yet to be consumed, we fail the request with a list of the
unconsumed parameters. An exception has to be made for parameters that
format the response (as opposed to controlling the request); for this
case, handlers are able to provide a list of parameters that should be
excluded from tripping the unconsumed parameters check because those
parameters will be used in formatting the response.

Additionally, some inconsistencies between the parameters in the code
and in the docs are corrected.

// validate unconsumed params, but we must exclude params used to format the response
// we copy because we do not want to modify the request params
final Set<String> unconsumedParams = new HashSet<>(request.unconsumedParams());

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

Maybe this should be a list instead?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 3, 2016

Author Member

I pushed 11396c8.

@@ -122,6 +128,14 @@ public final String param(String key, String defaultValue) {
return params;
}

Set<String> unconsumedParams() {

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

If you document that this returns a copy then there should be no need to copy a second time.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 3, 2016

Author Member

I pushed 11396c8.

*/
protected abstract Runnable doRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception;

protected Set<String> responseParams() {

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

Probably worth javadoc.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 3, 2016

Author Member

Sure, I can do that. There was a comment on the Javadoc for BaseRestHandler#doRequest in this regard, but I agree that it would be nice to have it here too.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 3, 2016

Author Member

I pushed 23da72f.

@@ -220,10 +220,11 @@ public void sendErrorResponse(RestRequest request, RestChannel channel, Exceptio
*/
boolean checkRequestParameters(final RestRequest request, final RestChannel channel) {
// error_trace cannot be used when we disable detailed errors
if (channel.detailedErrorsEnabled() == false && request.paramAsBoolean("error_trace", false)) {
// we consume the error_trace parameter first to ensure that it is always consumed
if (request.paramAsBoolean("error_trace", false) && channel.detailedErrorsEnabled() == false) {

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

Nice! Short circuiting if now causes problems with unconsumed request parameters!

@Override
public RestResponse buildResponse(FieldStatsResponse response, XContentBuilder builder) throws Exception {
builder.startObject();
buildBroadcastShardsHeader(builder, request, response);

builder.startObject("indices");
for (Map.Entry<String, Map<String, FieldStats>> entry1 :
response.getIndicesMergedFieldStats().entrySet()) {
response.getIndicesMergedFieldStats().entrySet()) {

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

++

import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

Are these leftovers from a previous iteration?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 3, 2016

Author Member

I pushed ce6a76c.

validateQueryRequest.query(
RestActions.getQueryContent(RestActions.getRestContent(request), indicesQueriesRegistry, parseFieldMatcher));
} catch (ParsingException e) {
return () -> handleException(channel, validateQueryRequest, e.getDetailedMessage());

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

I think it should be fine to just throw the exception here. So long as we haven't gone async this should be safe. We do it in other places. I guess the only difference is whether we report errors in the query first or extra parameters first which isn't really a big deal either way to me.

This does get me to thinking - maybe we should return a Consumer<RestChannel> instead of a Runanble? Maybe doRequest shouldn't have the variable it needs to send a response so it is never tempted?

Also the name doRequest. Maybe prepare or something? I'm much better at saying "I don't like that name" than I am at making a better name.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 3, 2016

Author Member

To rename doRequest to prepareRequest I pushed 5a2e83f. I'm still thinking about the consumer suggestion.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 3, 2016

Author Member

Regarding just letting the exception bubble up, I don't think so because we format the response nicely; see RestValidateQueryAction#buildErrorResponse.

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

Makes sense to me.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 4, 2016

Author Member

@nik9000 I pushed 777fb26 to apply your suggestion of returning a consumer instead of a runnable; please review carefully and let me know what you think? I did this in a way that can be easily reverted if we decide that we do not like it.

private static final Set<String> RESPONSE_PARAMS;

static {
final Set<String> responseParams = new HashSet<>(Arrays.asList("local", "health"));

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

This is like the one place where I miss Guava. It was really nice and fluent to make these.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 3, 2016

Author Member

Java 9 will fix this: http://openjdk.java.net/jeps/269

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

Awesome.


protected abstract void documentation(StringBuilder sb);

protected abstract Table getTableWithHeader(final RestRequest request);

@Override
public void handleRequest(final RestRequest request, final RestChannel channel, final NodeClient client) throws Exception {
public Runnable doRequest(final RestRequest request, final RestChannel channel, final NodeClient client) throws Exception {
boolean helpWanted = request.paramAsBoolean("help", false);
if (helpWanted) {
Table table = getTableWithHeader(request);

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

It feels like this if block out to be inside the Runnable.

channel.sendResponse(RestTable.buildResponse(table, channel));
} catch (Exception e) {
try {
channel.sendResponse(new BytesRestResponse(channel, e));

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 3, 2016

Contributor

Maybe it is ok to just through from here? That might mean declaring a new functional interface?

Add unit tests for strict URL parameter parsing
This commit adds unit tests for strict URL parameter parsing, including
test cases for response parameters.

@jasontedor jasontedor added the v5.1.1 label Oct 3, 2016

jasontedor and others added some commits Oct 3, 2016

Add Javadocs for BaseRequestHandler#responseParams
This commit adds Javadocs for BaseRequestHandler#responseParams to
clarify the intent of classes overriding this method.
Refactor RestRequest#unconsumedParams to be a list
This commit refactors RestRequest#unconsumedParams to return a list, for
simplicity, and clarifies that callers are free to modify the returned
list.
Remove unused imports from RestGetSettingsAction
This commit removes some unused imports from
o.e.r.a.a.i.RestGetSettingsAction.
Fix formatting in RestGetIndicesAction
This commit addresses a formatting issue in
o.e.r.a.a.i.RestGetIndicesAction that arose from a method's parameters
being indented at the same level as the method's body.
Rename BRH#doRequest to BRH#prepareRequest
This commit renames the method BaseRequestHandler#doRequest to
BRH#prepareRequest as this method is merely preparing the request for
execution.
Refactor BaseRestHandler#prepareRequest
This commit refactors BaseRequestHandler#prepareRequest to return a
RestChannelConsumer instead of a runnable. This removes the need for
REST request handlers to take a RestChannel parameter. Additionally, the
checked exception is tightened to just be an IOException instead of the
top-level Exception.
@s1monw

s1monw approved these changes Oct 4, 2016

Copy link
Contributor

left a comment

it looks great, one thing that I am concerned about is that if we push this into 5.x we will cause things to fail on the client side when users upgrade. The other option is to push it only to 6.0 but then it will take way to long to make it to the user. I am very torn since it's a big change to push it 5.0 either. :/

final Set<String> responseParams = responseParams();
final Iterator<String> it = unconsumedParams.iterator();

// this has to use an iterator lest a ConcurrentModificationException will arise

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 4, 2016

Contributor

not sure I get this comment maybe we shouldn't modify the params at all in that way but create a copy set of the unconsumed ones?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 4, 2016

Author Member

@s1monw It's merely the simple problem of not calling List#remove while you're iterating over it. I prefer your suggestion, I pushed fc03e96.

jasontedor added some commits Oct 4, 2016

Merge branch 'master' into strict-rest-params
* master:
  Optimized LatLon sorting does not work in the descending order.
  IndicesAliasesRequest should not implement CompositeIndicesRequest (#20726)
  Upgrade microbenchmarks to JMH 1.15
  Build: Use more general archivesBaseName setting, to ensure pom uses matching file name
  Update favicon (#20727)
  Skip prereleases for restore bwc tests too
  Skip prereleases in static bwc tests
Handle exception exclusive from validating query
This commit fixes an issue in the validate query handler where an
exception during parsing the query would still trigger attempting to
validate the query. These two cases should be mutually exclusive.
@nik9000

nik9000 approved these changes Oct 4, 2016

action.accept(channel);
}

@FunctionalInterface

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 4, 2016

Contributor

Nit: I think this belongs under the javadoc.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 4, 2016

Author Member

I thought this would violate the Javadoc linter? I will verify.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 4, 2016

Author Member

It doesn't make the linter mad, so I pushed a1f065e.

}
return () -> {};
return channel -> {};

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 4, 2016

Contributor

Maybe better to just throw an UnsupportedOperationException here.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 4, 2016

Author Member

Rather an IllegalArgumentException; UnsupportedOperationException means "there are no parameters you can pass here to make this method execute" but IllegalArgumentException says "there are parameters you can pass here to make this method execute, just not the parameters that you gave me". I get your point though, and will push a commit. (Also, this is in should-never-happen territory, since the handler will not be invoked without the method being GET or POST.)

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 4, 2016

Contributor

Yeah IAE is better, sorry!

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 4, 2016

Author Member

I pushed 0d39625.

throw new UncheckedIOException(e);
}
private void handleException(
final ValidateQueryRequest validateQueryRequest,

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 4, 2016

Contributor

Indent these please!

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 4, 2016

Author Member

I pushed 309fca4. Well, I didn't indent like you wanted, just made the problem go away. 😇

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2016

I'm as torn as @s1monw on versioning. I suppose getting this into 5.1 violates semver because garbage on the URL would be identified. I'd love to have it in 5.0 but I think it violates the spirit of code freeze to try and do it. It is super tempting though.

jasontedor added some commits Oct 4, 2016

Simplify unconsumed parameter computation
This commit simplifies the computation of the unconsumed parameters
remaining when handling a REST request.
Move FunctionalInterface annotation under Javadoc
This commit moves a FunctionalInterface annotation under the Javadoc on
BaseRestHandler$RestChannelConsumer because it looks better that way.
Throw illegal argument exception on invalid method
The /_upgrade REST handler accepts GET and POST requests, but would
silently ignore invocations for other HTTP verbs. This commit modifies
this handler so that an illegal argument exception is thrown instead.
Fix indentation on RVQA#handleException
This commit fixes some obnoxious indentation on the method parameters
for RestValidateQueryAction#handleException.
Merge branch 'master' into strict-rest-params
* master:
  Geo-distance sorting should use `POSITIVE_INFINITY` for missing geo points instead of `MAX_VALUE`.
  Process more expensive allocation deciders last (#20724)
  Skip shard management code when updating cluster state on client/tribe nodes (#20731)

@jasontedor jasontedor added the v5.0.0 label Oct 4, 2016

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2016

After conversation with @nik9000 and @s1monw, we have decided the leniency here is big enough a bug that we want to ship this code as early as possible. The options on the table were:

  • 5.0.0
  • 5.1.0
  • 6.0.0

We felt that 5.1.0 is not a viable option, it is too breaking of a change during a minor release, and waiting until 6.0.0 is waiting too long to fix this bug so we will ship this in 5.0.0.

@jasontedor jasontedor merged commit 51d5379 into elastic:master Oct 4, 2016

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@jasontedor jasontedor deleted the jasontedor:strict-rest-params branch Oct 4, 2016

jasontedor added a commit that referenced this pull request Oct 4, 2016

Remove lenient URL parameter parsing
Today when parsing a request, Elasticsearch silently ignores incorrect
(including parameters with typos) or unused parameters. This is bad as
it leads to requests having unintended behavior (e.g., if a user hits
the _analyze API and misspell the "tokenizer" then Elasticsearch will
just use the standard analyzer, completely against intentions).

This commit removes lenient URL parameter parsing. The strategy is
simple: when a request is handled and a parameter is touched, we mark it
as such. Before the request is actually executed, we check to ensure
that all parameters have been consumed. If there are remaining
parameters yet to be consumed, we fail the request with a list of the
unconsumed parameters. An exception has to be made for parameters that
format the response (as opposed to controlling the request); for this
case, handlers are able to provide a list of parameters that should be
excluded from tripping the unconsumed parameters check because those
parameters will be used in formatting the response.

Additionally, some inconsistencies between the parameters in the code
and in the docs are corrected.

Relates #20722

jasontedor added a commit that referenced this pull request Oct 4, 2016

Remove lenient URL parameter parsing
Today when parsing a request, Elasticsearch silently ignores incorrect
(including parameters with typos) or unused parameters. This is bad as
it leads to requests having unintended behavior (e.g., if a user hits
the _analyze API and misspell the "tokenizer" then Elasticsearch will
just use the standard analyzer, completely against intentions).

This commit removes lenient URL parameter parsing. The strategy is
simple: when a request is handled and a parameter is touched, we mark it
as such. Before the request is actually executed, we check to ensure
that all parameters have been consumed. If there are remaining
parameters yet to be consumed, we fail the request with a list of the
unconsumed parameters. An exception has to be made for parameters that
format the response (as opposed to controlling the request); for this
case, handlers are able to provide a list of parameters that should be
excluded from tripping the unconsumed parameters check because those
parameters will be used in formatting the response.

Additionally, some inconsistencies between the parameters in the code
and in the docs are corrected.

Relates #20722

@clintongormley clintongormley added v5.0.0-rc1 and removed v5.0.0 labels Oct 7, 2016

@clintongormley clintongormley removed the v5.1.1 label Dec 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.