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

UriTemplate reserved expansion should not escape reserved characters #1838

Closed
cHengstler opened this issue Mar 28, 2023 · 12 comments · Fixed by #1844 or #1845
Closed

UriTemplate reserved expansion should not escape reserved characters #1838

cHengstler opened this issue Mar 28, 2023 · 12 comments · Fixed by #1844 or #1845
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@cHengstler
Copy link
Contributor

UriTemplate.expand should not escape reserved characters in values for reserved expansion (e.g., templates of the form "{+var}").

According to section of 3.2.3 Reserved Expansion: {+var}, reserved character should not be escaped.
Level 2 templates add the plus ("+") operator, for expansion of values that are allowed to include reserved URI characters (Section 1.5).

Example code:

    @Test
    public void testExpandTemplates_reservedExpansion_shouldNotEscapeReservedCharSet() {

        var genDelims = ":/?#[]@";
        var subDelims = "!$&'()*+,;=";
        var reservedSet = genDelims+subDelims;

        var requestMap = Map.of("var", reservedSet);

        assertThat(UriTemplate.expand("{+var}", requestMap, false), is(reservedSet));

        // Expected: is ":/?#[]@!$&'()*+,;="
        //     but: was ":/?%23%5B%5D@!$&'()*+,;="
        
    }
@suztomo
Copy link
Member

suztomo commented Apr 4, 2023

@cHengstler Do you observe problems when you use Google services due to this bug? If so, would you write some code that we can reproduce the problem?

@cHengstler
Copy link
Contributor Author

@suztomo, Thanks for your reply.

To reproducte the bug, I've already provided a test case in the description of this issue.
Allow me to add another example with a more realistic case

Lets say I define a url template https://www.example.com/{+resourceWithFragment}.
The resourceWithFragment literal can contain a fragment like page.html#section1.

The expectation would be that the UriTemplate.expand method produces a valid URL where the fragment is not encoded. This is indicated by the + operator (reserved expansion) according to RFC6570.

Unfortunately the result is: https://www.example.com/page.html%23section1 which does not work in browsers as expected.

    public String expandTemplate() {

        String template = "https://www.example.com/{+resourceWithFragment}";
        Map requestMap = Map.of("resourceWithFragment", "page.html#section1");

        return UriTemplate.expand(template, requestMap, false);
    }

@suztomo
Copy link
Member

suztomo commented Apr 5, 2023

The priority of the bug depends on how much suffer from it when using a real Google service rather than test cases. Which Google service do you have problem with?

@cHengstler
Copy link
Contributor Author

We are using this component directly within our project as a maven dependency.

@suztomo
Copy link
Member

suztomo commented Apr 6, 2023

It's good that you're not suffering from problem with Google service. We may revisit this issue in future.

@suztomo suztomo added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p3 Desirable enhancement or fix. May not be included in next release. labels Apr 6, 2023
@cHengstler
Copy link
Contributor Author

cHengstler commented Apr 17, 2023

Hi @suztomo
I could contribute with an PR to solve the issue.
Would that speed up the process?

Thanks
Christian

@suztomo
Copy link
Member

suztomo commented Apr 18, 2023

Yes, that's helpful. That would clarify the change you have mind.

https://github.com/googleapis/google-http-java-client/blob/main/CONTRIBUTING.md

@cHengstler
Copy link
Contributor Author

Hi @suztomo,

is there any chance to consider the PR #1844

Thanks & best regards
Christian

@suztomo
Copy link
Member

suztomo commented May 4, 2023

I havent’t checked that pull request yet. Thank you for heads up.

gcf-merge-on-green bot pushed a commit that referenced this issue May 5, 2023
…1844)

When using UriTemplate.expand with a reserved expansion "{+var}", a set of allowed characters must not  be encoded. According to section of [3.2.3  Reserved Expansion: {+var}](https://www.rfc-editor.org/rfc/rfc6570#section-3.2.3), unreserved and reserved character should not be escaped.

This fix adds the missing characters `#[]` that must not be percent encoded when using reserved expansion.

- [x] Ensure the tests and linter pass
- [x] Code coverage does not decrease (if any source code was changed)
- [x] Appropriate docs were updated (if necessary)

Fixes #1838
@suztomo
Copy link
Member

suztomo commented May 5, 2023

I merged #1844. We may have other tasks to include next release. Give me 1 week.

@cHengstler
Copy link
Contributor Author

I merged #1844. We may have other tasks to include next release. Give me 1 week.

That's great, thanks @suztomo.

@suztomo
Copy link
Member

suztomo commented May 15, 2023

@cHengstler Your change was released as 1.43.2 https://central.sonatype.com/artifact/com.google.http-client/google-http-client/1.43.2 Thank you for contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
2 participants