fixed uri regex issue #3815
fixed uri regex issue #3815
Conversation
pkg/detectors/uri/uri.go
Outdated
| @@ -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.
\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.
kashifkhan0771
force-pushed
the
fix/github-3686
branch
from
441760e to
546eb5c
Compare
pkg/detectors/uri/uri.go
Outdated
| @@ -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.
This is missing a large number of valid characters for usernames and passwords. The host pattern is also still fairly permissive and would match things that could never be valid, e.g. @----__-2as-2.
There was a problem hiding this comment.
How about something like:
\b(?:https?:\/\/)?[\w-\.$~!&'()*+,;=:%-]{3,50}:([\w-\.%$^#&'()*+,;=:%-]{3,50})@[a-zA-Z0-9.-]+(?:\.[a-zA-Z]{2,})?\bAdded additional valid characters and fixed the host pattern too.
There was a problem hiding this comment.
Example: https://regex101.com/r/qBF4nS/1
There was a problem hiding this comment.
That's looking a bit better. Some notes:
- The scheme prefix shouldn't be optional
:isn't a valid username character- Username isn't always required (example)
- The username and password patterns are both missing special characters. It would be pragmatic to add all applicable special characters from
[[:graph:]], and remove them later if they're causing issues. e.g.,
\bhttps?:\/\/[\w!#$%&()*+,\-./;<=>?@[\\\]^_{|}~]{0,50}:([\w!#$%&()*+,\-./:;<=>?[\\\]^_{|}~]{3,50})@[a-zA-Z0-9.-]+(?:\.[a-zA-Z]{2,})?\b- The pattern needs to be able to detect port as well as path
There was a problem hiding this comment.
Updated with port detection
kashifkhan0771
force-pushed
the
fix/github-3686
branch
from
0ef5d64 to
a085517
Compare
kashifkhan0771
force-pushed
the
fix/github-3686
branch
from
cb8895d to
a0919f5
Compare
pkg/detectors/uri/uri.go
Outdated
| @@ -30,7 +30,7 @@ var _ interface { | |||
| } = (*Scanner)(nil) | |||
|
|
|||
| var ( | |||
| keyPat = regexp.MustCompile(`\b(?:https?:)?\/\/[\S]{3,50}:([\S]{3,50})@[-.%\w\/:]+\b`) | |||
| keyPat = regexp.MustCompile(`\bhttps?:\/\/[\w!#$%&()*+,\-./;<=>?@[\\\]^_{|}~]{0,50}:([\w!#$%&()*+,\-./:;<=>?[\\\]^_{|}~]{3,50})@[a-zA-Z0-9.-]+(?:\.[a-zA-Z]{2,})?(?::\d{1,5})?\b`) | |||
There was a problem hiding this comment.
Hmm, this no longer matches paths after host/port. That's probably something worth keeping.
kashifkhan0771
force-pushed
the
fix/github-3686
branch
from
bf09c87 to
ec315d0
Compare
|
@kashifkhan0771 Is this ready to be merged? Wanted to double check before merging. Thanks. |
It's done from myside. @rgmz need to do a final review of the regex. |
Description:
This PR fixes github issue #3686
Checklist:
make test-community)?make lintthis requires golangci-lint)?