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.

Sign up for GitHub

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

Jump to bottom

Rest-High-Level-Client:fix uri encode bug when url path start with '/' #34436

Merged
merged 2 commits into from
Apr 4, 2019
Merged

Rest-High-Level-Client:fix uri encode bug when url path start with '/' #34436

merged 2 commits into from
Apr 4, 2019

Conversation

roycarser
Copy link
Contributor

Closes #34433"
set userinfo and host parts of URI to something not null so that the path part will not be parsed as them

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@dakrone dakrone requested a review from hub-cap March 14, 2019 14:49
@hub-cap
Copy link
Contributor

hub-cap commented Mar 27, 2019

Hi @roycarser, sorry for taking a while to get back to you. I researched the problem, and whats happening is part of the forward slashes are being interpreted as a blank authority, as per https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/net/URI.java#L3161-L3166 . Changing the URI construction to use a // authority like below will cause all of your additional tests to pass. Since its been a while I can push this change for you, or Id be happy to have you push it as well. Once you push it, please do a git merge of master so we can ensure all of the tests can be run on our CI infrastructure.

URI uri = new URI(null, "//", "/" + pathPart, null, null);

hub-cap
hub-cap suggested changes Mar 27, 2019
View reviewed changes
Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

doh, I didnt leave the prior comment about the URI construction as a 'request changes', sorry about extra notification!

@roycarser
Copy link
Contributor Author

that's fine, I will finish it quickly

@roycarser roycarser force-pushed the master branch 2 times, most recently from f89efb9 to c025ebc Compare March 28, 2019 11:03
@hub-cap
Copy link
Contributor

hub-cap commented Mar 28, 2019 •
edited
Loading

Id like you to leave a comment in there expressing why we need the authority to be non empty. Something like

// the authority must be an empty string and not null, else paths that being with slashes could have them
// misinterpreted as part of the authority.
URI uri = new URI(null, "", "/" + pathPart, null, null);

Please note that I tested this with a blank / empty authority and it worked as well, so lets remove the "//" in favor for a blank string so its slightly less confusing.

@roycarser
Copy link
Contributor Author

The code has been commited.

@elasticcla
Copy link

Hi @roycarser, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

hub-cap
hub-cap approved these changes Apr 3, 2019
View reviewed changes
Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

awesome. Ill run it thru the paces and then backport it accordingly

@hub-cap
Copy link
Contributor

hub-cap commented Apr 3, 2019

@elasticmachine ok to test

@hub-cap hub-cap merged commit 629d2ea into elastic:master Apr 4, 2019
@hub-cap
Copy link
Contributor

hub-cap commented Apr 4, 2019

Thank you for the commit! Im going to backport it accordingly to 7.1.0

hub-cap pushed a commit that referenced this pull request Apr 4, 2019
@roycarser @hub-cap
HLRC: fix uri encode bug when url path starts with '/' (#34436)
075078e 
This commit sets the authority of a URI to blank such that it does not
misinterpret slashes in the path as the authority.

Closes #34433
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 4, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into jdk-13-ci
a3bc293 
* elastic/master: (25 commits)
  Rollup ignores time_zone on date histogram (elastic#40844)
  HLRC: fix uri encode bug when url path starts with '/' (elastic#34436)
  Mutes GatewayIndexStateIT.testRecoverBrokenIndexMetadata
  Docs: Pin two IDs in the rest client (elastic#40785)
  Adds version 6.7.2
  [DOCS] Remind users to include @ symbol when applying license file (elastic#40688)
  HLRC: Convert xpack methods to client side objects (elastic#40705)
  Allow ILM to stop if indices have nonexistent policies (elastic#40820)
  Add an OpenID Connect authentication realm (elastic#40674)
  [DOCS] Rewrite query def (elastic#40746)
  [ML] Changes default destination index field mapping and adds scripted_metric agg (elastic#40750)
  Docs: Drop inline callouts from painless book (elastic#40805)
  [ML] refactoring start task a bit, removing unused code (elastic#40798)
  [DOCS] Document index.load_fixed_bitset_filters_eagerly (elastic#40780)
  Make remote cluster resolution stricter (elastic#40419)
  [ML] Add created_by info to usage stats (elastic#40518)
  SQL: [Docs] Small fixes for CURRENT_TIMESTAMP docs (elastic#40792)
  convert modules to use testclusters (elastic#40804)
  Replace javax activation with jakarta activation (elastic#40247)
  Document restrictions on fuzzy matching when using synonyms (elastic#40783)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 5, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into avx-512-jdk-10-only
874617c 
* elastic/master: (36 commits)
  Remove unneded cluster config from test (elastic#40856)
  Make Fuzziness reject illegal values earlier (elastic#33511)
  Remove test-only customisation from TransReplAct (elastic#40863)
  Fix dense/sparse vector limit documentation (elastic#40852)
  Make -try xlint warning disabled by default. (elastic#40833)
  Async Snapshot Repository Deletes (elastic#40144)
  Revert "Replace usages RandomizedTestingTask with built-in Gradle Test (elastic#40564)"
  Init global checkpoint after copy commit in peer recovery (elastic#40823)
  Replace usages RandomizedTestingTask with built-in Gradle Test (elastic#40564)
  [DOCS] Removed redundant (not quite right) information about upgrades.
  Remove string usages of old transport settings (elastic#40818)
  Rollup ignores time_zone on date histogram (elastic#40844)
  HLRC: fix uri encode bug when url path starts with '/' (elastic#34436)
  Mutes GatewayIndexStateIT.testRecoverBrokenIndexMetadata
  Docs: Pin two IDs in the rest client (elastic#40785)
  Adds version 6.7.2
  [DOCS] Remind users to include @ symbol when applying license file (elastic#40688)
  HLRC: Convert xpack methods to client side objects (elastic#40705)
  Allow ILM to stop if indices have nonexistent policies (elastic#40820)
  Add an OpenID Connect authentication realm (elastic#40674)
  ...
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
@roycarser
HLRC: fix uri encode bug when url path starts with '/' (elastic#34436)
6ab1879 
This commit sets the authority of a URI to blank such that it does not
misinterpret slashes in the path as the authority.

Closes elastic#34433
@O-R O-R mentioned this pull request Apr 23, 2020
Closed
2 tasks
@frnandu frnandu mentioned this pull request Jul 15, 2021
Closed
@iverase iverase mentioned this pull request Sep 2, 2021
Closed
@ern
Copy link

ern commented Jan 20, 2022

Seeing this issue in 6.8, any chance we can get this backported to 6.8 also?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hub-cap hub-cap hub-cap approved these changes

Assignees
No one assigned
Labels
>bug v7.2.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Rest High Level Client]:Error parse when document Id start with '/'
7 participants
@roycarser @elasticmachine @hub-cap @elasticcla @ern @colings86 @jakelandis