Skip to content

Fix login redirect when referer contains a query string #23580

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

Merged
merged 4 commits into from
Jul 19, 2017

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Jul 19, 2017

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.

@nicolas-grekas
Copy link
Member

👍

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

cant agree more :)

Copy link
Contributor

@pamil pamil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to see it fixed, thanks! 🎉

@fabpot fabpot merged commit 022ac0b into symfony:2.7 Jul 19, 2017
fabpot added a commit that referenced this pull request Jul 19, 2017
…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
xabbuh added a commit to symfony/symfony-docs that referenced this pull request Jul 21, 2017
…uiluz)

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

Discussion
----------

Reworded the article about form login redirects

Now that form login redirects have been fully fixed (see symfony/symfony#23580) I thought about updating this article, specially its structure.

All changes are simple rewordings, except this one: previously, the article said that you can use a Symfony route name as the value of the `_target_path` parameter in the query string or the hidden form field. But if you check the code of this feature, it looks like you can't because we use the value of that parameter "as is" to redirect, so it must be a relative/absolute URL, right?

```php
protected function determineTargetUrl(Request $request)
{
    if ($this->options['always_use_default_target_path']) {
        return $this->options['default_target_path'];
    }

    // We redirect directly to the value of the parameter, so it can't be a route name, right ????
    if ($targetUrl = $request->get($this->options['target_path_parameter'], null, true)) {
        return $targetUrl;
    }

    // ...
}
```

Commits
-------

5015723 Reworded the article about form login redirects
@fabpot fabpot deleted the target_url branch September 11, 2017 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants