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.

Sign up for GitHub

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

Jump to bottom

scan rules: Clean code tweaks #5541

Merged
merged 2 commits into from
Sep 17, 2024
Merged

scan rules: Clean code tweaks #5541

merged 2 commits into from
Sep 17, 2024

Conversation

kingthorin
Copy link
Member

@kingthorin kingthorin commented Jun 27, 2024 •
edited
Loading

Overview

scan rules: Clean code tweaks

  • Add static modifier where applicable.
  • CHANGELOG > Add maintenance note (if there wasn't already one present).
  • pscanrules > Made resource message methods private again where example alerts have been implemented, or removed them where there was only a single usage (inlining the Constant resource message usage).

ascanrulesAlpha: Add example alerts to example rules

  • CHANGELOG > Added change note.
  • Scan Rules > Added example alert handling, updated to conform to the common active scan rule tests.
  • Scan Rule Unit Tests > Added to assert the example alert and references, as well as common tests.

Checklist

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

ricekot
ricekot reviewed Jul 3, 2024
View reviewed changes
Copy link
Member

@ricekot ricekot left a comment

Choose a reason for hiding this comment

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

Have only gone through the first few files for now. Will gradually look through the others :).

.../ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java Outdated Show resolved Hide resolved
...ns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/BufferOverflowScanRule.java Outdated Show resolved Hide resolved
addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/FormatStringScanRule.java Outdated Show resolved Hide resolved
@kingthorin kingthorin force-pushed the rules-clean branch 5 times, most recently from 18cb8bf to 6992ece Compare July 4, 2024 17:59
@kingthorin
Copy link
Member Author

Addressed those comments and rebased current (to fix the conflict).

@kingthorin
Copy link
Member Author

Spoke about this on slack. Will reduce this one to just address static modifiers. Then tackle start/stop logging, and javadoc/comments separately.

@kingthorin kingthorin changed the title scan rules: Clean code tweaks scan rules: Clean code tweaks - WIP Jul 16, 2024
@kingthorin kingthorin force-pushed the rules-clean branch 3 times, most recently from e72546c to ac1cd1f Compare July 17, 2024 16:27
@kingthorin kingthorin changed the title scan rules: Clean code tweaks - WIP scan rules: Clean code tweaks Jul 17, 2024
@kingthorin
Copy link
Member Author

Rebased current

psiinon
psiinon requested changes Aug 20, 2024
View reviewed changes
Copy link
Member

@psiinon psiinon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm fine adding the statics. But removing the get* methods means the script which generates the alert pages will not be able to extract the info.
Thats fine if the rules have example alerts, but the ones I looked at did not :(

@kingthorin kingthorin force-pushed the rules-clean branch 3 times, most recently from 97eff58 to 114347c Compare August 20, 2024 16:51
@kingthorin
Copy link
Member Author

Adjusted.

@kingthorin
Copy link
Member Author

Rebased current

@kingthorin
Copy link
Member Author

Is this waiting on me to add example alerts to the example rules?

@kingthorin
Copy link
Member Author

ascanrulesAlpha now also includes example alerts on example rules. pscanrulesAlpha were previously done.

@kingthorin kingthorin force-pushed the rules-clean branch 2 times, most recently from db71a9c to 84720db Compare September 9, 2024 15:10
@kingthorin kingthorin force-pushed the rules-clean branch 8 times, most recently from cb67b5d to ac0cded Compare September 10, 2024 14:20
@kingthorin kingthorin force-pushed the rules-clean branch 5 times, most recently from 4a7e682 to d1cf763 Compare September 16, 2024 21:27
@kingthorin
Copy link
Member Author

Think I've addressed all those,

@kingthorin
scan rules: Clean code tweaks
28f1112 
- Add static modifier where applicable.
- CHANGELOG > Add maintenance note (if there wasn't already one
present).
- pscanrules > Made resource message methods private again where example
alerts have been implemented, or removed them where there was only a
single usage (inlining the Constant resource message usage).
@kingthorin
ascanrulesAlpha: Add example alerts to example rules
141375b 
- CHANGELOG > Added change note.
- Scan Rules > Added example alert handling, updated to conform to the
common active scan rule tests.
- Scan Rule Unit Tests > Added to assert the example alert and
references, as well as common tests.

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

Okay I think I got those three, hopefully this is done

@thc202
Copy link
Member

thc202 commented Sep 17, 2024

Thank you!

@psiinon psiinon merged commit dbc199b into zaproxy:main Sep 17, 2024
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2024
@kingthorin kingthorin deleted the rules-clean branch September 17, 2024 13:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ricekot ricekot ricekot left review comments

@psiinon psiinon psiinon approved these changes

@thc202 thc202 thc202 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kingthorin @thc202 @psiinon @ricekot