-
Notifications
You must be signed in to change notification settings - Fork 213
Add CSP headers specifically for Web Forms #1526
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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
This was referenced Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
2 tasks
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
alxndrsn
reviewed
Dec 4, 2025
Member
Author
|
I'm happy with where this is. It's my PR so I can't approve it. @alxndrsn please squash and merge when you're happy with it! |
Contributor
@lognaturel does this need to be done? I haven't merged due to this - please feel free to merge, either with or without |
alxndrsn
approved these changes
Dec 5, 2025
alxndrsn
reviewed
Dec 5, 2025
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1130
Alternative to https://github.com/getodk/central/pull/1467/files
What has been done to verify that this works as intended?
Put on dev server, loaded All Question Types form as new submission, Public Link, and for submission edit. Looked in console for any violations.
I did NOT review reported violations extensively.
Why is this the best possible solution? Were any other approaches considered?
The most obvious alternative is to have the same policy for all of frontend. I don't think that would be so bad but the increased granularity is nice in some cases. The regex approach to matching routes feels a little unsatisfying/brittle but the stakes are low if there's a mistake because the policies are not very different.
One challenge for this behavior is where to define the Web Forms-specific headers which apply to several different routes. This PR uses a map to go from the request URI to the intended CSP headers. That feels like a clean, easy-to-read solution to me. I mostly took the regex from #1467 and trusted that the tests are complete enough.
I tried mapping on
$urito allow query parameters but realized that gets rewritten so mapped on$request_uriand explicitly allowed query params in the regex.The directives are intended to be in alphabetical order with
default-src: 'none'listed first for readability.Two fetch directives are omitted:
child-srcandprefetch-src. I think that's ok because they're not likely to be the cause of violations. The more specific directives likescript-elem-srcare also omitted and I think that's ok too.TODO:
I also reviewed each existing directive:
default-srcnonenoneconnect-src'self' Google translate'self' https:font-src'self''self' data:data:is because Roboto gets downloaded (note: seeing it on test. but not dev.)frame-src'self' https://getodk.github.io/central/news.html'none'img-srchttps: data:https: blob:data:image request from frontend buthttps:for frontend is because users can link to arbitrary images in markdown.https:for Web Forms is currently for map tiles. We could pretty easily be more restrictive here I think.manifest-src'none''none'media-src'none''none'object-src'none''none'script-src'self''self' 'wasm-unsafe-eval'style-src'self''self' 'unsafe-inline'DatePickerstyle-src-attr'unsafe-inline'worker-srcblob:blob:How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
The most important intended behavior change is that we start getting more meaningful / less noisy reports and that users don't see warnings in their consoles.
No immediate regression risk because we're still doing report only. We'd like to turn on actual protection as soon as possible, though, so if we made mistakes we run the risk of further delaying that.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No
Before submitting this PR, please make sure you have:
nextbranch OR only changed documentation/infrastructure (masteris stable and used in production)