Unsafe masking of a dynamic regex pattern
F3\FLOW3\Security\RequestPattern\Uri has method matchRequest() that uses a dynamic regex pattern.
return (boolean)preg_match('/^' . str_replace('/', '\/', $this->uriPattern) . '$/', $request->getRequestUri()->getPath());
Masking is done by str_replace(). This is not sufficient; think of a pattern that contains with '\/'. There's a dedicated method preq_quote() that takes care of proper masking.
So masking itself should be
Please grep through the complete code to possibly find similiar code; I haven't done that (I am using forge repository interface right now).
Updated by Karsten Dambekalns over 11 years ago
- Category set to Security
- Status changed from New to Needs Feedback
- Assignee set to Karsten Dambekalns
- Start date deleted (
- Estimated time set to 1.00 h
preg_quote() masks all characters that have a meaning in a regex. This is not wanted in this case, at least judging from the doc comments the pattern can be a full-fledged regex. Running that through
preg_quote() would render it useless.
The question remains, whether the
str_replace() is really helpful here. Either way the documentation needs to instruct the user on what happens to the pattern, so the right stuff can be fed into the class.
Updated by Andreas Förthner over 11 years ago
yes, I'm all for leaving it as a real regex configuration option. But you are right that the current situation is somehow inconsistent. On the other hand it's just convenient not to have to escape slashes in a URI pattern. What do you think, should we just add a note to the documentation that slashes will be automatically escaped? Or should we drop this automatic escaping and make a real regex pattern out of this option?