Skip to content

fix(query): fix fp in password and secrets Generic Token#7521

Closed
cx-andre-pereira wants to merge 6 commits into
Checkmarx:masterfrom
cx-andre-pereira:FP_for_Passwords_And_Secrets-Generic_token
Closed

fix(query): fix fp in password and secrets Generic Token#7521
cx-andre-pereira wants to merge 6 commits into
Checkmarx:masterfrom
cx-andre-pereira:FP_for_Passwords_And_Secrets-Generic_token

Conversation

@cx-andre-pereira

Copy link
Copy Markdown
Contributor

Reason for Proposed Changes

  • Currently the implementation to prevent flagging of github id-token(s) such as : 'id-token: write' is not detecting relevant instances reliably.
  • The regex used at the moment ((?i)['\"]?id-token\\s*[:=]\\s*(write|read|none)\\s*$) is faulty and inconsistent relative to the rest of the regex rules for passwords_and_secrets's regex_rules.
  • This regex stands out for its requirement to scan a full line to detect the id-token ($ => "end of string"), this way something like a trailing comment will have to be explicitly captured by the regex or a text like : permissions: {id-token: write, contents: ...} will also require the capture of unnecessary text.

Proposed Changes

  • To better standardize regex logic i propose the implementation of an updated regex:
    • (?i)['\"]?id-token['\"]?\\s*[:=]\\s*(write|read|none)
  • Aside from the obvious removal of the undesirable \\s*$, i added an extra check ['\"]? after the "id-token" key-word to better align the regex composition logic with most other regex rules.
  • With this implementation detection of the "id-token" is much more reliable and independent of trailing comments or any data structure it might find itself within.
  • Finally the 2 new tests positive53 and negative53 verify, respectively, that the id-token is detected when there is a trailing comment and that it is flagged with an invalid command.

NOTE: the regex are written with double backslashes to represent the exact characters used in the passwords_and_secrets's regex_rules file strings for they are literal strings, not raw.

I submit this contribution under the Apache-2.0 license.

@cx-andre-pereira cx-andre-pereira requested a review from a team as a code owner July 7, 2025 17:09
@github-actions github-actions Bot added community Community contribution query New query feature labels Jul 7, 2025
@cx-andre-pereira cx-andre-pereira changed the title fix(query):fix fp for s3_bucket_access_to_any_principal fix(query): fix fp in password and secrets Generic Token Jul 8, 2025

@cx-artur-ribeiro cx-artur-ribeiro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good to me, we could improve the actual positive results with valid cases, without using comments at the start of the line since the problem is actually with having comments after id-token: write (for example).
I'll leave to another reviewer the need to update this test cases or not.
Nice work eitherway!

@cx-andre-pereira cx-andre-pereira deleted the FP_for_Passwords_And_Secrets-Generic_token branch July 16, 2025 10:40
@cx-andre-pereira cx-andre-pereira restored the FP_for_Passwords_And_Secrets-Generic_token branch July 16, 2025 10:51
@cx-andre-pereira cx-andre-pereira deleted the FP_for_Passwords_And_Secrets-Generic_token branch July 16, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community Community contribution query New query feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants