Fix regex logic for SecretsUsedInArgOrEnv to match comment and test #6089
Conversation
Signed-off-by: Curt Marker <curt@getharvest.com>
markercm
force-pushed
the
fix-SecretsUsedInArgOrEnv-logic-to-match-comment-and-test
branch
from
ccae356 to
dcda7e3
Compare
tonistiigi
left a comment
tonistiigi
left a comment
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. |
Signed-off-by: Curt Marker <curt@getharvest.com>
2 participants
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