fix(website): Add `Access control allow origin` header for website file requests by tombch · Pull Request #7304 · loculus-project/loculus

tombch · 2026-09-10T15:54:00Z

Summary

Switching to outputFileUrlType: website in #7301 showed these errors in the CI:

Error: Unexpected console error: Access to fetch at 'http://10.1.0.60:3000/seq/LOC_000001Y.1/annotations/LOC_000001Y.1.embl' from origin 'http://localhost:3000' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

this only appears for chromium because of logic in console-warnings.fixture.ts:

                if (!isHarmless && browserName === 'chromium') {
                    expect(false, `Unexpected console ${msg.type()}: ${messageText}`).toBe(true);
                }
            }

CORS always hurts my head but through some discussions with Claude the issue is essentially:

To get the file size, the website uses fetch to do a HEAD request to whatever the absolute file URL is.
Before, with URL type backend, this was fine because the backend has allowedOrigins('*') in WebConfig.kt.
However with URL type website, this hits [fileName].ts which never returned an Allow origins: * header to the browser. Therefore, any cross origin request here gets blocked by the browser.
This only appears as an issue in CI because the playwright browser is at localhost but the fetch request goes to 10.1.0.60 which is the GitHub Actions runner's own private network address (therefore a cross origin request).

The fix is then to do what the backend does and Allow origins: * for this website endpoint. As far as I understand, this is not an issue security wise as with the current setup, Allow origins: * means a cross-origin request with user cookies would get a response but the browser will refuse to read it. If we also enabled Access-Control-Allow-Credentials: true that could cause problems with a malicious site making authenticated requests.

Notes

I would think a more straightforward thing in future is to have the file size not need to be requested separately, and remove the separate HEAD request entirely, I think this is a bigger change though.

Screenshot

PR Checklist

All necessary documentation has been adapted.
The implemented feature is covered by appropriate, automated tests.
Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: Add preview label to enable

claude · 2026-09-10T15:55:50Z

Claude finished @tombch's task in 2m 18s —— View job

Review

The diagnosis in the PR description is correct and the fix is the right one. Two inline comments, both non-blocking.

Your security reasoning holds. Access-Control-Allow-Origin: * is safe here specifically because the browser refuses to pair * with credentials: 'include', so a cross-origin caller can never get the session cookie attached. Without the cookie there is no session, getAccessToken returns nothing, no Authorization header reaches the backend, and the backend only hands back a presigned URL for a publicly-readable file. A malicious page also can't read the Location of the 307. The route is read-only (GET/HEAD), so there's no state-change/CSRF angle either.

The fix is complete, not just half the chain. Worth stating explicitly since it's the thing that could have made this a partial fix: the client uses redirect: 'follow' (DataTableEntryValue.tsx:148), so the browser applies the CORS check to every hop — the 307 from this route and the final response from S3. This PR only fixes the first hop. The second hop must already be fine, because the previous outputFileUrlType: backend path also terminated in an S3 redirect and worked. Similarly, Content-Length — the only header the client reads — is CORS-safelisted, so no Access-Control-Expose-Headers is needed on either hop.

Scope is right. ReleasedDataModel.kt:255 confirms FileUrlType.WEBSITE only ever generates /seq/{accession}.{version}/{category}/{name}, so this one route is the only website endpoint that needed the header.

Minor, not worth an inline comment: an accession that fails parseAccessionVersionFromString (line 18) throws before any of the withCorsHeader call sites, so Astro's own 500 goes back without the header. Harmless — the fetch fails either way — but it means a malformed-accession bug would surface in the console as a misleading CORS error rather than the real one.

The eslint-disable-next-line on line 40 still lands on the right line after the wrapping, and headers.set (rather than append) correctly overwrites any Access-Control-Allow-Origin copied from the backend response, so no duplicate-header risk in the passthrough branch.

Not verified

I could not run CI=1 npm run test / npm run check-types — the commands needed approval in this environment, so this review is static analysis only. The new test asserts on a Response built by undici, where Location and Access-Control-Allow-Origin are both ordinary (non-forbidden) response headers, so it should behave as written — but please confirm CI is green.

Gather context (diff vs origin/outputFileUrlType-website)
Read changed files and surrounding code
Analyze CORS correctness & security
Review test coverage
Post review
· branch allow-origin-website-file

claude · 2026-09-10T15:57:50Z

+// Allow requests from any origin
+// Same as the backend allows on /files/get, which this route proxies
+function withCorsHeader(headers: Headers): Headers {
+    headers.set('Access-Control-Allow-Origin', '*');
+    return headers;
+}


No OPTIONS handler is exported, so this route only supports simple CORS requests. That's sufficient today — the only cross-origin caller is DataTableEntryValue.tsx:148, fetch(url, { method: 'HEAD', redirect: 'follow' }) with no custom headers, which the browser sends without a preflight.

It becomes a silent trap the moment anyone adds a header to that fetch (e.g. Authorization, or a custom X- header): the preflight OPTIONS would hit Astro, get no matching handler, and the request would fail with the same opaque CORS error this PR is fixing. The backend already covers this via allowedMethods(... "OPTIONS") in WebConfig.kt:16.

Worth either exporting a trivial OPTIONS handler alongside GET/HEAD, or extending the comment to record that only simple requests are supported.

Fix this →

I think adding a comment is sufficient for now

claude · 2026-09-10T15:58:00Z

