Skip to content

pscanrules: Update CSPMissing for current guidance (Issue 7653) #4338

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
Merged
thc202 merged 11 commits into from
Feb 23, 2023
Merged

pscanrules: Update CSPMissing for current guidance (Issue 7653) #4338

thc202 merged 11 commits into from
Feb 23, 2023

Conversation

@davidahall
Copy link
Contributor

@davidahall davidahall commented Jan 3, 2023 •
edited by thc202
Loading

The alerts raised by the Content Security Policy Missing scan rule do not reflect the current guidance in the linked documents. The current guidance is that the X-Content-Security-Policy and X-WebKit-CSP headers are obsolete, and should not be used. However, at Low threshold, absence of these headers results in an alert being raised. I've updated the logic to more accurately reflect the OWASP guidance.

Fix zaproxy/zaproxy#7653.

@davidahall davidahall mentioned this pull request Jan 3, 2023
Closed
@thc202 thc202 changed the title Update CSPMissing for current guidance (Issue 7653) pscanrules: Update CSPMissing for current guidance (Issue 7653) Jan 3, 2023
kingthorin
kingthorin reviewed Jan 3, 2023
View reviewed changes
Copy link
Member

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

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

I still need to look at the tests but here’s a quick first pass.

@kingthorin
Copy link
Member

kingthorin commented Jan 3, 2023

To address the DCO requirement you'll need to sign-off the commit(s):

@davidahall davidahall force-pushed the main branch 2 times, most recently from d5dfffa to 9f66f4a Compare January 3, 2023 19:41
thc202
thc202 reviewed Jan 4, 2023
View reviewed changes
@thc202
Copy link
Member

thc202 commented Jan 4, 2023

The branch needs to be updated and the changelog moved to the Unreleased version.

kingthorin
kingthorin reviewed Jan 8, 2023
View reviewed changes
@davidahall
Copy link
Contributor Author

davidahall commented Jan 12, 2023

Couple of process questions, because I'm new on this project.

  • Should one or more of the workflows need to be run?
  • Am I missing a task to be completed?

@kingthorin
Copy link
Member

kingthorin commented Jan 12, 2023

All the workflows completed successfully. The only remaining step is for a second approving review :) So for now it's kinda on us.

thc202
thc202 reviewed Jan 14, 2023
View reviewed changes
kingthorin
kingthorin approved these changes Jan 19, 2023
View reviewed changes
Copy link
Member

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

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

Thanks

@psiinon
Copy link
Member

psiinon commented Feb 2, 2023 •
edited by kingthorin
Loading

The scan rule doesnt return any examples.
That was an existing restriction.
However now that it returns 2 different alerts I think its more significant.
I'd like it to support the examples :)
Hopefully this wont be much work - we have lots of examples.
And that will then improve this page: https://www.zaproxy.org/docs/alerts/10038/

Edit: Reference 👉 zaproxy/zaproxy#6119

kingthorin
kingthorin reviewed Feb 8, 2023
View reviewed changes
@thc202
Copy link
Member

thc202 commented Feb 8, 2023

Needs to be rebased to properly update the changelog.

@kingthorin
Copy link
Member

kingthorin commented Feb 10, 2023

You seem to have merged, not rebased.

@davidahall
Copy link
Contributor Author

davidahall commented Feb 12, 2023

You seem to have merged, not rebased.

apparently so; I'll figure out how to get this reset and resubmit.

@thc202
Copy link
Member

thc202 commented Feb 13, 2023

We are happy to help.

@davidahall davidahall force-pushed the main branch 2 times, most recently from 305c619 to 0530c81 Compare February 14, 2023 23:42
David Hall added 6 commits February 14, 2023 18:46
pscanrules: Update CSPMissing for current guidance
7dba7c9 
Implementations that followed current guidance with respect to which
headers to use would see alerts raised at Low threshold.  Now, the
alerts reflect the current guidance (ie, don't use the obsolete CSP
headers).

Signed-off-by: David Hall <[email protected]>
pscanrules: Removed needless and incorrect comment
a8ea235 
Signed-off-by: David Hall <[email protected]>
pscanrules: Addressing review comment and failing test
d42f0e6 
Signed-off-by: David Hall <[email protected]>
pscanrules: Removed duplicate sentence from docs.
c5c62b9 
Signed-off-by: David Hall <[email protected]>
pscanrules: Obsolete CSP alert is more distinct
55952dd 
- Obsolete CSP header usage is always flagged, not just at LOW
- General refactoring suggested by reviewers

Signed-off-by: David Hall <[email protected]>
pscanrules: Improved text in changelog, html
0c47791 
Signed-off-by: David Hall <[email protected]>
@davidahall
Copy link
Contributor Author

davidahall commented Feb 14, 2023

in my day job, we don't really use forks, so that is new to me. I've found that the best way to learn more about git is to git yourself outta trouble. Hopefully, I didn't hammer it up too badly -- I do see your recent commits in the history on my fork, so there is some reassurance.

@thc202 thc202 merged commit 9ac7acc into zaproxy:main Feb 23, 2023
@thc202
Copy link
Member

thc202 commented Feb 23, 2023

Thank you!

@davidahall
Copy link
Contributor Author

davidahall commented Feb 24, 2023

Thank you both; this was an interesting experience

@thc202 thc202 mentioned this pull request Mar 10, 2023
Open
3 tasks
@thc202
Copy link
Member

thc202 commented Mar 10, 2023

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

@davidahall
Copy link
Contributor Author

davidahall commented Mar 12, 2023 via email

@kingthorin kingthorin mentioned this pull request Mar 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thc202 thc202 thc202 approved these changes

@kingthorin kingthorin kingthorin approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Possible false positive in CSP Header test

4 participants

@davidahall @kingthorin @thc202 @psiinon