8000 [Validator] Allow basic auth in url when using UrlValidator. by blaugueux · Pull Request #11601 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Validator] Allow basic auth in url when using UrlValidator. #11601

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

Closed
wants to merge 1 commit into from
Closed

[Validator] Allow basic auth in url when using UrlValidator. #11601

wants to merge 1 commit into from

Conversation

blaugueux
Copy link
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Now an url with basic auth like http://username:password@symfony.com can be valid.

@@ -24,6 +24,7 @@ class UrlValidator extends ConstraintValidator
{
const PATTERN = '~^
(%s):// # protocol
((.+?):(.+?)@)? # basic auth
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.+? is equivalent to .*

and anyway, this pattern is wrong . is too permissive here. For instance, you cannot have a @ in there (you have to urlencode it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof Is this better ([\pL\pN-]*):([\pL\pN-]*)@ ?

I'm not an expert in regex, but i need this to be fixed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be (([\pL\pN-]+:)?([\pL\pN-]+)@)? then, as the first part is optional and at least the second should exists with the @ sign.

OK: user:pass@, user@ and empty.
WRONG: user:pass, @, :pass@, :pass

Hmm seems regex is failing need to take a look at this.

Ah never mind I forgot to click for the test 😳

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx @sstok for your help

Improve regex. Add tests.
@blaugueux
Copy link
Contributor Author

@stof do i need to fix something else ?

@stof
Copy link
Member
stof commented Aug 8, 2014

👍

@fabpot
Copy link
Member
fabpot commented Aug 9, 2014

👍

@fabpot
Copy link
Member
fabpot commented Aug 9, 2014

Thank you @blaugueux.

fabpot added a commit that referenced this pull request Aug 9, 2014
…r. (blaugueux)

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #11601).

Discussion
----------

[Validator] Allow basic auth in url when using UrlValidator.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |
Now an url with basic auth like ```http://username:password@symfony.com``` can be valid.

Commits
-------

f1ea987 Allow basic auth in url. Improve regex. Add tests.
@fabpot fabpot closed this Aug 9, 2014
@blaugueux blaugueux deleted the url-validator-basic-auth branch August 9, 2014 10:41
@blaugueux
Copy link
Contributor Author

Huge thanks for all your work @fabpot

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
0