Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pscanrules: Address Suspicious Comments rule JS FPs #5813

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kingthorin
Copy link
Member

@kingthorin kingthorin commented Oct 13, 2024

Overview

  • CHANGELOG > Added fix note.
  • InformationDisclosureSuspiciousCommentsScanRule > Updated handling to target comments in JavaScript more specifically.
  • InformationDisclosureSuspiciousCommentsScanRuleUnitTest b> Updated and added tests.
  • Messages.properties > Updated to detail/report the findings more specifically based on the new behavior.
  • pscanrules.html > Correct occurrence of "add-on" (vs addon).

Note: The regexes used for JS comments are based on https://github.com/antlr/grammars-v4/blob/c82c128d980f4ce46fb3536f87b06b45b9619922/javascript/javascript/JavaScriptLexer.g4#L49-L50

Related Issues

Checklist

  • Update help
  • Update changelog
  • Run ./gradlew spotlessApply for code formatting
  • Write tests
  • Check code coverage
  • Sign-off commits
  • Squash commits
  • Use a descriptive title

@kingthorin kingthorin force-pushed the sus-comm-fp branch 2 times, most recently from 93a5cfd to 031dd13 Compare October 13, 2024 02:50
new AlertSummary(
pattern.toString(),
line,
Alert.CONFIDENCE_LOW,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be changing the confidence, we are still not extracting comments (e.g. "// FROM"), and IMO we should not be saying that we are checking comments, "likely comments".

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, that's fair

@kingthorin kingthorin force-pushed the sus-comm-fp branch 2 times, most recently from e40a6e1 to a1483ab Compare October 13, 2024 11:16
@kingthorin
Copy link
Member Author

Tweaked

@kingthorin kingthorin force-pushed the sus-comm-fp branch 2 times, most recently from 25d4dc4 to abcb851 Compare October 17, 2024 10:36
- CHANGELOG > Added fix note.
- InformationDisclosureSuspiciousCommentsScanRule > Updated handling to
target comments in JavaScript more specifically.
- InformationDisclosureSuspiciousCommentsScanRuleUnitTest b> Updated and
added tests.
- Messages.properties > Updated to detail/report the findings more
specifically based on the new behavior.
- pscanrules.html > Correct occurrence of "add-on" (vs addon).

Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>
@kingthorin
Copy link
Member Author

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants