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

[Security] Preserve URI fragment in HttpUtils::generateUri() #24203

Merged
merged 1 commit into from Sep 17, 2017

Conversation

Projects
None yet
5 participants
@chalasr
Member

chalasr commented Sep 14, 2017

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #23675
License MIT
Doc PR n/a
@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Sep 14, 2017

Contributor

This way of url generating causes more issues as it simply throws in all attributes: iltar/http-bundle#25

Contributor

iltar commented Sep 14, 2017

This way of url generating causes more issues as it simply throws in all attributes: iltar/http-bundle#25

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Sep 14, 2017

Member

@iltar we should open another issue for that

Member

chalasr commented Sep 14, 2017

@iltar we should open another issue for that

@chalasr chalasr requested a review from javiereguiluz Sep 14, 2017

Show outdated Hide outdated src/Symfony/Component/Security/Http/HttpUtils.php
@@ -150,7 +150,16 @@ public function generateUri($request, $path)
// fortunately, they all are, so we have to remove entire query string

This comment has been minimized.

@javiereguiluz

javiereguiluz Sep 15, 2017

Member

Should we update this comment too?

@javiereguiluz

javiereguiluz Sep 15, 2017

Member

Should we update this comment too?

This comment has been minimized.

@chalasr

chalasr Sep 15, 2017

Member

dedicated comment added just before fragment handling

@chalasr

chalasr Sep 15, 2017

Member

dedicated comment added just before fragment handling

@@ -150,7 +150,12 @@ public function generateUri($request, $path)
// fortunately, they all are, so we have to remove entire query string
$position = strpos($url, '?');
if (false !== $position) {

This comment has been minimized.

@xabbuh

xabbuh Sep 15, 2017

Member

Why is that necessary? What about a path like /foo/bar#fragment?

@xabbuh

xabbuh Sep 15, 2017

Member

Why is that necessary? What about a path like /foo/bar#fragment?

This comment has been minimized.

@chalasr

chalasr Sep 15, 2017

Member

Fragment gets stripped only if there is a query string, otherwise the urlGenerator result is returned directly

@chalasr

chalasr Sep 15, 2017

Member

Fragment gets stripped only if there is a query string, otherwise the urlGenerator result is returned directly

This comment has been minimized.

@xabbuh

xabbuh Sep 15, 2017

Member

Thanks for the clarification, but maybe we should add a test for that too to prevent regressions?

@xabbuh

xabbuh Sep 15, 2017

Member

Thanks for the clarification, but maybe we should add a test for that too to prevent regressions?

This comment has been minimized.

@iltar

iltar Sep 15, 2017

Contributor

In most cases there is always a query string, because it uses the attributes from the request to build it.

@iltar

iltar Sep 15, 2017

Contributor

In most cases there is always a query string, because it uses the attributes from the request to build it.

This comment has been minimized.

@chalasr

chalasr Sep 15, 2017

Member

@xabbuh Test added

@chalasr

chalasr Sep 15, 2017

Member

@xabbuh Test added

@xabbuh

xabbuh approved these changes Sep 15, 2017

@chalasr chalasr merged commit 4dd2e3e into symfony:3.3 Sep 17, 2017

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

chalasr added a commit that referenced this pull request Sep 17, 2017

bug #24203 [Security] Preserve URI fragment in HttpUtils::generateUri…
…() (chalasr)

This PR was merged into the 3.3 branch.

Discussion
----------

[Security] Preserve URI fragment in HttpUtils::generateUri()

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

Commits
-------

4dd2e3e Preserve URI fragment in HttpUtils::generateUri()

@chalasr chalasr deleted the chalasr:auth-failure-path-keep-fragment branch Sep 17, 2017

@fabpot fabpot referenced this pull request Oct 5, 2017

Merged

Release v3.3.10 #24452

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