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

ascanrulesBeta: Address possible FP in proxy detection rule #5718

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

Conversation

kingthorin
Copy link
Member

@kingthorin kingthorin commented Sep 9, 2024

Overview

  • CHANGELOG > Added change note.
  • ProxyDisclosureScanRule > Added condition to skip messages if they have evidence content to start with. Removed misleading Attack text from the Alert.
  • ProxyDisclosureScanRuleUnitTest > Added a test to assert the new behavior.

Related Issues

Fixes zaproxy/zaproxy#8556

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 prxy-xfwd branch 2 times, most recently from e0c29af to 44b000e Compare September 9, 2024 11:34
@kingthorin
Copy link
Member Author

Tweaked

addOns/ascanrulesBeta/CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 75 to 76
"X-Forwarded-For: 76.69.54.171", "X-Forwarded-For: 127.0.0.1",
"X-Forwarded-Host: api.test.glaypen.garnercorp.com", "X-Forwarded-Port: 443",
Copy link
Member

@thc202 thc202 Sep 9, 2024

Choose a reason for hiding this comment

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

Better remove public IPs and domains, there are also headers being tested that wouldn't raise an alert anyway. It's missing Via (for completeness). Also, would be good that the server behaviour did raise an alert if it wasn't for the evidence being already present.

Copy link
Member

Choose a reason for hiding this comment

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

The test is still not testing the expected code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I glossed over your second point about the served content. Will adjust.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do plan to work through this, but it's gonna take a bit longer to get the nano handler setup for all the pre-checks.

@thc202 thc202 mentioned this pull request Sep 9, 2024
5 tasks
@kingthorin kingthorin force-pushed the prxy-xfwd branch 2 times, most recently from 824b5df to f62d8fa Compare September 10, 2024 00:44
@kingthorin
Copy link
Member Author

Got all those I think.

@kingthorin kingthorin added the waiting-for:pr-author This PR is currently waiting for input or changes from the original submitter label Sep 11, 2024
- CHANGELOG > Added change note.
- ProxyDisclosureScanRule > Added condition to skip messages if they
have evidence content to start with. Removed misleading Attack
text from the Alert.
- ProxyDisclosureScanRuleUnitTest > Added a test to assert the new
behavior.

Signed-off-by: kingthorin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog waiting-for:pr-author This PR is currently waiting for input or changes from the original submitter
Development

Successfully merging this pull request may close these issues.

ProxyDisclosureScanRule reporting is confusing for cases where servers freely offer X-Forwarded-* text
2 participants