-
-
Notifications
You must be signed in to change notification settings - Fork 870
feat(secure-headers): Add CSP report-to and report-uri directive support #4555
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4555 +/- ##
=======================================
Coverage 91.49% 91.49%
=======================================
Files 172 172
Lines 11230 11230
Branches 3257 3259 +2
=======================================
Hits 10275 10275
Misses 954 954
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const resultValues: string[] = [] | ||
|
|
||
| for (const [directive, value] of Object.entries(contentSecurityPolicy)) { | ||
| if (directive === 'reportUri') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that this if (directive === 'reportUri') {} and if (contentSecurityPolicy.reportUri) {} is not necessary, right?
So I think that we can apply this patch:
diff --git a/src/middleware/secure-headers/secure-headers.ts b/src/middleware/secure-headers/secure-headers.ts
index 55afa21d..cb81a0fd 100644
--- a/src/middleware/secure-headers/secure-headers.ts
+++ b/src/middleware/secure-headers/secure-headers.ts
@@ -244,9 +244,6 @@ function getCSPDirectives(
const resultValues: string[] = []
for (const [directive, value] of Object.entries(contentSecurityPolicy)) {
- if (directive === 'reportUri') {
- continue
- }
const valueArray = Array.isArray(value) ? value : [value]
valueArray.forEach((value, i) => {
@@ -266,12 +263,6 @@ function getCSPDirectives(
'; '
)
}
- if (contentSecurityPolicy.reportUri) {
- const uris = Array.isArray(contentSecurityPolicy.reportUri)
- ? contentSecurityPolicy.reportUri
- : [contentSecurityPolicy.reportUri]
- resultValues.push('report-uri', ' ', uris.join(' '), '; ')
- }
resultValues.pop()
return callbacks.length === 0There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing that out! I removed those lines as suggested! If there's any more suggestions, I'll be happy to implement.
Remove 'reportUri' directive handling from Content Security Policy.
yusukebe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Hey @cruzz77 ! Looks good to me. I'll merge this later and include this change in the next minor release. Thank you for your contribution! |
Thank a lot! Had fun doing this! |
Description
This PR adds support for CSP
report-toandreport-uridirectives to thesecure-headersmiddleware, addressing the feature request in #4527.Implements the functionality accordingly.
Changes
reportToandreportUrioptions to the CSP configurationContent-Security-PolicyheaderUsage
Checklist
bun run format:fix && bun run lint:fixto format the codeTesting
reportToandreportUrioptions