Skip to content

csp: allow backend passthrough for /oidc/callback #1478

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.

Sign up for GitHub

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
alxndrsn merged 5 commits into from
Dec 2, 2025
Merged

csp: allow backend passthrough for /oidc/callback #1478

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
534aac6
csp: allow backend passthrough for /oidc/callback
Nov 5, 2025
1acbe3d
Merge branch 'next' into oidc-csp
alxndrsn Nov 18, 2025
30b1cf4
Merge branch 'next' into oidc-csp
alxndrsn Nov 20, 2025
96a2910
Merge branch 'next' into oidc-csp
alxndrsn Dec 2, 2025
1a406b4
resinstate top-level headers
Dec 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions files/nginx/backend.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
proxy_set_header X-Forwarded-Proto $scheme;
proxy_pass http://service:8383;
proxy_redirect off;

# buffer requests, but not responses, so streaming out works.
proxy_request_buffering on;
proxy_buffering off;
proxy_read_timeout 2m;
20 changes: 10 additions & 10 deletions files/nginx/odk.conf.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,6 @@ server {

server_tokens off;

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;
Comment on lines 94 to 95
Copy link
Member

@matthew-white matthew-white Dec 2, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@alxndrsn alxndrsn Dec 2, 2025

Choose a reason for hiding this comment

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

Is it definitely safe to remove this include?

Good question!

Do all location blocks 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.

Copy link
Member

@matthew-white matthew-white Dec 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. location blocks that individually include common-headers.conf and add the Content-Security-Policy-Report-Only header (including location blocks that try_files ... @blank.html;).
  2. location blocks that return 301
    • These don't need any of the normal response headers, right?
  3. /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.

Copy link
Member

@matthew-white matthew-white Dec 2, 2025

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@alxndrsn alxndrsn Dec 2, 2025

Choose a reason for hiding this comment

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

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.


client_max_body_size 100m;

gzip on;
Expand Down Expand Up @@ -145,6 +142,7 @@ server {

# More lax CSP for enketo-express:
# Google Maps API: https://developers.google.com/maps/documentation/javascript/content-security-policy
proxy_hide_header Content-Security-Policy-Report-Only;
add_header Content-Security-Policy-Report-Only "default-src 'none'; connect-src 'self' blob: https://maps.googleapis.com/ https://maps.google.com/ https://maps.gstatic.com/mapfiles/ https://fonts.gstatic.com/ https://fonts.googleapis.com/ https://translate.google.com https://translate.googleapis.com; font-src 'self' https://fonts.gstatic.com/; frame-src 'none'; img-src data: blob: jr: 'self' https://maps.google.com/maps/ https://maps.gstatic.com/mapfiles/ https://maps.googleapis.com/maps/ https://tile.openstreetmap.org/ https://translate.google.com; manifest-src 'none'; media-src blob: jr: 'self'; object-src 'none'; script-src 'unsafe-inline' 'self' https://maps.googleapis.com/maps/api/js/ https://maps.google.com/maps/ https://maps.google.com/maps-api-v3/api/js/; style-src 'unsafe-inline' 'self' https://fonts.googleapis.com/css; style-src-attr 'unsafe-inline'; report-uri /csp-report";
#
# Rules set to 'none' here would fallback to default-src if excluded.
Expand All @@ -154,15 +152,17 @@ server {
}
# End of Enketo Configuration.

location ~ ^/v\d+/oidc/callback$ {
include /usr/share/odk/nginx/common-headers.conf;
include /usr/share/odk/nginx/backend.conf;
}

location ~ ^/v\d {
proxy_set_header X-Forwarded-Proto $scheme;
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";
Copy link
Member

@matthew-white matthew-white Dec 2, 2025

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor Author

@alxndrsn alxndrsn Dec 3, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@alxndrsn alxndrsn Dec 3, 2025

Choose a reason for hiding this comment

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

Filed at:


# buffer requests, but not responses, so streaming out works.
proxy_request_buffering on;
proxy_buffering off;
proxy_read_timeout 2m;
include /usr/share/odk/nginx/common-headers.conf;
include /usr/share/odk/nginx/backend.conf;
}

location @blank.html {
Expand Down
1 change: 1 addition & 0 deletions nginx.dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ COPY files/nginx/setup-odk.sh \
/scripts/

COPY files/nginx/redirector.conf /usr/share/odk/nginx/
COPY files/nginx/backend.conf /usr/share/odk/nginx/
COPY files/nginx/common-headers.conf /usr/share/odk/nginx/
COPY files/nginx/robots.txt /usr/share/nginx/html
COPY --from=intermediate client/dist/ /usr/share/nginx/html
Expand Down
4 changes: 4 additions & 0 deletions test/nginx/mock-http-server/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ const app = express();

app.use((req, res, next) => {
console.log(new Date(), req.method, req.originalUrl);

// always set CSP header to detect (or allow) leaks from backend through to the client
res.set('Content-Security-Policy-Report-Only', 'default-src NOTE:FROM-BACKEND');

next();
});

Expand Down
13 changes: 13 additions & 0 deletions test/nginx/test-nginx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ const contentSecurityPolicies = {
'img-src': 'https://translate.google.com',
'report-uri': '/csp-report',
},
'backend-unmodified': {
'default-src': 'NOTE:FROM-BACKEND',
},
'central-frontend': {
'default-src': none,
'connect-src': [
Expand Down Expand Up @@ -353,6 +356,16 @@ describe('nginx config', () => {
);
});

it('/oidc/callback should serve Content-Security-Policy from backend', async () => {
// when
const res = await fetchHttps('/v1/oidc/callback');

// then
assert.equal(res.status, 200);
assert.equal(await res.text(), 'OK');
assertSecurityHeaders(res, { csp:'backend-unmodified' });
});

it('should set x-forwarded-proto header to "https"', async () => {
// when
const res = await fetchHttps('/v1/reflect-headers');
Expand Down