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

dimabory · 2019-03-01T17:21:44Z

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.~~

src/Symfony/Component/Security/Http/Tests/Firewall/ExceptionListenerTest.php

curry684 · 2019-04-06T14:17:59Z

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

Fixes themself are fine.

Status: reviewed

curry684 · 2019-04-06T14:21:00Z

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

curry684 · 2019-04-06T14:49:42Z

@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.

nicolas-grekas · 2019-04-07T20:45:42Z

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

curry684 · 2019-04-08T08:48:48Z

@dimabory can you pick that up?

dimabory · 2019-04-08T13:07:28Z

@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 · 2019-04-08T16:47:06Z

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 · 2019-04-08T21:38:45Z

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 · 2019-04-09T07:10:04Z

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 · 2019-04-09T12:37:16Z

So what's the base branch 3.4 or master?

curry684 · 2019-04-09T13:12:31Z

3.4.

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

fabpot · 2019-04-10T06:03:33Z

Thank you @dimabory.

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 · 2019-04-10T07:06:16Z

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

fabpot · 2019-04-10T07:27:20Z

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

JarJak · 2019-04-16T16:04:06Z

@fabpot There probably is a need, see #31136

stof · 2019-04-16T16:41:56Z

src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php

@@ -131,8 +131,6 @@ private function handleAccessDeniedException(GetResponseForExceptionEvent $event
            } catch (\Exception $e) {
                $event->setException($e);
            }
-
-            return;


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

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...

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

…rule (dimabory)" This reverts commit fd1408b, reversing changes made to b93d2bf.

andrew-demb · 2019-04-17T14:41:38Z

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 · 2019-04-17T14:43:36Z

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 · 2019-04-17T14:58:11Z

@andrew-demb fix is coming, see #31142

…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)"

* 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

* 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

carsonbot added Status: Needs Review Security Bug labels Mar 1, 2019

dimabory marked this pull request as ready for review March 1, 2019 17:24

dimabory force-pushed the security-access-denied-rework branch from 9b2acc0 to 9073c94 Compare March 1, 2019 17:28

chalasr added this to the 3.4 milestone Mar 2, 2019

chalasr self-requested a review March 2, 2019 16:35

nicolas-grekas reviewed Mar 7, 2019

View reviewed changes

src/Symfony/Component/Security/Http/Tests/Firewall/ExceptionListenerTest.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Mar 7, 2019

View reviewed changes

src/Symfony/Component/Security/Http/Tests/Firewall/ExceptionListenerTest.php Outdated Show resolved Hide resolved

dimabory force-pushed the security-access-denied-rework branch 2 times, most recently from f1849b3 to 461b2a4 Compare March 7, 2019 08:47

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 6, 2019

michaelcullum added the ⭐️ EUFOSSA Hackathon label Apr 6, 2019

michaelcullum changed the base branch from master to 3.4 April 6, 2019 14:34

michaelcullum changed the base branch from 3.4 to master April 6, 2019 14:34

curry684 mentioned this pull request Apr 7, 2019

Custom AccessDeniedHandler didn't work #28229

Closed

nicolas-grekas removed the ⭐️ EUFOSSA Hackathon label Apr 8, 2019

dimabory changed the base branch from master to 3.4 April 9, 2019 14:28

Rework firewall access denied rule

5790859

dimabory force-pushed the security-access-denied-rework branch from 461b2a4 to 5790859 Compare April 9, 2019 15:06

chalasr approved these changes Apr 9, 2019

View reviewed changes

fabpot approved these changes Apr 10, 2019

View reviewed changes

fabpot merged commit 5790859 into symfony:3.4 Apr 10, 2019

This was referenced Apr 16, 2019

Release v3.4.25 #31123

Merged

Release v4.2.6 #31125

Merged

stof reviewed Apr 16, 2019

View reviewed changes

curry684 mentioned this pull request Apr 17, 2019

[3.4.25][BC Break] 403 instead of 401 for unauthorized users #31136

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 mentioned this pull request Apr 17, 2019

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

Merged

This was referenced Apr 17, 2019

Release v3.4.26 #31146

Merged

Release v4.2.7 #31148

Merged

nesk mentioned this pull request Nov 21, 2019

[Security] Rework firewall access denied rule #34476

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dimabory commented Mar 1, 2019 •

edited

curry684 commented Apr 6, 2019

curry684 commented Apr 6, 2019

curry684 commented Apr 6, 2019

nicolas-grekas commented Apr 7, 2019

curry684 commented Apr 8, 2019

dimabory commented Apr 8, 2019

curry684 commented Apr 8, 2019

chalasr commented Apr 8, 2019

curry684 commented Apr 9, 2019

dimabory commented Apr 9, 2019

curry684 commented Apr 9, 2019

fabpot commented Apr 10, 2019

dimabory commented Apr 10, 2019

fabpot commented Apr 10, 2019

JarJak commented Apr 16, 2019

stof Apr 16, 2019

curry684 Apr 16, 2019

JarJak Apr 16, 2019

andrew-demb commented Apr 17, 2019 •

edited

andrew-demb commented Apr 17, 2019 •

edited

JarJak commented Apr 17, 2019

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

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

Conversation

dimabory commented Mar 1, 2019 • edited

curry684 commented Apr 6, 2019

curry684 commented Apr 6, 2019

curry684 commented Apr 6, 2019

nicolas-grekas commented Apr 7, 2019

curry684 commented Apr 8, 2019

dimabory commented Apr 8, 2019

curry684 commented Apr 8, 2019

chalasr commented Apr 8, 2019

curry684 commented Apr 9, 2019

dimabory commented Apr 9, 2019

curry684 commented Apr 9, 2019

fabpot commented Apr 10, 2019

dimabory commented Apr 10, 2019

fabpot commented Apr 10, 2019

JarJak commented Apr 16, 2019

stof Apr 16, 2019

Choose a reason for hiding this comment

curry684 Apr 16, 2019

Choose a reason for hiding this comment

JarJak Apr 16, 2019

Choose a reason for hiding this comment

andrew-demb commented Apr 17, 2019 • edited

andrew-demb commented Apr 17, 2019 • edited

JarJak commented Apr 17, 2019

dimabory commented Mar 1, 2019 •

edited

andrew-demb commented Apr 17, 2019 •

edited

andrew-demb commented Apr 17, 2019 •

edited