Fix false positives detection in LarkSuite token regex #4194
Conversation
|
|
|
@annetteshajan Thank you for contributing. Please sign the CLA first so that we can move forward with the review. |
| @@ -34,9 +34,9 @@ const ( | |||
| var ( | |||
| defaultClient = common.SaneHttpClient() | |||
| tokenPats = map[tokenType]*regexp.Regexp{ | |||
| TenantAccessToken: regexp.MustCompile(detectors.PrefixRegex([]string{"lark", "larksuite", "tenant"}) + `\b(t-[a-z0-9A-Z_.]{14,50})\b`), | |||
There was a problem hiding this comment.
Hey @annetteshajan! Thanks for updating this. I have a small suggestion. We could remove the \b word boundaries and use negative lookbehind and lookahead instead in order to avoid potential backtracking and improve regex efficiency.
Something like:
(?<![-\w])(t-[a-zA-Z0-9_.]{14,50})(?![-\w])
There was a problem hiding this comment.
I had tried this initially, but I found that Go doesn't support lookahead and lookbehind with the standard regex libraries. I will have to think of another workaround
There was a problem hiding this comment.
@amanfcp Hi, it is done!
There was a problem hiding this comment.
@annetteshajan Ah, thanks for clarifying. In that case the current regex works.
pkg/detectors/larksuite/larksuite.go
Outdated
| TenantAccessToken: regexp.MustCompile(detectors.PrefixRegex([]string{"lark", "larksuite", "tenant"}) + `(?:^|[^-])\b(t-[a-z0-9A-Z_.]{14,50})\b(?:^|[^-])`), | ||
| UserAccessToken: regexp.MustCompile(detectors.PrefixRegex([]string{"lark", "larksuite", "user"}) + `(?:^|[^-])\b(u-[a-z0-9A-Z_.]{14,50})\b(?:^|[^-])`), | ||
| AppAccessToken: regexp.MustCompile(detectors.PrefixRegex([]string{"lark", "larksuite", "app"}) + `(?:^|[^-])\b(a-[a-z0-9A-Z_.]{14,50})\b(?:^|[^-])`), |
There was a problem hiding this comment.
The current regex won't match valid patterns at the end of input because the final (?:^|[^-]) requires either start-of-string (impossible at end of match) or a non-hyphen character after the pattern.
Current:
(?:^|[^-])\b(a-[a-z0-9A-Z_.]{14,50})\b(?:^|[^-])
Should be:
(?:^|[^-])\b(a-[a-z0-9A-Z_.]{14,50})\b(?:[^-]|$)
Issue: String like text a-example123456789 (ending with target pattern) won't match with current regex.
Fix: Replace final (?:^|[^-]) with (?:[^-]|$) to handle end-of-string cases.
There was a problem hiding this comment.
Hi @amanfcp
Thanks for the comments!
I have made the changes
annetteshajan
force-pushed
the
larksuite-regex
branch
from
1954fc4 to
54e25ac
Compare
annetteshajan
force-pushed
the
larksuite-regex
branch
from
54e25ac to
2aa30cb
Compare
|
@nabeelalam Can you review it again please? |
Description:
This commit fixes the issue mentioned in #4101
It cleans up the regex used for checking access tokens in LarkSuite.
Currently "\b" only covers non word characters (letters, numbers, underscore). It does not include dashes which is popularly used in URLs and other naming conventions.
I have also included a test which checks for invalid tokens.
Checklist:
make test-community)?make lintthis requires golangci-lint)?