[Security] Make statefull pages NOT turn responses private when the token is not used #28089

nicolas-grekas · 2018-07-30T17:23:40Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#26769 et al.
License	MIT
Doc PR	-

Replaces #27817
By taking over session usage tracking and replacing it with token usage tracking, we can prevent responses that don't actually use the token from turning responses private without changing anything to the lifecycle of security listeners. This makes the behavior much more seamless, allowing to still log the user with the monolog processor, and display it in the profiler toolbar.

nicolas-grekas · 2018-07-30T20:01:14Z

Now tested and green, ready.

dmaicher · 2018-07-30T20:13:22Z

src/Symfony/Bridge/Monolog/Processor/TokenProcessor.php

@@ -30,7 +30,7 @@ public function __construct(TokenStorageInterface $tokenStorage)
    public function __invoke(array $records)
    {
        $records['extra']['token'] = null;
-        if (null !== $token = $this->tokenStorage->getToken()) {
+        if (null !== $token = $this->tokenStorage->getToken(false)) {


I have the feeling this is a step backwards in terms of code quality 😢

When I see this and then look at the interface I'm confused:

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authentication/Token/Storage/TokenStorageInterface.php#L28

So here we kind of code against the implementation UsageTrackingTokenStorage and not against the abstraction/interface anymore.

Also if someone decorated the original security.token_storage service this will not work properly, right? (as the argument will be missing in any parent::getToken() call because its simply not part of the contract).

we kind of code against the implementation UsageTrackingTokenStorage

fixed by adding a new UsageTrackingTokenStorageInterface

if someone decorated the original security.token_storage service this will not work properly, right?

The behavior will be the same as currently. That's the typical downside with service decoration: you're required to adapt to changes done at the decorated level. It's not very different from being required to write new code to leverage any new features. There is no way around.

the issue here is that your new interface adds a new argument to the same method, making it really weird for decorators

I don't understand what/why this is weird. This works fine.

@stof I think that's OK. We don't want ppl to have to use two different methods for fetching the token. And this is in a separate interface (UsageTrackingTokenStorageInterface) so that's documented. We don't want ppl to decorate blindly btw.

nicolas-grekas · 2018-09-03T08:23:25Z

I'm putting this on-hold because I think we can get rid of the UsageTrackingTokenStorageInterface.

Status: needs work

…oken is not used

fabpot · 2018-09-26T05:54:27Z

Any news on this one? Can we close #27817?

nicolas-grekas · 2018-09-26T07:04:38Z

Not yet, I'll keep you posted.

Plopix · 2019-09-07T20:31:48Z

hello, I have just encountered this "issue".
Something is starting the session even if we don't use it which triggers private page. (Firewall?)

Which is not cool for performances :D

My current workaround if (useful for others) is a listener where I disabled this feature for now

public function onKernelResponse(ResponseEvent $event): void
    {
        $event->getResponse()->headers->set(AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER, 'true');
    }

Note that I agree with the concept when this will work.

If we read the session, then that's private. That makes sense ;) (but only if we really read it)

wadjeroudi · 2019-09-18T05:45:54Z

@nicolas-grekas using the usageIndex, seems like a hack. Could it be better to load/start the session only when the token is really used ?
The token would be a callback that would load/unserialize the token only if "getted". So the session won't start if nobody uses the token.

nicolas-grekas · 2019-09-21T18:23:52Z

@wadjeroudi that wouldn't work as soon as subrequests are involved, especially when HttpCache is used with ESI.

nicolas-grekas · 2019-09-22T20:57:51Z

Continued in #33663, please review there now.

…ate only when needed (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Security] Make stateful firewalls turn responses private only when needed | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26769 *et al.* | License | MIT | Doc PR | - Replaces #28089 By taking over session usage tracking and replacing it with token usage tracking, we can prevent responses that don't actually use the token from turning responses private without changing anything to the lifecycle of security listeners. This makes the behavior much more seamless, allowing to still log the user with the monolog processor, and display it in the profiler toolbar. This works by using two separate token storage services: - `security.token_storage` now tracks access to the token and increments the session usage tracker when needed. This is the service that is injected in userland. - `security.untracked_token_storage` is a raw token storage that just stores the token and is disconnected from the session. This service is injected in places where reading the session doesn't impact the generated output in any way (as e.g. in Monolog processors, etc.) Commits ------- 20df3a1 [Security] Make stateful firewalls turn responses private only when needed

nicolas-grekas added this to the next milestone Jul 30, 2018

carsonbot added Status: Needs Review Security Feature labels Jul 30, 2018

nicolas-grekas mentioned this pull request Jul 30, 2018

[Security] add "lazy_authentication" mode to firewalls #27817

Closed

2 tasks

nicolas-grekas force-pushed the sec-dis-track branch 5 times, most recently from d53fee4 to e47281f Compare July 30, 2018 19:48

nicolas-grekas force-pushed the sec-dis-track branch from e47281f to 6fc67d4 Compare July 30, 2018 20:06

dmaicher reviewed Jul 30, 2018

View reviewed changes

nicolas-grekas force-pushed the sec-dis-track branch 3 times, most recently from 9acbebc to 2ee4f02 Compare July 31, 2018 07:57

nicolas-grekas mentioned this pull request Aug 31, 2018

Close session after fetching token in ContextListener #28325

Closed

carsonbot added Status: Needs Work and removed Status: Needs Review labels Sep 3, 2018

[Security] Make statefull pages NOT turn responses private when the t…

4b1277a

…oken is not used

nicolas-grekas force-pushed the sec-dis-track branch from 2ee4f02 to 4b1277a Compare September 5, 2018 09:54

nicolas-grekas changed the base branch from master to 4.4 June 2, 2019 20:04

nicolas-grekas mentioned this pull request Sep 22, 2019

[Security] Make stateful firewalls turn responses private only when needed #33663

Merged

nicolas-grekas closed this Sep 22, 2019

nicolas-grekas deleted the sec-dis-track branch September 22, 2019 20:58

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Security] Make statefull pages NOT turn responses private when the token is not used #28089

[Security] Make statefull pages NOT turn responses private when the token is not used #28089

Uh oh!

nicolas-grekas commented Jul 30, 2018 •

edited

Loading

Uh oh!

nicolas-grekas commented Jul 30, 2018 •

edited

Loading

Uh oh!

dmaicher Jul 30, 2018

Uh oh!

dmaicher Jul 30, 2018 •

edited

Loading

Uh oh!

nicolas-grekas Jul 31, 2018

Uh oh!

stof Aug 20, 2018

Uh oh!

nicolas-grekas Aug 20, 2018 •

edited

Loading

Uh oh!

nicolas-grekas Aug 21, 2018

Uh oh!

nicolas-grekas commented Sep 3, 2018

Uh oh!

fabpot commented Sep 26, 2018

Uh oh!

nicolas-grekas commented Sep 26, 2018

Uh oh!

Plopix commented Sep 7, 2019

Uh oh!

wadjeroudi commented Sep 18, 2019

Uh oh!

nicolas-grekas commented Sep 21, 2019

Uh oh!

nicolas-grekas commented Sep 22, 2019

Uh oh!

Uh oh!

Uh oh!

[Security] Make statefull pages NOT turn responses private when the token is not used #28089

[Security] Make statefull pages NOT turn responses private when the token is not used #28089

Uh oh!

Conversation

nicolas-grekas commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmaicher Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

dmaicher Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

stof Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Sep 3, 2018

Uh oh!

fabpot commented Sep 26, 2018

Uh oh!

nicolas-grekas commented Sep 26, 2018

Uh oh!

Plopix commented Sep 7, 2019

Uh oh!

wadjeroudi commented Sep 18, 2019

Uh oh!

nicolas-grekas commented Sep 21, 2019

Uh oh!

nicolas-grekas commented Sep 22, 2019

Uh oh!

Uh oh!

nicolas-grekas commented Jul 30, 2018 •

edited

Loading

nicolas-grekas commented Jul 30, 2018 •

edited

Loading

dmaicher Jul 30, 2018 •

edited

Loading

nicolas-grekas Aug 20, 2018 •

edited

Loading