Fix regex logic for SecretsUsedInArgOrEnv to match comment and test #6089
Conversation
Signed-off-by: Curt Marker <[email protected]>
markercm
force-pushed
the
fix-SecretsUsedInArgOrEnv-logic-to-match-comment-and-test
branch
from
ccae356 to
dcda7e3
Compare
There was a problem hiding this comment.
I'm not sure what is the difference between matching PASSWORD_ROOT but not matching DB_PASSWORD_ROOT. If this is causing too many false positives (not sure if the case) then matching last component or full text could be more limited set. @colinhemmings
A more general fix would be to add the skip comments per-line, that are currently only supported per-file.
| key string | ||
| isSecret bool | ||
| }{ | ||
| // Positive matches |
There was a problem hiding this comment.
None of these tests seem to actually check for the change of behavior (handling secrets in the middle of string).
There was a problem hiding this comment.
{name: "contains key but not fitst of last or full", key: "new_key_file_path", isSecret: false},This matches on the old behavior even though key is neither the first nor last nor full match but doesn't with the new logic.
I would agree with you and would love to see that get prioritized as it seems like the only real option. |
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| tc := tc |
There was a problem hiding this comment.
Check the linter error caused by this line
Signed-off-by: Curt Marker <[email protected]>
I've added the regex pattern that @Nuru suggested in #5775 (comment) and added tests for the function to ensure that it actually works.
Copilot created the tests.
Inspired by a comment in but doesn't actually address #5775