-
Notifications
You must be signed in to change notification settings - Fork 41.5k
A URI with non-alpha characters in its scheme is not sanitized #27482
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
Conversation
As described in RFC2396 (https://datatracker.ietf.org/doc/html/rfc2396#section-3.1)
@billyto Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@billyto Thank you for signing the Contributor License Agreement! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for the pull request, @billyto. I left one comment regarding the updated regex. Could you please take a look when you have a moment?
@@ -50,7 +50,7 @@ | |||
private static final Set<String> URI_USERINFO_KEYS = new LinkedHashSet<>( | |||
Arrays.asList("uri", "uris", "url", "urls", "address", "addresses")); | |||
|
|||
private static final Pattern URI_USERINFO_PATTERN = Pattern.compile("\\[?[A-Za-z]+://.+:(.*)@.+$"); | |||
private static final Pattern URI_USERINFO_PATTERN = Pattern.compile("^[A-Za-z][\\w\\+\\.\\-]+://.+:(.*)@.+$"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\w
includes _
which I don't think is correct here. Should it be A-Za-z0-9
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are totally correct, good catch! Let me fix it in a couple of hours when getting back home.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
as \w includes underscore character "_"
Thanks very much for making your first contribution to Spring Boot, @billyto. |
The Sanitizer.java class infers that the scheme portion of a URI can only contain letters, but as described in RFC2396 it can follow:
scheme = alpha *( alpha | digit | "+" | "-" | "." )
We have a use case where a valid URI with the scheme
mongodb+sr
, won't get the password token sanitized due to the current regex pattern.