10000 [Security] Rework firewall's access denied rule by dimabory · Pull Request #30423 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Rework firewall's access denied rule #30423

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
Apr 10, 2019
Merged

[Security] Rework firewall's access denied rule #30423

merged 1 commit into from
Apr 10, 2019

Conversation

dimabory
Copy link
Contributor
@dimabory dimabory commented Mar 1, 2019 •
edited
Loading
Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30099, #28229
License MIT
Doc PR

Follow tickets provided above to reproduce bugs. (there are also some project examples)

In addition, I'm looking for someone who knows an answer to this regarding rework in this PR.

@dimabory 10000 dimabory marked this pull request as ready for review March 1, 2019 17:24
@dimabory dimabory force-pushed the security-access-denied-rework branch from 9b2acc0 to 9073c94 Compare March 1, 2019 17:28
@chalasr chalasr added this to the 3.4 milestone Mar 2, 2019
@chalasr chalasr self-requested a review March 2, 2019 16:35
@dimabory dimabory force-pushed the security-access-denied-rework branch 2 times, most recently from f1849b3 to 461b2a4 Compare March 7, 2019 08:47
@curry684
Copy link
Contributor
curry684 commented Apr 6, 2019

Please use a single PR per issue next time, easier to review.

Fixes themself are fine.

Status: reviewed

@curry684
Copy link
Contributor
curry684 commented Apr 6, 2019

Actually it shouldn't be based on master as it's a bugfix. I'll see if I can rebase it on 3.4.

@michaelcullum michaelcullum changed the base branch from master to 3.4 April 6, 2019 14:34
@michaelcullum michaelcullum changed the base branch from 3.4 to master April 6, 2019 14:34
@curry684
Copy link
Contributor
curry684 commented Apr 6, 2019

@chalasr I just discussed with @michaelcullum that I think we should consider merging it into master anyway despite being a bugfix, as older applications may actually be accidentally relying on the broken behavior.

So I'd prefer putting it into master instead and documenting it as a B/C breaking bugfix in the upgrade guide.

@curry684 curry684 mentioned this pull request Apr 7, 2019
Closed
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 7, 2019 8000

Let's add some notes in the CHANGELOG.md file of the component, and give some instructions in UPGRADE-4.3.md then

@curry684
Copy link
Contributor
curry684 commented Apr 8, 2019

@dimabory can you pick that up?

@dimabory
Copy link
Contributor Author
dimabory commented Apr 8, 2019

@curry684, @nicolas-grekas I'm not sure about B/C breaking. Can you guide me on what exactly can be broken with this change?

@curry684
Copy link
Contributor
curry684 commented Apr 8, 2019

The change may cause the AccessDeniedHandler to be executed now in cases where it wasn't before, and applications may depend on the previous erroneous behavior to some degree.

Technically everything that changes existing behavior is to be considered BC breakage, but in this case the old behavior was technically wrong and out of spec, hence why we're accepting the fix in a minor release, but not backporting it. But the PR should mention it in the upgrade guide.

@chalasr
Copy link
Member
chalasr commented Apr 8, 2019

Looking at the patch and the fixed ticket, I think we should merge in 3.4, every bugfix breaks someone's behavior.
Calling the access denied handler doesn't sound harmful, is it?

@curry684
Copy link
Contributor
curry684 commented Apr 9, 2019

I'm mainly hesitant because it'll be a patch level change in low level security behavior. I'm hard pressed to find a harmful scenario but I prefer to err on the side of caution where access control is involved... 😉

@dimabory
Copy link
Contributor Author
dimabory commented Apr 9, 2019

So what's the base branch 3.4 or master?

@curry684
Copy link
Contributor
curry684 commented Apr 9, 2019

3.4.

Technically it's correct for a bugfix, I won't object to it and @chalasr is the boss of security component 😉

@dimabory dimabory changed the base branch from master to 3.4 April 9, 2019 14:28
@dimabory dimabory force-pushed the security-access-denied-rework branch from 461b2a4 to 5790859 Compare April 9, 2019 15:06
@fabpot
Copy link
Member
fabpot commented Apr 10, 2019

Thank you @dimabory.

@fabpot fabpot merged commit 5790859 into symfony:3.4 Apr 10, 2019
fabpot added a commit that referenced this pull request Apr 10, 2019
@fabpot
bug #30423 [Security] Rework firewall's access denied rule (dimabory)
fd1408b 
This PR was merged into the 3.4 branch.

Discussion
----------

[Security] Rework firewall's access denied rule

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~~#30099~~, #28229
| License       | MIT
| Doc PR        |

Follow tickets provided above to reproduce bugs. (there are also some project examples)

~~In addition, I'm looking for someone who knows an answer to [this](#30099 (comment)) regarding rework in this PR.~~

Commits
-------

