-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
fixed uri regex issue #3815
base: main
Are you sure you want to change the base?
fixed uri regex issue #3815
Conversation
@@ -23,7 +23,7 @@ var _ detectors.Detector = (*Scanner)(nil) | |||
var _ detectors.CustomFalsePositiveChecker = (*Scanner)(nil) | |||
|
|||
var ( | |||
keyPat = regexp.MustCompile(`\b(?:https?:)?\/\/[\S]{3,50}:([\S]{3,50})@[-.%\w\/:]+\b`) | |||
keyPat = regexp.MustCompile(`\b(?:https?:)?\/\/[\w-\.]{3,50}:([\w-\.]{3,50})@[-.%\w\/:]+\b`) |
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.
\S
matches any non-whitespace character, which is very broad. Instead, we are now using \w
, which matches [A-Za-z0-9_]
, and extending it by adding a few special characters to suit our needs.
@@ -23,7 +23,7 @@ var _ detectors.Detector = (*Scanner)(nil) | |||
var _ detectors.CustomFalsePositiveChecker = (*Scanner)(nil) | |||
|
|||
var ( | |||
keyPat = regexp.MustCompile(`\b(?:https?:)?\/\/[\S]{3,50}:([\S]{3,50})@[-.%\w\/:]+\b`) | |||
keyPat = regexp.MustCompile(`\b(?:https?:)?\/\/[\w-\.]{3,50}:([\w-\.]{3,50})@[-.%\w\/:]+\b`) |
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.
In my opinion, passwords in URIs can consist of any non-whitespace characters. Using \w limits the match to a specific set of characters, which might exclude valid ones. Are you noticing any detection issues when using \S 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.
e.g. http://username:p%40ssword@127.0.0.1" Will not be detected when using \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.
Yes, actually as the scenario mentioned in the linked issue, it matches some special chars like "
which it should not I guess. I can add more special characters like %$^...
in the set to expand the functionality.
Description:
This PR fixes github issue #3686
Checklist:
make test-community
)?make lint
this requires golangci-lint)?