[Security] Preserve URI fragment in HttpUtils::generateUri() #24203
Conversation
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 |
5818ebf to
0133c2e
Compare
|
This way of url generating causes more issues as it simply throws in all attributes: linaori/http-bundle#25 |
|
@iltar we should open another issue for that |
| @@ -150,7 +150,16 @@ public function generateUri($request, $path) | ||
| // fortunately, they all are, so we have to remove entire query string | ||
| $position = strpos($url, '?'); | ||
| if (false !== $position) { | ||
| $fragment = ''; |
There was a problem hiding this comment.
Could we use http://php.net/parse_url here instead of this custom logic?
| @@ -150,7 +150,16 @@ public function generateUri($request, $path) | ||
| // fortunately, they all are, so we have to remove entire query string |
There was a problem hiding this comment.
Should we update this comment too?
There was a problem hiding this comment.
dedicated comment added just before fragment handling
0133c2e to
34b9475
Compare
| @@ -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) { |
There was a problem hiding this comment.
Why is that necessary? What about a path like /foo/bar#fragment?
There was a problem hiding this comment.
Fragment gets stripped only if there is a query string, otherwise the urlGenerator result is returned directly
There was a problem hiding this comment.
Thanks for the clarification, but maybe we should add a test for that too to prevent regressions?
There was a problem hiding this comment.
In most cases there is always a query string, because it uses the attributes from the request to build it.
34b9475 to
4dd2e3e
Compare
…() (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()