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

Add the Report-To header and correct report-to syntax #17

Merged
merged 7 commits into from
Jul 18, 2024

Conversation

edwilde
Copy link
Contributor

@edwilde edwilde commented Oct 19, 2023

Fixes #16

The report-to CSP syntax was being generated incorrectly, it should be a string group-name, with associated Report-To header - see examples.

This does a few things:

  • Fixes the syntax and adds the Report-To header if the report-to directive is set
  • Adds a bunch of tests for each different scenario
    • If the environment variable is set with a single url, then both report-uri and report-to directives are included
    • If the environment variable is set with multiple urls (comma separated) then just the report-to directives are included, as report-uri doesn't support multiple endpoints
    • If the environment variable is not set, then either report-uri or report-to directives, or both, can be set manually
    • If either directive is set with an empty string, then the directive is removed, for example I might want to override the environment variable for a single url and only use report-uri.

I have kept the previous, invalid, directive syntax for report-to and tidied it up when the header is added, this should mean this is backwards compatible.

And lastly, a cosmetic change - I added a space after each ; in the CSP header. This is valid syntax and makes it much easier to read.

@Cheddam
Copy link

Cheddam commented Jul 1, 2024

Any chance this could be reviewed and merged? I'm looking at adopting this module (and potentially contributing some enhancements), but functional reporting is a key requirement for the project.

Copy link
Contributor

@Jianbinzhu Jianbinzhu left a comment

Choose a reason for hiding this comment

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

This looks good ✅

@Jianbinzhu Jianbinzhu merged commit a5944e5 into main Jul 18, 2024
27 checks passed
@Jianbinzhu Jianbinzhu deleted the bugfix/csp-report-to-syntax branch July 18, 2024 06:07
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.

CSP report-to syntax is incorrect
3 participants