5790859 Rework firewall access denied rule
@dimabory
Copy link
Contributor Author

Excuse me, guys, but I didn't update CHANGELOG.md and UPGRADE.md (like mentioned @curry684).

@fabpot
Copy link
Member
fabpot commented Apr 10, 2019

@dimabory As this is a bug fix, there is no need to update the CHANGELOG/UPGRADE files.

This was referenced Apr 16, 2019
Merged
Merged
@JarJak
Copy link
JarJak commented Apr 16, 2019

@fabpot There probably is a need, see #31136

stof
stof reviewed Apr 16, 2019
View reviewed changes
src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php
@@ -131,8 +131,6 @@ private function handleAccessDeniedException(GetResponseForExceptionEvent $event
} catch (\Exception $e) {
$event->setException($e);
}

return;
Copy link
Member

Choose a reason for hiding this comment

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

this now calls the accessDeniedHandler even when we call the entry point, which looks weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, but it's pretty much technically in line given the rest of the implementation (it also logs at 123, same thing). Not really trivial to fix given how the flow works...

Copy link

Choose a reason for hiding this comment

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

So if entry point returns 403 response you get endless redirect loop

@curry684 curry684 mentioned this pull request Apr 17, 2019
Closed
chalasr pushed a commit to chalasr/symfony that referenced this pull request Apr 17, 2019
Revert "bug symfony#30423 [Security] Rework firewall's access denied …
cd77f6f 
…rule (dimabory)"

This reverts commit fd1408b, reversing
changes made to b93d2bf.
@dimabory dimabory mentioned this pull request Apr 17, 2019
Merged
@andrew-demb
Copy link
Contributor
andrew-demb commented Apr 17, 2019 •
edited
Loading

These changes make log with message "Access denied, the user is neither anonymous, nor remember-me."(https://github.com/chalasr/symfony/blob/fd1408b13869a381fbebf9b9967f7a80e8b141d3/src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php#L137) lying, because token may be anonymous on !$this->authenticationTrustResolver->isFullFledged($token) (https://github.com/chalasr/symfony/blob/fd1408b13869a381fbebf9b9967f7a80e8b141d3/src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php#L121).

@andrew-demb
Copy link
Contributor
andrew-demb commented Apr 17, 2019 •
edited
Loading

Also with this PR symfony call access denied handler without any context, that response already generated for anonymous token.

In my case my handler doesn't expect calling for anonymous token and I got an infinite redirect.

I think it's BC break.

@JarJak
Copy link
JarJak commented Apr 17, 2019

@andrew-demb fix is coming, see #31142

fabpot pushed a commit that referenced this pull request Apr 17, 2019
@nicolas-grekas
bug #31142 Revert "bug #30423 [Security] Rework firewall's access den…
55a21fb 
…ied rule (dimabory)" (chalasr)

This PR was merged into the 3.4 branch.

Discussion
----------

Revert "bug #30423 [Security] Rework firewall's access denied rule (dimabory)"

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

Commits
-------

cd77f6f Revert "bug #30423 [Security] Rework firewall's access denied rule (dimabory)"
fabpot pushed a commit that referenced this pull request Apr 17, 2019
@nicolas-grekas
Merge branch '3.4' into 4.2
6aec217 
* 3.4:
  Revert "bug #30423 [Security] Rework firewall's access denied rule (dimabory)"
  [FrameworkBundle] minor: remove a typo from changelog
  [VarDumper][Ldap] relax some locally failing tests
  [Validator] #30192 Added the missing translations for the Tagalog ("tl") locale.
  Make MimeTypeExtensionGuesser case insensitive
fabpot pushed a commit that referenced this pull request Apr 17, 2019
@nicolas-grekas
Merge branch '4.2'
8d96861 
* 4.2:
  Revert "bug #30423 [Security] Rework firewall's access denied rule (dimabory)"
  [FrameworkBundle] minor: remove a typo from changelog
  [VarDumper] fix tests with ICU 64.1
  [VarDumper][Ldap] relax some locally failing tests
  [Validator] #30192 Added the missing translations for the Tagalog ("tl") locale.
  Make MimeTypeExtensionGuesser case insensitive
  Fix get session when the request stack is empty
  [Routing] fix trailing slash redirection with non-greedy trailing vars
  [FrameworkBundle] decorate the ValidatorBuilder's translator with LegacyTranslatorProxy
This was referenced Apr 17, 2019
Merged
Merged
@nesk nesk mentioned this pull request Nov 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@curry684 curry684 curry684 left review comments

@JarJak JarJak JarJak left review comments

@fabpot fabpot fabpot approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug Security Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
10 participants
@dimabory @curry684 @nicolas-grekas @chalasr @fabpot @JarJak @andrew-demb @stof @michaelcullum @carsonbot
0