Skip to content

Conversation

@kingthorin
Copy link
Member

@kingthorin kingthorin commented May 21, 2025

Overview

  • CHANGELOG > Add re-ordering note.
  • ExternalRedirectScanRule > Use an enum for payloads & types. Move some logic to be within the payloads enum for simplicity. Move alert reference logic within the types enum. Move payload counts per Strength logic to init method and use a simplified case structure. Remove unnecessary comments.
  • ExternalRedirectScanRuleUnitTest > Remove unnecessary assignments. Use isEmpty vs length greater than zero.

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

@psiinon
Copy link
Member

psiinon commented May 21, 2025

Logo
Checkmarx One – Scan Summary & Details21de6ed5-ff3e-418e-89a8-7946be6cf307

Great job! No new security vulnerabilities introduced in this pull request

@kingthorin
Copy link
Member Author

kingthorin commented May 21, 2025

Is there any good reason for the type ints to be instantiated in the 0x## format?

L85-92

@thc202 thc202 changed the title ascanrules: Tiddy up ExternalRedirectScanRule ascanrules: Tidy up ExternalRedirectScanRule May 26, 2025
@thc202
Copy link
Member

thc202 commented May 26, 2025

#6458 (comment)

No, and it would be better to use an enum to avoid duplication like in getExampleAlerts().

@kingthorin kingthorin force-pushed the ext-redir-enum branch 4 times, most recently from 743e1f4 to 71fc43e Compare May 26, 2025 16:43
@kingthorin
Copy link
Member Author

Done (I think)

@thc202
Copy link
Member

thc202 commented May 28, 2025

The point of changing to enum was to also get rid of getAlertReference() and getRedirectionReason().

@kingthorin kingthorin force-pushed the ext-redir-enum branch 3 times, most recently from 7268c09 to 7fe4a10 Compare May 28, 2025 14:32
@kingthorin
Copy link
Member Author

Got all those.

@kingthorin kingthorin force-pushed the ext-redir-enum branch 2 times, most recently from 9eee5d9 to 8949b5f Compare June 20, 2025 11:24
@kingthorin kingthorin requested a review from Copilot June 27, 2025 10:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR tidies up the ExternalRedirectScanRule functionality by refactoring the payload and redirect type handling into enums and cleaning up test code.

  • Replace integer constants with enums for redirect types and payloads.
  • Refactor payload injection and payload count logic with switch expressions.
  • Update unit tests to use .isEmpty() instead of length checks and remove redundant assignments.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRuleUnitTest.java Refactored empty string checks and cleaned up some redundant assignments.
addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java Replaced integer-based redirect types with enums and refactored payload injection and alert-building logic.
addOns/ascanrules/CHANGELOG.md Added a maintenance note reflecting the code improvements.

NEITHER,
ALLOW_LIST,
CONCAT_PARAM,
CONCAT_PATH

This comment was marked as resolved.

@kingthorin kingthorin force-pushed the ext-redir-enum branch 4 times, most recently from d9e6907 to affee0e Compare July 28, 2025 16:47
@kingthorin kingthorin force-pushed the ext-redir-enum branch 2 times, most recently from 7853896 to 5a1d500 Compare July 28, 2025 18:01
@kingthorin kingthorin force-pushed the ext-redir-enum branch 2 times, most recently from d1e42de to ca2ff06 Compare July 28, 2025 20:02
- SQL Injection - PostgreSQL
- The Remote OS Command Injection scan rule has been broken into two rules; one feedback based, and one time based (Issue 7341). This includes assigning the time based rule ID 90037.
- For Alerts raised by the SQL Injection scan rules the Attack field values are now simply the payload, not an assembled description.
- Maintenance changes.
Copy link
Member Author

Choose a reason for hiding this comment

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

It's at the top of the list.

@kingthorin kingthorin changed the title ascanrules: Tidy up ExternalRedirectScanRule ascanrules: Tidy up External Redirect Scan Rule Jul 28, 2025
- CHANGELOG > Add re-ordering note.
- ExternalRedirectScanRule > Use an enum for payloads & types. Move some
logic to be within the payloads enum for simplicity. Move alert
reference logic within the types enum. Move payload counts
per Strength logic to init method and use a simplified case structure.
Remove unnecessary comments.
- ExternalRedirectScanRuleUnitTest > Remove unnecessary assignments. Use
isEmpty vs length greater than zero.

Signed-off-by: kingthorin <[email protected]>
@thc202
Copy link
Member

thc202 commented Jul 29, 2025

Thank you!

@psiinon psiinon merged commit 864d479 into zaproxy:main Jul 29, 2025
9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 29, 2025
@kingthorin kingthorin deleted the ext-redir-enum branch July 29, 2025 09:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants