[Security] Make Login Rate Limiter also case insensitive for non-ascii user identifiers #41173
Conversation
| return [ | ||
| $this->globalFactory->create($request->getClientIp()), | ||
| $this->localFactory->create(strtolower($request->attributes->get(Security::LAST_USERNAME)).'-'.$request->getClientIp()), | ||
| $this->localFactory->create(mb_strtolower($request->attributes->get(Security::LAST_USERNAME)).'-'.$request->getClientIp()), |
There was a problem hiding this comment.
we should not rely on the global php.ini config
this should instead be:
\function_exists('mb_strtolower') && preg_match('//u', $v) ? mb_strtolower($v, 'UTF-8') : strtolower($v);
There was a problem hiding this comment.
Do we need function_exists('mb_strtolower')? The polyfill should make sure that the method exists, shouldn't it?
There was a problem hiding this comment.
@derrabus that's my understanding too.
@nicolas-grekas are you sure about passing 'UTF-8'? I don't mind changing, but seeing as utf-8 has been the default for a while, and if someone explicitly sets it to something else I'd assume they have a good reason and maybe it makes sense to follow their setting.
There was a problem hiding this comment.
true, about function_exists
for the ini setting, I think the code base is decoupled from it and we always use that kind of calls.
no global state rulez :)
There was a problem hiding this comment.
Ok done.. I kept the preg_match() guard, not sure if it's really needed or not? Is the polyfill not enough?
…i user identifiers
nicolas-grekas
changed the title
|
Thank you Jordi. |
5 participants
As per discussion in #41156