Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Make statefull pages NOT turn responses private when the token is not used#28089
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
d53fee4 toe47281fComparenicolas-grekas commentedJul 30, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Now tested and green, ready. |
| { | ||
| $records['extra']['token'] =null; | ||
| if (null !==$token =$this->tokenStorage->getToken()) { | ||
| if (null !==$token =$this->tokenStorage->getToken(false)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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:
So here we kind of code against the implementationUsageTrackingTokenStorage and not against the abstraction/interface anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also if someone decorated the originalsecurity.token_storage service this will not work properly, right? (as the argument will be missing in anyparent::getToken() call because its simply not part of the contract).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we kind of code against the implementation UsageTrackingTokenStorage
fixed by adding a newUsageTrackingTokenStorageInterface
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the issue here is that your new interface adds a new argument to thesame method, making it really weird for decorators
nicolas-grekasAug 20, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't understand what/why this is weird. This works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
9acbebc to2ee4f02Comparenicolas-grekas commentedSep 3, 2018
I'm putting this on-hold because I think we can get rid of the Status: needs work |
2ee4f02 to4b1277aComparefabpot commentedSep 26, 2018
Any news on this one? Can we close#27817? |
nicolas-grekas commentedSep 26, 2018
Not yet, I'll keep you posted. |
Plopix commentedSep 7, 2019
hello, I have just encountered this "issue". Which is not cool for performances :D My current workaround if (useful for others) is a listener where I disabled this feature for now 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 commentedSep 18, 2019
@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 ? |
nicolas-grekas commentedSep 21, 2019
@wadjeroudi that wouldn't work as soon as subrequests are involved, especially when |
nicolas-grekas commentedSep 22, 2019
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#28089By 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
Uh oh!
There was an error while loading.Please reload this page.
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.