[Validator] fix URL validator to detect non supported chars according to RFC 3986 #22022
Conversation
|
Awesome. Thank you for your contribution. Your code will definitely improve Symfony code base by fixing potential security error and many projects will benefit of that. I made some tests. This (non-valid) URL isn't validated (which is great) I also tested this other URL that is considered as valid but is not: |
|
@Nek- , thanks for your review! I just extended test cases with your example ( |
|
@e-moe it's valid as DNS but not as query string (where is must be url-encoded). But well, it's fine to me, I'm just noticing it. |
|
oh, I see your point. If needed it can be fixed. Let's wait for other reviews and then we will be able to make a decision |
| const PATTERN = '~^ | ||
| (%s):// # protocol | ||
| const PATTERN = '<^ | ||
| (%protocol%):// # protocol |
There was a problem hiding this comment.
why these two changes? better not do them imho, and continue using sprintf (requires doubling the % below)
|
@nicolas-grekas , I just fixed code based on your comments. should I create new PR for 2.7 branch? |
|
as for BC break - new constraint is more strict comparing to old version. but new one is closer to RFC should it be considered as a BC break? |
|
It's not a BC break! It's a fix. |
|
Thank you @e-moe. |
…s according to RFC 3986 (e-moe) This PR was submitted for the 3.2 branch but it was merged into the 2.7 branch instead (closes #22022). Discussion ---------- [Validator] fix URL validator to detect non supported chars according to RFC 3986 | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21961 | License | MIT | Doc PR | none Commits ------- 3599c47 [Validator] fix URL validator to detect non supported chars according to RFC 3986
|
I have a template link field with Url validator. By template I mean |
|
@Aliance yes. I'm sorry to read it breaks your code but this is the attempted behavior of the URL Validator. (here was a potential security issue that is now fixed) |
Uh oh!
There was an error while loading. Please reload this page.