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

[HttpKernel] Correctly Render Signed URIs Containing Fragments #29679

Merged
merged 1 commit into from Jan 5, 2019

Conversation

Projects
None yet
4 participants
@zanbaldwin
Copy link
Contributor

zanbaldwin commented Dec 24, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no?
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a
  • Rebuild the URL with the computed hash instead of appending it onto the end of the fragment.
  • Update unit tests, and add new unit test to cover URIs that include fragments.

@zanbaldwin zanbaldwin changed the title Correctly Render Signed URIs Containing Fragments [HttpKernel] Correctly Render Signed URIs Containing Fragments Dec 24, 2018

[HttpKernel] Correctly Render Signed URIs Containing Fragments
Rebuild the URL with the computed hash instead of appending it onto the end of the URI, preventing incorrect formatting when dealing with URIs containing fragments.

@zanbaldwin zanbaldwin force-pushed the zanbaldwin:feature/uri-signer-fragment branch from e4cfa4e to b9ece6b Dec 24, 2018

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Dec 24, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 25, 2018

Hi Zan, I'm not sure I get what this fixes? From the description, I feel like this is just a different way to achieve the same. Can you give some more hints please? Thanks :)

@zanbaldwin

This comment has been minimized.

Copy link
Contributor

zanbaldwin commented Dec 25, 2018

Sorry, I should have given an example!
If a URI contained a fragment, the resulting signed URL would have resulted in the hash query parameter appended to the URI string (after the fragment), instead of being placed in the query string part of the URL. For example:

Before: http://example.com/path?foo=bar#fragment&_hash=$HASH

After: http://example.com/path?foo=bar&_hash=$HASH#fragment

@fabpot

fabpot approved these changes Dec 29, 2018

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 5, 2019

Thank you @zanbaldwin.

@fabpot fabpot merged commit b9ece6b into symfony:3.4 Jan 5, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jan 5, 2019

bug #29679 [HttpKernel] Correctly Render Signed URIs Containing Fragm…
…ents (zanbaldwin)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] Correctly Render Signed URIs Containing Fragments

| Q             | A
| ------------- | ---
| Branch?       | `3.4`
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no?
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

- Rebuild the URL with the computed hash instead of appending it onto the end of the fragment.
- Update unit tests, and add new unit test to cover URIs that include fragments.

Commits
-------

b9ece6b [HttpKernel] Correctly Render Signed URIs Containing Fragments

This was referenced Jan 6, 2019

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