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: Update CSRF countermeasures rule to consider only html files #4833

Merged
merged 4 commits into from
Sep 8, 2023

Conversation

gustavocovas
Copy link
Contributor

Overview

Updated the passive scan CRSF countermeasures rule to consider only HTML files, as discussed in zaproxy/zaproxy#7890

I know that it is currently assigned for @FiveOFive, but I saw this as an opportunity for my first contribution (hope that it is ok)

Related Issues

zaproxy/zaproxy#7890

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

For more details, please refer to the developer rules and guidelines.

@thc202 thc202 changed the title Update pscan CSRF countermeasures rule to consider only html files pscanrules: Update CSRF countermeasures rule to consider only html files Aug 18, 2023
@thc202
Copy link
Member

thc202 commented Aug 18, 2023

There should be a test showing that non-HTML does not raise an alert. The help should be updated to mention that non-HTML content will not be checked.

It's better to ask first in the issue when it's already assigned to avoid duplicating efforts.

@kingthorin
Copy link
Member

kingthorin commented Aug 18, 2023

I'm not sure this is the right approach because csrf on image, pdf, csv, Excel, etc is a potential DoS issue. I'd rather it simply exclude js and css.

@gustavocovas
Copy link
Contributor Author

I'm not sure this is the right approach because csrf on image, pdf, csv, Excel, etc is a potential DoS issue. I'd rather it simply exclude js and css.

My understanding is that the rule is specific for HTML forms, so the concern here would be about embedded HTML forms in those types of responses? If so, should we make the same change to the active CSRF rule?

@kingthorin
Copy link
Member

You're right. I was thinking "response" but it's the response to the POST (or whatever) that I'm concerned about not the actual response that contains the form in question. So yes you're correct. Sorry.

@gustavocovas
Copy link
Contributor Author

No worries. And thanks to both of you for the quick reviews and responses. What an awesome project and team 😄
I will now work on the changes requested

@kingthorin
Copy link
Member

@gustavocovas
Copy link
Contributor Author

Folks, I believe I addressed all the comments. Should I squash the commits?

@thc202 thc202 linked an issue Sep 1, 2023 that may be closed by this pull request
1 task
@thc202
Copy link
Member

thc202 commented Sep 8, 2023

This needs to be rebased and the changelog entry added to the latest Unreleased section.

@kingthorin
Copy link
Member

@thc202 thc202 enabled auto-merge (squash) September 8, 2023 19:01
@thc202
Copy link
Member

thc202 commented Sep 8, 2023

Thank you!

@thc202 thc202 merged commit 4c01b90 into zaproxy:main Sep 8, 2023
10 checks passed
@kingthorin
Copy link
Member

Thanks!

@thc202
Copy link
Member

thc202 commented Sep 9, 2023

@gustavocovas how would you like to be credited (e.g. name, handle)?
https://www.zaproxy.org/docs/desktop/credits/#zap-extended-team

@gustavocovas
Copy link
Contributor Author

It can be "Gustavo Covas (@gustavocovas)"
Thanks

@gustavocovas gustavocovas deleted the pscan-csrf-html-only branch September 9, 2023 14:29
@kingthorin
Copy link
Member

Thanks, I'll tackle that Monday morning.

kingthorin added a commit to kingthorin/zap-core-help that referenced this pull request Sep 12, 2023
kingthorin added a commit to kingthorin/zap-core-help that referenced this pull request Sep 12, 2023
kingthorin added a commit to kingthorin/zap-core-help that referenced this pull request Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

CSRF Reported on JavaScript page
3 participants