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

Send REST response for incorrectly encoded url params #28909

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@olcbean
Copy link
Contributor

commented Mar 6, 2018

Previously if the path contained incorrectly encoded sequence, an error was returned :

curl -XGET http://localhost:9200/index%?pretty
{
  "error" : {
    "root_cause" : [
      {
        "type" : "illegal_argument_exception",
        "reason" : "unterminated escape sequence at end of string: index%"
      }
    ],
    "type" : "illegal_argument_exception",
    "reason" : "unterminated escape sequence at end of string: index%"
  },
  "status" : 400
}

But if the incorrect sequence was part of the parameters, no response was returned.

curl -XGET http://localhost:9200/index?pretty%
curl: (52) Empty reply from server

This PR analyze the parameters and returns an error when there is a wrong encoding sequence in the url params ( The errors are consistent with the ones returned for incorrect path )

curl -XGET http://localhost:9200/index?pretty%
{
  "error": {
    "root_cause": [
      {
        "type": "illegal_argument_exception",
        "reason": "unterminated escape sequence at end of string: pretty%"
      }
    ],
    "type": "illegal_argument_exception",
    "reason": "unterminated escape sequence at end of string: pretty%"
  },
  "status": 400
}

Fixes : #21974

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2018

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2018

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@olcbean olcbean changed the title Send REST responce for incorrectly encoded url params Send REST response for incorrectly encoded url params Mar 6, 2018

@olcbean olcbean force-pushed the olcbean:rest_params_escape_seq branch to ac967f1 Mar 6, 2018

@javanna

This comment has been minimized.

Copy link
Member

commented Mar 6, 2018

@jasontedor
Copy link
Member

left a comment

I appreciate the motivation for the change, but the technical implementation is not attractive to me. Mutating the request as is done here is not something we want to be in the business of. I am marking this as "Request changes" because I can not approve of this PR as-is.

@@ -63,6 +63,8 @@
private final Set<String> consumedParams = new HashSet<>();
private final SetOnce<XContentType> xContentType = new SetOnce<>();

private Exception exception;

This comment has been minimized.

Copy link
@jasontedor

jasontedor Mar 6, 2018

Member

Adding this mutable field (and thus making RestRequest not immutable) is not attractive.

This comment has been minimized.

Copy link
@olcbean

olcbean Mar 6, 2018

Author Contributor

@jasontedor thanks!
Have another look ?

@jasontedor
Copy link
Member

left a comment

I left another comment; I continue to think that unless we can find a clean implementation here we let this one go.

}
this.exception.set(parsingQueryStringException);
this.params = params;

This comment has been minimized.

Copy link
@jasontedor

jasontedor Mar 8, 2018

Member

This change still bothers me a lot. Again, I appreciate the motivation for it but the implementation that we are being forced into here leaves a lot to be desired (I am not faulting you for it, we are somewhat painted into a corner here). Another problem here is that now the object is a garbage state, we have no idea what the state of params is here yet we are allowing construction of this object to complete as if nothing happened. This is dangerous and I think that we should not do it.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

I'd be tempted to make a checked exception that contains a RestRequest and throw it from the RestRequest ctor like new BadParametersException(cause, new RestRequest(xContentRegistry, rawPath, headers) {/*empty stuff*/});. Catch it is Netty4RequestHandler, close the netty request as needed, and fire the dummy request off into serverTransport.

And when I type it out I realize that is fairly hairy, but it doesn't need any SetOnce and a funny, half built HttpRequest object.

@jasontedor, I know my idea isn't great, but does it give you any ideas? I think we ought return something real to the user if we can get away with it.

@jasontedor

This comment has been minimized.

Copy link
Member

commented Mar 25, 2018

Thanks @nik9000; your comment has made me take a closer look, and I think there are more problems here that are going to require a holistic approach: if we blow up creating the channel (e.g., parsing ?pretty=this-value-is-neither-true-or-false) we do not respond to the client, we do not have a channel to the client to respond on, and we leak the pipelined request! This is similar to the problem of blowing up while creating the REST request which this PR is addressing.

@jasontedor

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

Thanks for poking at this one @olcbean, it uncovered some really bad stuff. I opened #29249 to address all the bad things that I could find.

@olcbean

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

Closing in favor of #29249.

@jasontedor thanks for following up on this with a much more robust solution!

@olcbean olcbean closed this Mar 27, 2018

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.