csp: allow backend passthrough for /oidc/callback #1478
Conversation
Generate the proper content-hash for the page's inline `<style>` element. Ref getodk/central#1235 Ref getodk/central#1478
|
Maybe I should review this one after all, since I reviewed getodk/central-backend#1679, and I commented on #1479. |
matthew-white
requested review from
matthew-white
and removed request for
brontolosone
Generate the proper content-hash for the page's inline `<style>` element. Ref getodk/central#1235 Ref getodk/central#1478
| add_header Content-Security-Policy-Report-Only "default-src 'none'; connect-src https://translate.google.com https://translate.googleapis.com; img-src https://translate.google.com; report-uri /csp-report"; | ||
| include /usr/share/odk/nginx/common-headers.conf; |
There was a problem hiding this comment.
Is it definitely safe to remove this include? Do all location blocks that need it already individually include it?
Similarly for the Content-Security-Policy-Report-Only header, is the reason for removing it here because it's already been added to all the individual location blocks that need it?
There was a problem hiding this comment.
Is it definitely safe to remove this
include?
Good question!
Do all
locationblocks that need it already individually include it?
I think so, as do the current tests. But that's not a guarantee!
The most conservative approach might be to lock down all headers here as much as possible. Should we try that? That could definitely catch some cases, but still won't protect from add_header directives in lower-level blocks from blowing away all headers set here.
There was a problem hiding this comment.
I actually kind of prefer removing these lines if possible. I feel like it's easier to reason about the headers if they're all on one level. But I thought I'd check about it first.
I did a scan of the location blocks in this PR, and there seem to be three categories:
locationblocks that individuallyincludecommon-headers.conf and add theContent-Security-Policy-Report-Onlyheader (includinglocationblocks thattry_files ... @blank.html;).locationblocks thatreturn 301- These don't need any of the normal response headers, right?
- /csp-report
- This is a specific use case, so maybe it doesn't need any of the normal response headers. Users shouldn't be navigating to this URL directly. What do you think?
- This is also a proxy to Sentry.
There was a problem hiding this comment.
I'm also open to locking down all headers here if you think that's better. 👍 Or at least locking down Content-Security-Policy-Report-Only. I think my first choice would be to remove these lines if possible, but it's not a strong preference (I'm happy either way).
There was a problem hiding this comment.
seem to be three categories
👍 looks accurate to me
These don't need any of the normal response headers, right?
I think this is a slightly complicated question. I'll reinstate these lines, and merge the PR. We can try to delete or refine these default headers down the line.
| proxy_pass http://service:8383; | ||
| proxy_redirect off; | ||
| proxy_hide_header Content-Security-Policy-Report-Only; | ||
| add_header Content-Security-Policy-Report-Only "default-src 'none'; connect-src https://translate.google.com https://translate.googleapis.com; img-src https://translate.google.com; report-uri /csp-report"; |
There was a problem hiding this comment.
This is kind of an aside/tangent, but why do we include these Google Translate URLs for the Backend CSP? Backend almost never returns HTML, so I feel like the Google Translate bit isn't needed. Google Translate isn't going to translate JSON returned from Backend. I know this has been the default value of this header, but I wonder whether it could be further simplified for Backend.
Or is the attitude more like, "it never hurts to include Google Translate, so let's just always include it"?
There was a problem hiding this comment.
I think this is a mistake - API services should not be loaded in the browser as the main document, so it should be safe to completely lock down their CSPs. On the other hand, the blank.html file may be loaded as the main document, so should probably be allowing google translate.
There was a problem hiding this comment.
Updates `setup-odk.sh` to patch correct nginx config file ref pinning `X-Forwarded-Proto` to 'https'. Originally broken in 924d320 / #1478. For more discussion, see: * https://forum.getodk.org/t/odk-central-2025-3-cannot-login-due-to-401-delete-users-session-current/57224 * https://forum.getodk.org/t/setting-up-central-behind-a-proxy-this-authentication-method-is-only-available-over-https/57236/10
2 participants
With getodk/central-backend#1679, closes #1235
What has been done to verify that this works as intended?
Updated tests.
Why is this the best possible solution? Were any other approaches considered?
This allows style content-hash in CSP set in getodk/central-backend#1679 to pass through nginx.
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?
Should reduce CSP violation reports; most users won't notice they were happening in the first place.
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)