Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamline validation of patternProperties Regex #653

Merged
merged 3 commits into from
Mar 18, 2021
Merged

Streamline validation of patternProperties Regex #653

merged 3 commits into from
Mar 18, 2021

Conversation

guilliamxavier
Copy link
Contributor

And always escape used delimiter in passed pattern.

Refs #650

`#` is escaped by preg_quote() in PHP >= 7.3, and `/` needs to be escaped currently (before #650), so let's go with `~`
@erayd erayd merged commit cd26b16 into jsonrainbow:master Mar 18, 2021
@erayd
Copy link
Contributor

erayd commented Mar 18, 2021

# is escaped by preg_quote() in PHP >= 7.3, and / needs to be escaped currently (before #650), so let's go with ~

Why does the choice of delimiter matter? I have no issue with using a different delimiter (so I have merged the PR regardless), but given that preg_quote() should never be run on the output of this, I don't understand why you've changed it.

@erayd erayd self-assigned this Mar 18, 2021
@guilliamxavier guilliamxavier deleted the patch-1 branch March 19, 2021 08:21
@guilliamxavier
Copy link
Contributor Author

guilliamxavier commented Mar 19, 2021

Why does the choice of delimiter matter? I have no issue with using a different delimiter (so I have merged the PR regardless), but given that preg_quote() should never be run on the output of this, I don't understand why you've changed it.

Not on the output indeed, but possibly on the input (part of):
I have a use case with dynamic patterns involving some fixed strings, e.g.

'^[0-9]~' . preg_quote($fixed, '/') . '~[0-9]$'

(note: the extra '/' arg to preg_quote() was necessary before #650, and is unneeded after but doesn't harm).
If $fixed = '+#/~%'; (containing all the possibilities of #194, showing that its approach wasn't exhaustive), then that gives (in PHP 7.3+) the raw string
^[0-9]~\+\#\/~%~[0-9]$ (which is OK). But then the original code of #650 would transform it into the PCRE
#^[0-9]~\+\\#\/~%~[0-9]$#u, which is invalid (the middle # is double-backslashed, i.e. not escaped anymore). By changing the delimiter to ~ (which is not escaped by preg_quote(), and has no reason to be escaped manually), it instead becomes
~^[0-9]\~\+\#\/\~%\~[0-9]$~u, which is valid (and correct). (Changing the delimiter to % would also work.)

Actually a really foolproof solution should only escape delimiter occurrences that are not already escaped (i.e. that are not preceded by an odd number of backslashes), but it would require changing the simple str_replace() to something much more complicated/heavy (using preg_replace() or even preg_replace_callback(), or some kind of loop with string operations)...

Anyway, thanks for merging 🙂 (I'm looking forward to the next 5.x backport 😉)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants