8000 [Security] Strengthen comparison of target_url vs login_path by mrzard · Pull Request #19026 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Strengthen comparison of target_url vs login_path #19026

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 22, 2017
Merged

[Security] Strengthen comparison of target_url vs login_path #19026

merged 1 commit into from
Mar 22, 2017

Conversation

mrzard
Copy link
@mrzard mrzard commented Jun 10, 2016 •
edited
Loading
Q A
Branch? "master"
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #18862
License MIT
Doc PR

@sstok
Copy link
Contributor
sstok commented Jun 12, 2016

Looks good to me 👍

Status: Reviewed

@ro0NL
Copy link
Contributor
ro0NL commented Jun 12, 2016

Not sure on this one.. but shouldn't we consider trailing slash normalization since the router, AFAIK, has some issues with it (#18702)

@cdesign
Copy link
cdesign commented Jun 12, 2016 •
edited
Loading

Minor issue, but there is a type in the test name: ('That' --> 'Than')

public function testRefererWithoutParametersHasToBeDifferentThatLoginUrl()

should be:

public function testRefererWithoutParametersHasToBeDifferentThanLoginUrl()

@mrzard
Copy link
Author
mrzard commented Jun 12, 2016 •
edited
Loading

@cdesign Thanks! Fixed. Fixed the original test name too.

@sstok thanks for taking the time to look at this.

@ro0NL Not very sure how to apply that detection as it is kind of "quirk" of the Router, any pointers? I'd be happy to actually code them.

@ro0NL ro0NL mentioned this pull request Jun 15, 2016
Closed
@ro0NL
Copy link
Contributor
ro0NL commented Jun 15, 2016

@mrzard i think it's a problem we cant fix.. as we cant determine the login_path route supports both variants yes or no. Currently how i understand it, it only is supported for imported routes.

I foresee some nasty side effects, but im not sure as i havent tested it, i.e. it needs to be reproduced first to properly document what no to do :-)

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 6, 2016
@fabpot
Copy link
Member
fabpot commented Mar 22, 2017

Thank you @mrzard.

@fabpot fabpot merged commit ac9d75a into symfony:master Mar 22, 2017
fabpot added a commit that referenced this pull request Mar 22, 2017
@fabpot
feature #19026 [Security] Strengthen comparison of target_url vs logi…
bafa8e2 
…n_path (mrzard)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Security] Strengthen comparison of target_url vs login_path

| Q | A |
| --- | --- |
| Branch? | "master" |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #18862 |
| License | MIT |
| Doc PR |  |

Commits
-------

ac9d75a [Security] Strengthen comparison of target_url vs login_path
@nicolas-grekas nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017
@fabpot fabpot mentioned this pull request May 1, 2017
Merged
@wcluijt wcluijt mentioned this pull request Jun 2, 2017
Closed
@pamil pamil mentioned this pull request Jul 5, 2017
Closed
@fabpot fabpot mentioned this pull request Jul 19, 2017
Merged
fabpot added a commit that referenced this pull request Jul 19, 2017
@fabpot
bug #23580 Fix login redirect when referer contains a query string (f…
f4172b0 
…abpot)

This PR was squashed before being merged into the 2.7 branch (closes #23580).

Discussion
----------

Fix login redirect when referer contains a query string

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19026, #23027, #23061, #23411, #23551
| License       | MIT
| Doc PR        | n/a

In 3.3, #19026 was merged to fix a bug that should have been fixed in 2.7. The fix was wrong anyway, so this PR fixes it the proper way.

The first two commits refactors test (using mocks for data objects is a bad idea and using too many mocks actually makes tests test nothing).

The actual fix is in the third commit.

Commits
-------

022ac0b [Security] added more tests
9c7a140 [Security] fixed default target path when referer contains a query string
b1f1ae2 [Security] simplified tests
3387612 [Security] refactored tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Security Status: Reviewed
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
7 participants
@mrzard @sstok @ro0NL @cdesign @fabpot @nicolas-grekas @carsonbot
0