[HttpFoundation] JSONP callback validation #20127
Conversation
|
For now I'm 👎 because the replaced feature is minuscule and introducing a new dependency is not worth it (specially now, that Symfony is furiously removing dependencies ... even between its components). |
|
To be fair @javiereguiluz the dependency in question is fairly small as well, its scope is the scope of the feature. |
|
Understand.. however if im correct the philosophy is to not reinvent the wheel. Problem is we cant workaround symfony's out-of-the-box validation. |
|
@ro0NL you can workaround it by overriding it, it's a public method. You're PR is not changing anything in this regard. |
|
please note that this is introducing an optional dependency. |
|
@theofidry fair enough :) but still is cumbersome imo. |
|
We could also just copy the "right" regex.. i'd prefer the dependency though. |
|
IIUC, if you don't install the dependency, the callback is not validated anymore? Anyway, I don't see why we should depend on an external library for that. Is there any issue with our code? If yes, let's fix it. But as that's not something that changes over time, I don't see the advantage of depending on something else. |
Correct.. I dont mind forcing validation either, but if so, it should be correct. So basically there are 2 viable options;
Im not sure we should keep patching the regex whenever it shows being incorrect, ie. a stable/tested version is available => https://github.com/willdurand/JsonpCallbackValidator. |
|
Let's fix our regex. |
|
Shall we just take https://github.com/willdurand/JsonpCallbackValidator/blob/master/src/JsonpCallbackValidator.php#L11 then? ( |
|
You can take everything (and don't forget to include all the author/license information with it). |
|
👍 ill have a look tonight. |
|
@fabpot ready. I kept the unicode support as well.. so basically we only added support for bracket notation.. (taken from willdurand/JsonpCallbackValidator). |
| @@ -89,11 +89,16 @@ public static function fromJsonString($data = null, $status = 200, $headers = ar | |||
| public function setCallback($callback = null) | |||
| { | |||
| if (null !== $callback) { | |||
| // taken from http://www.geekality.net/2011/08/03/valid-javascript-identifier/ | |||
| $pattern = '/^[$_\p{L}][$_\p{L}\p{Mn}\p{Mc}\p{Nd}\p{Pc}\x{200C}\x{200D}]*+$/u'; | |||
There was a problem hiding this comment.
Removed the possessive + as this wont affect number of steps to match due ^$.
| // taken from http://www.geekality.net/2011/08/03/valid-javascript-identifier/ | ||
| $pattern = '/^[$_\p{L}][$_\p{L}\p{Mn}\p{Mc}\p{Nd}\p{Pc}\x{200C}\x{200D}]*+$/u'; | ||
| // taken from http://www.geekality.net/2011/08/03/valid-javascript-identifier/ and https://github.com/willdurand/JsonpCallbackValidator | ||
| $pattern = '/^[$_\p{L}][$_\p{L}\p{Mn}\p{Mc}\p{Nd}\p{Pc}\x{200C}\x{200D}]*(?:\[(?:"(?:\\\.|[^"\\\])*"|\'(?:\\\.|[^\'\\\])*\'|\d+)\])*?$/u'; |
There was a problem hiding this comment.
You need to add the copyright and the license
There was a problem hiding this comment.
Can you have a look.. not sure i did it correct.
|
👍 |
1 similar comment
|
👍 |
|
Thank you @ro0NL. |
This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #20127). Discussion ---------- [HttpFoundation] JSONP callback validation | Q | A | ------------- | --- | Branch? | "master" | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17923 | License | MIT | Doc PR | reference to the documentation PR, if any Maybe this is too small for a new dep, but at least it's stable. Symfony itself will make no assumption on validation by default, ie. things should keep working as usual. Commits ------- 1159f8b [HttpFoundation] JSONP callback validation
Maybe this is too small for a new dep, but at least it's stable. Symfony itself will make no assumption on validation by default, ie. things should keep working as usual.