Skip to content

fix(csp): allow duplicate report-* directives #151

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

Merged
merged 3 commits into from
Jan 3, 2025

Conversation

argl
Copy link
Contributor

@argl argl commented Jan 2, 2025

Description

If a CSP entry contains duplicate report-to or report-uri entries, the entry is still considered valid, similar to browser behaviour. Only the first encountered value is taken into account.

(MP-1799)

Motivation

CSP entries were flagged as invalid because of duplicate report-uri values. After discussion with the security team, report-* directives were deemed ok to contain duplicates in our scans. Other duplicates are still flagged as invalid as in the original observatory.

Additional details

There is a new result type CspImplementedButHasDuplicateDirectives (0 points) that kicks in when the CSP test generally passes, but duplicate report-* directives were detected.

@argl argl requested a review from a team as a code owner January 2, 2025 15:19
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM overall, just nits.

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Actually, this one is not a nit.

@caugner caugner changed the title chore(csp): duplicate report-* directives are allowed chore(csp): allow duplicate report-* directives Jan 2, 2025
@argl argl changed the title chore(csp): allow duplicate report-* directives fix(csp): allow duplicate report-* directives Jan 3, 2025
@argl argl requested a review from caugner January 3, 2025 11:27
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just one more thought: Wouldn't it be better to have a general _warnings key that is an array of warning objects, and could be used for other checks as well:

{
  "_warnings": [
    {
      "type": "duplicate_keys",
      "keys": ["report-to"]
    }
  ]
}

@argl
Copy link
Contributor Author

argl commented Jan 3, 2025

If we need any more warnings, I'll extend the scheme. There is no real infrastructure to conveigh these warnings to the user yet, though, apart from matching these to result constants.

@argl argl merged commit c172ce8 into main Jan 3, 2025
4 checks passed
@argl argl deleted the MP-1799-duplicate-csp-warning branch January 3, 2025 13:23
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.

2 participants