+    test('allows any origin on the S3 redirect', async () => {
+        const response = await callRoute({ isLoggedIn: false });
+        expect(response.headers.get('Access-Control-Allow-Origin')).toBe('*');
+    });


withCorsHeader is applied at three call sites, but only the 307 redirect path is covered here. The untested passthrough branch ([fileName].ts:45) is the one most likely to regress, because it's the only place where the header map is seeded from the backend response (new Headers(response.headers)) rather than built fresh — a future change there could easily drop or overwrite the header.

Suggest adding at least the passthrough case, e.g. a 404 from the backend:

test('allows any origin on non-redirect responses', async () => { fetchMock.mockResolvedValue(new Response('not found', { status: 404 })); const response = await callRoute({ isLoggedIn: false }); expect(response.status).toBe(404); expect(response.headers.get('Access-Control-Allow-Origin')).toBe('*'); });

Fix this →

theosanderson · 2026-09-10T17:21:59Z

 import { getAccessToken } from '../../../../utils/getAccessToken';

+// Allow requests from any origin
+// Same as the backend allows on /files/get, which this route proxies


Suggested change

// Same as the backend allows on /files/get, which this route proxies

Just an idea - (A) maybe it's not needed (B) proxied would initially make me think we're passing through the whole file, which we're not (though I accept it's accurate)

[but we say proxy below so ignore B]

claude actually thinks it is good to make these changes even if we were to refactor FileSizeComponent:

This is the URL the backend embeds in its public released-data API response (what getReleasedData/LAPIS consumers pull metadata from) for every file attached to a public sequence. Any third party building on top of that public API — a genome browser, an analysis notebook, some downstream tool — pulls that URL out of the JSON and does its own fetch() on it, from its own origin. That's the real cross-origin scripted consumer this header is for.

Yeah I can change the wording! In terms of the header, I think it makes sense out of the options, mainly as that's why the backend URLs already worked for the file size fetch.

There's also the option to do a relative URL in the file size component (like how FilesDialog does) but I just tried this and its a lot of prop threading of accession version and file category. There's also the option to include file size in released data but not sure we want that?

There's also the option to include file size in released data but not sure we want that?

I feel like this would make sense (but is probably not possible as released data is consumed by SILO so it needs a specific structure)

I think we should add this header either way for third party consumers

Yeah I agree

theosanderson · 2026-09-10T17:26:33Z

    if (response.status === 307 || response.status === 302) {
        const s3Url = response.headers.get('Location');
        if (!s3Url) {
-            return new Response('Backend redirect missing Location header', { status: 500 });
+            return new Response('Backend redirect missing Location header', {
+                status: 500,
+                headers: withCorsHeader(new Headers()),
+            });
        }
        return new Response(null, {
            status: response.status,
            // eslint-disable-next-line @typescript-eslint/naming-convention
-            headers: { Location: s3Url },
+            headers: withCorsHeader(new Headers({ Location: s3Url })),
        });
    }


Reading this code I don't actually see that we need this branch at all?

[sorry I know that's not on you/ this PR]

@theosanderson-agent do you see a reason for the 307/302 branch?

No, I don't think it's needed anymore. It looks like a leftover from history:

The branch was added in feat(website): Add dialog to view processed files in 'review' window (before approving) #4298, when the fallthrough was new Response(response.body, { status }) with no headers, so Location had to be forwarded explicitly.

feat: display sizes of extra files on sequence details page #5077 then changed the fallthrough to copy all backend headers (new Headers(response.headers)), which already includes Location. Since then the branch has been redundant: for a 307 the fallthrough would return the same status, the same Location, and now the same CORS header.

The 302 check has never matched anything: the backend always returns SC_TEMPORARY_REDIRECT (307). The 302 seems to date from the pre-feat(website): Add dialog to view processed files in 'review' window (before approving) #4298 version of this route, which itself issued a 302.

Node's fetch with redirect: 'manual' exposes the real 307 response (no opaque-redirect filtering like in browsers), so Location is readable in the fallthrough. The branch already relied on that anyway.

The only behavioural differences if it's removed: the 307 would also pass through any other backend headers (e.g. Content-Length: 0, Vary), which is harmless, and the "missing Location" 500 case would go away, which would just surface as a 307 without Location instead.

I checked locally: with the whole if block deleted, [fileName].spec.ts still passes (4/4), including the 307 + Location assertions.

Yeah I guess it would be losing a guard that the location is there, but that is something the backend explicitly sets so should always be there really

anna-parker

approving as I think the suggestions are small and the 302/307 branch can also be removed in a follow up PR if needbe

Allow any origin for file requests

d82fde2

claude Bot added the website Tasks related to the web application label Sep 10, 2026

tombch marked this pull request as ready for review September 10, 2026 15:55

claude Bot reviewed Sep 10, 2026

View reviewed changes

tombch requested review from anna-parker and theosanderson September 10, 2026 16:06

theosanderson reviewed Sep 10, 2026

View reviewed changes

anna-parker approved these changes Sep 11, 2026

View reviewed changes

Uh oh!

Conversation

tombch commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notes

Screenshot

PR Checklist

Uh oh!

claude Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Not verified

Uh oh!

claude Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anna-parker Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anna-parker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tombch commented Sep 10, 2026 •

edited

Loading

claude Bot commented Sep 10, 2026 •

edited

Loading

anna-parker Sep 11, 2026 •

edited

Loading