Skip to content

Conversation

Majkl578
Copy link
Contributor

@staabm
Copy link
Contributor

staabm commented Oct 10, 2017

Does quoting # work when not in extended mode?

@Majkl578
Copy link
Contributor Author

Majkl578 commented Oct 10, 2017

It behaves the same in extended and normal mode: https://3v4l.org/f1eg0

Quoting documentation:

For example, if you want to match a "*" character, you write "\*" in the pattern. This applies whether or not the following character would otherwise be interpreted as a meta-character, so it is always safe to precede a non-alphanumeric with "\" to specify that it stands for itself.

@cmb69
Copy link
Member

cmb69 commented Oct 11, 2017

I'm somewhat concerned regarding the potential BC break. What if existing code is already properly escaping the #? E.g. preg_match('~^(' . preg_quote('hello\#world', '~') . ')\z~x', 'hello#world') would return 0 with this patch applied.

@Majkl578
Copy link
Contributor Author

Majkl578 commented Oct 11, 2017

Your code is not working now as well because \ gets escaped anyway: https://3v4l.org/O8B5o (without x mode to avoid broken regex)
You can't properly escape # before preg_quote(), only afterwards by e.g. addcslashes(preg_quote(...), '#').
Such code would be broken by this PR I suppose. Yet this is still a bug that should be fixed at some point.
BC concern is legit, although it's a rare use / edge case.
Absence of escaping # could even be considered a security risk in some scenarios, because you can break the the pattern this way and entirely bypass the validation (i.e. bypass \z).

@cmb69
Copy link
Member

cmb69 commented Oct 11, 2017

You can't properly escape # before preg_quote(), […]

Of course, you're right.

Anyhow, it might be worthwhile to discuss this issue on internals to reach a larger audience.

@krakjoe
Copy link
Member

krakjoe commented Oct 19, 2017

@Majkl578 please link back to internals discussion (via externals preferably) for future reference ...

@nikic
Copy link
Member

nikic commented Dec 16, 2017

Merged as 8423534 into master. Given that we have already added characters to preg_quote in the past and given the potential security vector, I think it's reasonable to introduce this in the next minor. If this breaks someones code, they have the next year to report it...

@nikic nikic closed this Dec 16, 2017
@Majkl578 Majkl578 deleted the bugfix/75355 branch December 16, 2017 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants