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

Ignore missing csp if page is not xss capable #3126

Merged
merged 19 commits into from
Jul 1, 2024
Merged

Conversation

noamblitz
Copy link
Contributor

Changes

We do not create missing CSP findings if the page is not XSS capable. Open for discussion:

For the other findings (misconfigured csp and disallowed domains in csp) we have two options:

  1. We do not create the headers in the header normalizer for non xss capable pages using the same check (simplest option).
  2. We rewrite the other bits to also gather the Content-Type header and do the same check there. It is the cleanest option but comes at the price of performance.

Issue link

#1950

Closes #1950

Demo

Please add some proof in the form of screenshots or screen recordings to show (off) new functionality, if there are interesting new features for end-users.

QA notes

Please add some information for QA on how to test the newly created code.


Code Checklist

  • All the commits in this PR are properly PGP-signed and verified.
  • This PR only contains functionality relevant to the issue.
  • I have written unit tests for the changes or fixes I made.
  • I have checked the documentation and made changes where necessary.
  • I have performed a self-review of my code and refactored it to the best of my abilities.
  • Tickets have been created for newly discovered issues.
  • For any non-trivial functionality, I have added integration and/or end-to-end tests.
  • I have informed others of any required .env changes files if required and changed the .env-dist accordingly.
  • I have included comments in the code to elaborate on what is not self-evident from the code itself, including references to issues and discussions online, or implicit behavior of an interface.

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@noamblitz noamblitz requested a review from a team as a code owner June 24, 2024 09:51
Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few tiny remark to improve it even more

octopoes/bits/missing_headers/missing_headers.py Outdated Show resolved Hide resolved
octopoes/bits/missing_headers/missing_headers.py Outdated Show resolved Hide resolved
@noamblitz
Copy link
Contributor Author

Lets decide whether we want to tackle the other csp finding in this pr or in another one, and lets also determine the best option!

@noamblitz
Copy link
Contributor Author

We went for the second solution, need to add to release notes that a recalculate bits is needed after upgrading

Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

No remarks

@stephanie0x00
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.
  • I checked the logs for errors and/or warnings and made issues where necessary

What works:

The following scenarios were tested:

  • URL with svg which would trigger the No-CSP finding.
  • URL with png which would not trigger the CSP finding.
  • HTML URL without any CSP headers which would trigger the NO-CSP finding.

The tested scenarios all give the expected output.

What doesn't work:

n/a

Bug or feature?:

n/a

  • _bull

@underdarknl underdarknl merged commit fdf0f47 into main Jul 1, 2024
19 of 20 checks passed
@underdarknl underdarknl deleted the ignore-missing-csp branch July 1, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dont check for CSP headers when the content of the URL is not XSS capable
4 participants