Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Security] Add a normalization step for the user-identifier in firewalls#51744
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
aec0d91
to180fd7a
Comparesrc/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
25e91d6
to782ef6a
Compare782ef6a
to7bda5b3
CompareRebased to target Symfony 7.1 |
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.
needs a changelog entry :)
src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Http/Tests/Authenticator/Passport/Badge/UserBadgeTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
ed19a61
toadb4098
Compare
I checked how the built-in user providers could handle this, but I'm not sure if any of them can be changed as well. |
What's the next step here? How do we make it easy to opt-in for the OWASP-recommended normalization? |
adb4098
to4f87434
Compare
Hello@nicolas-grekas, This PR is now up to date, rebased against the 7.3 branch, and squashed. It’s ready for review. The next step is to finalize the documentation. It should demonstrate how to inject a normalizer to ensure that different variations (e.g., BR. |
254b8cd
tof81db2a
CompareCan you give us a hint about how this can be used? : ) |
Spomky commentedJan 6, 2025 • 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.
This feature will most likely be used by the firewall Authenticator service that creates the UserBadge, but it could also be used by the Below is a simple example of how to implement a namespaceApp\Security;useSymfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;useSymfony\Component\String\UnicodeString;usefunctionSymfony\Component\String\u;finalclass NormalizedUserBadgeextends UserBadge{publicfunction__construct(string$identifier) {$callback =staticfn (string$identifier) =>u($identifier)->normalize(UnicodeString::NFKC)->ascii()->lower()->toString();parent::__construct($identifier, identifierNormalizer:$callback); }} From the Authenticator: <?phpnamespaceApp\Security;finalclass PasswordAuthenticatorextends AbstractLoginFormAuthenticator{// Simplified for brievetypublicfunctionauthenticate(Request$request):Passport {$username = (string)$request->request->get('username','');$password = (string)$request->request->get('password','');$request->getSession() ->set(SecurityRequestAttributes::LAST_USERNAME,$username);returnnewPassport(newNormalizedUserBadge($username),newPasswordCredentials($password), [//All other useful badges ] ); } When a user attempts to log in with “John.Doe” or “JOhn.doe,” both will be treated as “john.doe.” Another potential use case involves scenarios where users can log in with various forms of the same identifier: $callback =staticfunction (string$identifier) {$normalized =u($identifier)->normalize(UnicodeString::NFKC)->ascii()->lower()->toString();$username ='';$domain ='';match (true) {str_contains($normalized,'@') => [$localPart,$domain =explode('@',$normalized,2),$username =str_contains($localPart,'.') ?substr(explode('.',$localPart,2)[0],0,1) .explode('.',$localPart,2)[1] :$localPart, ],str_contains($normalized,'\\') => [$domain,$username =explode('\\',$normalized,2) ],str_contains($normalized,'://') => [$parts =parse_url($normalized),isset($parts['host'],$parts['path']) && [$domain =$parts['host'],$username =ltrim($parts['path'],'/') ] ],default =>$username =$normalized };$username =preg_replace('/[^a-z0-9]/','',$username);return$domain .'\\' .$username; }; |
f81db2a
to149754a
CompareThere 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.
Thanks for this@Spomky!
I think the documentation of this feature must provide real life example to help users to use it properly (I think mostly about username policy of Google wherefoo.bar@gmail.com
,foobar@gmail.com
, andf.o.o.b.a.r@gmail.com
refers to the same user).
For the code, it's a 👍 for me
149754a
todb01811
CompareThank you@Spomky. |
e04b670
intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
As a next step, I'm wondering if we should somehow define some reusable normalizers that developers can use without reinventing the wheel (like the Google strategy mentioned above). |
Uh oh!
There was an error while loading.Please reload this page.
With this PR, the user identifier is lowered and normalized to make sure they are case-insensitive.
When enable, please note that the user provider or the user repository should be adapt:
This is a very first attempt to resolve issue#49269. Any comments and reactions are welcome.
See alsohttps://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#user-ids