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

GenericUrl converts plus sign into space within URI path component #398

Closed
vasiliy-bout opened this issue Feb 28, 2018 · 3 comments · Fixed by #913
Closed

GenericUrl converts plus sign into space within URI path component #398

vasiliy-bout opened this issue Feb 28, 2018 · 3 comments · Fixed by #913
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. status: blocked Resolving the issue is dependent on other work. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@vasiliy-bout
Copy link

This is a simple junit test which fails but should work according to https://tools.ietf.org/html/rfc3986#section-3.3 because + sign is allowed to be used within a path component and has no special meaning when used within a path component:

    @Test
    public void testGenericUrl() throws URISyntaxException {
        final URI uri = new URI("http://foo.bar/path+with+plus");

        final GenericUrl genericUrl = new GenericUrl(uri);

        Assert.assertEquals("/path+with+plus", genericUrl.getRawPath());
        Assert.assertEquals(ImmutableList.of("", "path+with+plus"), genericUrl.getPathParts());
    }

This bug is related to #255 but shows different behaviour. We are using google-http-client library version 1.23.0:

        <dependency>
            <groupId>com.google.http-client</groupId>
            <artifactId>google-http-client</artifactId>
            <version>1.23.0</version>
        </dependency>

From the first glance on sources, it looks like there is a bug inside toPathParts methods implementation on line 550: https://github.com/google/google-http-java-client/blob/110b3ab6fa7838dbe9926038427c712db9ed8aae/google-http-client/src/main/java/com/google/api/client/http/GenericUrl.java#L545-L553

There a call is made to CharEscapers.decodeUri which is invalid, because JavaDoc of this method explicitly states that it must not be used for path segments (see the second paragraph): https://github.com/google/google-http-java-client/blob/110b3ab6fa7838dbe9926038427c712db9ed8aae/google-http-client/src/main/java/com/google/api/client/util/escape/CharEscapers.java#L78-L90

@mattwhisenhunt
Copy link
Contributor

You are exactly right about how and where this is broken.

I'm marking that as wont-fix because the main purpose of this library is to connect to the 100+ Google APIs and other GenericURL unit tests seem to suggest that at least some of those APIs have non-conforming paths.

@vasiliy-bout are you hitting this bug while connecting to one of the Google APIs?

@mattwhisenhunt mattwhisenhunt added status: will not fix type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Mar 15, 2018
@vasiliy-bout
Copy link
Author

vasiliy-bout commented Mar 16, 2018 via email

@googleapis googleapis deleted a comment from rizarahman Mar 28, 2018
@JustinBeckwith JustinBeckwith added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed status: will not fix labels Jun 6, 2018
@JustinBeckwith JustinBeckwith added the 🚨 This issue needs some love. label Aug 27, 2018
@chingor13 chingor13 self-assigned this Aug 28, 2018
ajaaym added a commit to ajaaym/google-http-java-client that referenced this issue Dec 4, 2018
@ajaaym ajaaym added the status: will not fix Invalid (untrue/unsound/erroneous), inconsistent with product, not on roadmap. label Dec 10, 2018
@chingor13 chingor13 added status: blocked Resolving the issue is dependent on other work. and removed status: will not fix Invalid (untrue/unsound/erroneous), inconsistent with product, not on roadmap. labels Dec 13, 2018
@donghanmiao
Copy link

donghanmiao commented Jun 19, 2019

ping on this.

Our Google CloudSearch Connectors SDK is having problem with this issue in at least on of our API
https://developers.google.com/cloud-search/docs/reference/rest/v1/indexing.datasources.items/push

when customer resource name contains '+', it was incorrectly converted into space.

@codyoss codyoss self-assigned this Dec 10, 2019
codyoss added a commit to codyoss/google-http-java-client that referenced this issue Dec 10, 2019
The old implementation was incorrecly treating '+' as a space. Now
the only things that get decoded in the path are uri escaped sequences.

Fixes googleapis#398
codyoss added a commit to codyoss/google-http-java-client that referenced this issue Dec 10, 2019
The old implementation was incorrecly treating '+' as a space. Now
the only things that get decoded in the path are uri escaped sequences.

Fixes googleapis#398
chingor13 pushed a commit that referenced this issue Dec 17, 2019
* feat: decode uri path components correctly

The old implementation was incorrecly treating '+' as a space. Now
the only things that get decoded in the path are uri escaped sequences.

Fixes #398

* tweak javadoc

* remove hardcoded string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. status: blocked Resolving the issue is dependent on other work. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants