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

Testing: RestClient does not escape source HTTP parameter correctly #9769

Closed
spinscale opened this issue Feb 19, 2015 · 0 comments
Closed

Testing: RestClient does not escape source HTTP parameter correctly #9769

spinscale opened this issue Feb 19, 2015 · 0 comments
Assignees
Labels
>bug >test Issues or PRs that are addressing/adding tests

Comments

@spinscale
Copy link
Contributor

This test reproduces the issue. Be aware, that this only happens, when the whole source is put into the source HTTP parameter and not sent as body, You can control this behaviour in RestClient.callApiBuilder() for testing purposes

---
"Expressions scripting test":

  - do:
      index:
        index:  test123
        type:   test
        id:     1
        body:   { age: 23 }

  - do:
      indices.refresh: {}

  - do: { search: { body: { script_fields : { my_field : { lang: expression, script: 'doc["age"].value + 19' } } } } }
  - match:  { hits.hits.0.fields.my_field: [ 42.0 ] }

The problem is, that everything gets encoded correctly, with the exception of the + sign, which gets replaced with a space upon decoding - and this makes the script compilation fail.

@spinscale spinscale added >bug >test Issues or PRs that are addressing/adding tests labels Feb 19, 2015
spinscale added a commit that referenced this issue Feb 19, 2015
The plus sign is not treated correctly in encoding and can lead
to problems, if the search request is encoded as HTTP parameter
instead of the HTTP body.

Relates #9769
spinscale added a commit that referenced this issue Feb 19, 2015
The plus sign is not treated correctly in encoding and can lead
to problems, if the search request is encoded as HTTP parameter
instead of the HTTP body.

Relates #9769
spinscale added a commit that referenced this issue Feb 19, 2015
The plus sign is not treated correctly in encoding and can lead
to problems, if the search request is encoded as HTTP parameter
instead of the HTTP body.

Relates #9769
spinscale added a commit that referenced this issue Feb 19, 2015
Note: The plus sign is not treated correctly in encoding and can lead
to problems, if the search request is encoded as HTTP parameter
instead of the HTTP body.

Relates #9769
@clintongormley clintongormley added help wanted adoptme and removed help wanted adoptme labels Feb 27, 2015
@javanna javanna self-assigned this Mar 2, 2015
javanna added a commit to javanna/elasticsearch that referenced this issue Mar 3, 2015
We've been relying on URI for url encoding, but it turns out it has some problems. For instance '+' stays as is while it should be encoded to `%2B`. If we go and manually encode query params we have to be careful though not to run into double encoding ('+'=>'%2B'=>'%252B'). The applied solution relies on URI encoding for the url path, but manual url encoding for the query parameters. We prevent URI from double encoding query params by using its single argument constructor that leaves everything as is.

We can also revert back the expression script REST test that revealed this to its original content (which contains an addition).

Closes elastic#9769
Closes elastic#9946
javanna added a commit that referenced this issue Mar 3, 2015
We've been relying on URI for url encoding, but it turns out it has some problems. For instance '+' stays as is while it should be encoded to `%2B`. If we go and manually encode query params we have to be careful though not to run into double encoding ('+'=>'%2B'=>'%252B'). The applied solution relies on URI encoding for the url path, but manual url encoding for the query parameters. We prevent URI from double encoding query params by using its single argument constructor that leaves everything as is.

We can also revert back the expression script REST test that revealed this to its original content (which contains an addition).

Closes #9769
Closes #9946
javanna added a commit that referenced this issue Mar 3, 2015
We've been relying on URI for url encoding, but it turns out it has some problems. For instance '+' stays as is while it should be encoded to `%2B`. If we go and manually encode query params we have to be careful though not to run into double encoding ('+'=>'%2B'=>'%252B'). The applied solution relies on URI encoding for the url path, but manual url encoding for the query parameters. We prevent URI from double encoding query params by using its single argument constructor that leaves everything as is.

We can also revert back the expression script REST test that revealed this to its original content (which contains an addition).

Closes #9769
Closes #9946
@javanna javanna closed this as completed in 4ad33c3 Mar 3, 2015
mute pushed a commit to mute/elasticsearch that referenced this issue Jul 29, 2015
The plus sign is not treated correctly in encoding and can lead
to problems, if the search request is encoded as HTTP parameter
instead of the HTTP body.

Relates elastic#9769
mute pushed a commit to mute/elasticsearch that referenced this issue Jul 29, 2015
We've been relying on URI for url encoding, but it turns out it has some problems. For instance '+' stays as is while it should be encoded to `%2B`. If we go and manually encode query params we have to be careful though not to run into double encoding ('+'=>'%2B'=>'%252B'). The applied solution relies on URI encoding for the url path, but manual url encoding for the query parameters. We prevent URI from double encoding query params by using its single argument constructor that leaves everything as is.

We can also revert back the expression script REST test that revealed this to its original content (which contains an addition).

Closes elastic#9769
Closes elastic#9946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants