Remove lenient URL parameter parsing by jasontedor · Pull Request #20722 · elastic/elasticsearch

jasontedor · 2016-10-03T16:49:13Z

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

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.

nik9000 · 2016-10-03T16:55:20Z

core/src/main/java/org/elasticsearch/rest/BaseRestHandler.java

+
+        // 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());


Maybe this should be a list instead?

I pushed 11396c8.

nik9000 · 2016-10-03T16:56:26Z

core/src/main/java/org/elasticsearch/rest/RestRequest.java

        return params;
    }

+    Set<String> unconsumedParams() {


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

I pushed 11396c8.

nik9000 · 2016-10-03T16:56:44Z

core/src/main/java/org/elasticsearch/rest/BaseRestHandler.java

+     */
+    protected abstract Runnable doRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception;
+
+    protected Set<String> responseParams() {


Probably worth javadoc.

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.

I pushed 23da72f.

nik9000 · 2016-10-03T16:57:29Z

core/src/main/java/org/elasticsearch/rest/RestController.java

        // 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) {


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

nik9000 · 2016-10-03T16:58:04Z

core/src/main/java/org/elasticsearch/rest/action/RestFieldStatsAction.java

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


nik9000 · 2016-10-03T17:05:14Z

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetSettingsAction.java

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


Are these leftovers from a previous iteration?

I pushed ce6a76c.

nik9000 · 2016-10-03T17:14:01Z

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryAction.java

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


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.

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

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

Makes sense to me.

@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.

nik9000 · 2016-10-03T17:27:42Z

core/src/main/java/org/elasticsearch/rest/action/cat/RestIndicesAction.java

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


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

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

nik9000 · 2016-10-03T17:28:43Z

core/src/main/java/org/elasticsearch/rest/action/cat/AbstractCatAction.java

+    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);


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

nik9000 · 2016-10-03T17:32:10Z

plugins/jvm-example/src/main/java/org/elasticsearch/plugin/example/ExampleCatAction.java

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


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

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

This commit adds Javadocs for BaseRequestHandler#responseParams to clarify the intent of classes overriding this method.

This commit refactors RestRequest#unconsumedParams to return a list, for simplicity, and clarifies that callers are free to modify the returned list.

This commit removes some unused imports from o.e.r.a.a.i.RestGetSettingsAction.

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.

This commit renames the method BaseRequestHandler#doRequest to BRH#prepareRequest as this method is merely preparing the request for execution.

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

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. :/

s1monw · 2016-10-04T07:44:54Z

core/src/main/java/org/elasticsearch/rest/BaseRestHandler.java

+        final Set<String> responseParams = responseParams();
+        final Iterator<String> it = unconsumedParams.iterator();
+
+        // this has to use an iterator lest a ConcurrentModificationException will arise


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?

@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.

* 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

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 · 2016-10-04T12:26:05Z

core/src/main/java/org/elasticsearch/rest/BaseRestHandler.java

+        action.accept(channel);
+    }
+
+    @FunctionalInterface


Nit: I think this belongs under the javadoc.

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

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

nik9000 · 2016-10-04T12:29:13Z

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestUpgradeAction.java

+            return handlePost(request, client);
        }
-        return () -> {};
+        return channel -> {};


Maybe better to just throw an UnsupportedOperationException here.

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.)

Yeah IAE is better, sorry!

I pushed 0d39625.

nik9000 · 2016-10-04T12:30:07Z

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryAction.java

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


Indent these please!

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

nik9000 · 2016-10-04T12:33:20Z

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.

This commit simplifies the computation of the unconsumed parameters remaining when handling a REST request.

This commit moves a FunctionalInterface annotation under the Javadoc on BaseRestHandler$RestChannelConsumer because it looks better that way.

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.

This commit fixes some obnoxious indentation on the method parameters for RestValidateQueryAction#handleException.

* 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 · 2016-10-04T16:14:28Z

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.

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 >bug review :Core/Infra/REST API REST infrastructure and utilities v6.0.0-alpha1 labels Oct 3, 2016

nik9000 requested changes Oct 3, 2016

View reviewed changes

Add unit tests for strict URL parameter parsing

5c487cf

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

jasontedor added the v5.1.1 label Oct 3, 2016

jasontedor and others added 8 commits October 3, 2016 13:58

Add Javadocs for BaseRequestHandler#responseParams

23da72f

This commit adds Javadocs for BaseRequestHandler#responseParams to clarify the intent of classes overriding this method.

Refactor RestRequest#unconsumedParams to be a list

11396c8

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

ce6a76c

This commit removes some unused imports from o.e.r.a.a.i.RestGetSettingsAction.

Fix formatting in RestGetIndicesAction

fd794dd

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

5a2e83f

This commit renames the method BaseRequestHandler#doRequest to BRH#prepareRequest as this method is merely preparing the request for execution.

Skip prereleases in static bwc tests

916c854

Skip prereleases for restore bwc tests too

dccdb49

s1monw approved these changes Oct 4, 2016

View reviewed changes

jasontedor added 2 commits October 4, 2016 06:08

Handle exception exclusive from validating query

1e8f7fd

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 approved these changes Oct 4, 2016

View reviewed changes

jasontedor added 5 commits October 4, 2016 08:56

Simplify unconsumed parameter computation

fc03e96

This commit simplifies the computation of the unconsumed parameters remaining when handling a REST request.

Move FunctionalInterface annotation under Javadoc

a1f065e

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

0d39625

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

309fca4

This commit fixes some obnoxious indentation on the method parameters for RestValidateQueryAction#handleException.

jasontedor added the v5.0.0 label Oct 4, 2016

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

jasontedor deleted the strict-rest-params branch October 4, 2016 16:45

This was referenced Oct 4, 2016

Clarify wording for the strict REST params message #20745

Merged

Add did you mean to strict REST params #20747

Merged

Add response params to REST params did you mean #20753

Merged

jbaiera mentioned this pull request Oct 5, 2016

Strict URL Parsing Rejects Compound preference Parameter #20769

Closed

jasontedor mentioned this pull request Oct 6, 2016

Change separator for shards preference #20786

Merged

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

jasontedor mentioned this pull request Oct 7, 2016

Add docs for strict REST params parsing #20801

Merged

$@polyfractal$ polyfractal mentioned this pull request Oct 11, 2016

Huge change is ES behavior with JS client from ES 5.0.0 beta1 to rc1 #20855

Closed

nik9000 mentioned this pull request Oct 11, 2016

Adding built-in sorting capability to _cat apis. #20658

Merged

This was referenced Oct 19, 2016

Remove output_uuid parameter from cluster stats #21020

Merged

Remove node_info_format parameter from node stats #21021

Merged

jakommo mentioned this pull request Oct 20, 2016

[5.0] _nodes?all results in exception elastic/support-diagnostics#133

Closed

This was referenced Nov 4, 2016

Expand /_cat/snapshots API to accept _all and treat /_cat/snapshots as a synonym for /_cat/snapshots/_all #21346

Closed

Elasticsearch 5.0: _nodes/stats?all throws a 400 #21410

Closed

Remove lenient stats parsing #21417

Merged

This was referenced Nov 15, 2016

Remove lenient stats parsing 5.x #21576

Merged

Whitelist node stats indices level parameter #21024

Merged

jasontedor mentioned this pull request Nov 25, 2016

/_bulk write with consistency param throws an error #21804

Closed

clintongormley removed the v5.1.1 label Dec 8, 2016

Conversation

jasontedor commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Oct 4, 2016

Uh oh!

jasontedor commented Oct 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

jasontedor Oct 4, 2016 •

edited

Loading