bermudalamb/redefined-designs
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects 3 Releases Wiki Activity

Serve user uploads from an origin of their own, not the app's #103

New Issue
Closed
opened 2026-08-21 16:24:20 -05:00 by bermudalamb · 1 comment
bermudalamb commented 2026-08-21 16:24:20 -05:00
Owner
Copy Link
Copy Source

app.ts:34 serves customer-visible uploaded files from the same origin as the application:

app.use('/uploads', express.static(process.env.UPLOADS_DIR || '/app/uploads'));

Anything in that directory is therefore same-origin content. A stored .html is served as text/html from the site's own origin; a stored .svg is served as image/svg+xml, and both can carry script that runs with the site's cookies and DOM.

This is the separation that is actually principled

It came out of asking whether the backend should move into its own container. It should not — that would publish a second port and change nothing about the admin boundary. But there is a real separation argument here, and it is not backend versus frontend. It is user-supplied content versus everything else.

Content a stranger uploaded and content you wrote have no business sharing an origin, because the origin is the entire unit of trust in a browser. A separate hostname pointing at the same files neutralises the whole class — stored XSS, cookie theft, DOM access — regardless of how many containers exist or what file types get through.

Relationship to #95 — complementary, not duplicate

#95 stops a dangerous file being stored: server-side type validation, so a .html never lands in the uploads directory. This issue stops a stored file doing damage if one ever gets there — through a validation gap, a .svg that is allowed as an image, a path added later, or a file already on disk from before validation existed.

Neither replaces the other, and the ordering matters: #95 is cheaper and should land first.

A correction to the cheaper mitigation I suggested

I proposed Content-Disposition: attachment as the low-cost version. That needs qualifying, and possibly discarding.

These files are product photographs rendered inline in <img> tags across the storefront. A header that tells the browser to download rather than display is exactly wrong for that, and browser behaviour for Content-Disposition on a subresource load is inconsistent enough that it should be tested rather than assumed.

The nuance that actually matters: an <img src="evil.svg"> does not execute script. The dangerous case is a browser navigating directly to /uploads/evil.svg or /uploads/evil.html — a link, a pasted URL, an <iframe>. So the mitigations worth measuring are the ones that affect direct navigation without touching inline rendering:

  • X-Content-Type-Options: nosniff, so a mislabelled file cannot be sniffed into HTML.
  • A restrictive Content-Security-Policy on responses from /uploads — default-src 'none'; sandbox — which constrains a directly-navigated document while leaving <img> embedding alone.
  • Serving an explicit safe Content-Type derived from the validated type rather than from the stored extension, which is #95's other half.

Each of those needs verifying against a real product photo still rendering, not reasoning about.

The migration detail that decides the cost

Image paths are stored in the database as relative URLs — /uploads/<uuid>.jpg — and rendered straight into src. Moving to a separate origin means either rewriting those to absolute URLs at render time from a configured base, or rewriting the stored values.

The render-time option is clearly better: one configured base URL, no migration, and the stored value stays a path rather than baking a hostname into data that outlives the hostname. It also keeps local development working, where there is no second origin and the base is simply empty.

Worth deciding

  • Separate hostname, or separate path with headers? The hostname is the real fix and needs an NPM host and possibly a certificate. Headers alone are a partial mitigation, and it is worth being honest about the word "partial" rather than treating them as equivalent.
  • Whether the uploads origin should be cookie-free. Most of the benefit comes from the app's session cookie not being scoped to it, which means a genuinely different hostname rather than a path on the same one.
  • Does the admin panel need the same treatment? Admin previews the same files, behind authentik, so the exposure there is narrower but not zero.

Severity

Medium. Not exploitable by an anonymous visitor today — uploading requires the admin API behind authentik — so this is defence in depth, exactly like #63. The reason it is worth doing anyway is that it is the mitigation that keeps working when one of the other controls fails, and the failure it prevents is script running as the site.

Found while reviewing whether to split the backend into its own container.

`app.ts:34` serves customer-visible uploaded files from the same origin as the application: ```ts app.use('/uploads', express.static(process.env.UPLOADS_DIR || '/app/uploads')); ``` Anything in that directory is therefore same-origin content. A stored `.html` is served as `text/html` from the site's own origin; a stored `.svg` is served as `image/svg+xml`, and both can carry script that runs with the site's cookies and DOM. ## This is the separation that is actually principled It came out of asking whether the backend should move into its own container. It should not — that would publish a second port and change nothing about the admin boundary. But there **is** a real separation argument here, and it is not backend versus frontend. It is **user-supplied content versus everything else**. Content a stranger uploaded and content you wrote have no business sharing an origin, because the origin is the entire unit of trust in a browser. A separate hostname pointing at the same files neutralises the whole class — stored XSS, cookie theft, DOM access — regardless of how many containers exist or what file types get through. ## Relationship to #95 — complementary, not duplicate #95 stops a dangerous file being **stored**: server-side type validation, so a `.html` never lands in the uploads directory. This issue stops a stored file **doing damage** if one ever gets there — through a validation gap, a `.svg` that is allowed as an image, a path added later, or a file already on disk from before validation existed. Neither replaces the other, and the ordering matters: #95 is cheaper and should land first. ## A correction to the cheaper mitigation I suggested I proposed `Content-Disposition: attachment` as the low-cost version. That needs qualifying, and possibly discarding. These files are **product photographs rendered inline** in `<img>` tags across the storefront. A header that tells the browser to download rather than display is exactly wrong for that, and browser behaviour for `Content-Disposition` on a subresource load is inconsistent enough that it should be tested rather than assumed. The nuance that actually matters: **an `<img src="evil.svg">` does not execute script.** The dangerous case is a browser *navigating directly* to `/uploads/evil.svg` or `/uploads/evil.html` — a link, a pasted URL, an `<iframe>`. So the mitigations worth measuring are the ones that affect direct navigation without touching inline rendering: - `X-Content-Type-Options: nosniff`, so a mislabelled file cannot be sniffed into HTML. - A restrictive `Content-Security-Policy` on responses from `/uploads` — `default-src 'none'; sandbox` — which constrains a directly-navigated document while leaving `<img>` embedding alone. - Serving an explicit safe `Content-Type` derived from the validated type rather than from the stored extension, which is #95's other half. Each of those needs verifying against a real product photo still rendering, not reasoning about. ## The migration detail that decides the cost Image paths are stored in the database as relative URLs — `/uploads/<uuid>.jpg` — and rendered straight into `src`. Moving to a separate origin means either rewriting those to absolute URLs at render time from a configured base, or rewriting the stored values. The render-time option is clearly better: one configured base URL, no migration, and the stored value stays a path rather than baking a hostname into data that outlives the hostname. It also keeps local development working, where there is no second origin and the base is simply empty. ## Worth deciding - **Separate hostname, or separate path with headers?** The hostname is the real fix and needs an NPM host and possibly a certificate. Headers alone are a partial mitigation, and it is worth being honest about the word "partial" rather than treating them as equivalent. - **Whether the uploads origin should be cookie-free.** Most of the benefit comes from the app's session cookie not being scoped to it, which means a genuinely different hostname rather than a path on the same one. - **Does the admin panel need the same treatment?** Admin previews the same files, behind authentik, so the exposure there is narrower but not zero. ## Severity Medium. Not exploitable by an anonymous visitor today — uploading requires the admin API behind authentik — so this is defence in depth, exactly like #63. The reason it is worth doing anyway is that it is the mitigation that keeps working when one of the other controls fails, and the failure it prevents is script running as the site. Found while reviewing whether to split the backend into its own container.
bermudalamb added this to the Make the password-reset email editable from Admin project 2026-08-21 16:26:27 -05:00
bermudalamb self-assigned this 2026-08-21 16:26:36 -05:00
bermudalamb added this to the Customer and Admin UI review findings project 2026-08-23 09:28:29 -05:00
bermudalamb removed this from the Make the password-reset email editable from Admin project 2026-08-23 09:28:48 -05:00
bermudalamb commented 2026-08-24 17:18:23 -05:00
Author
Owner
Copy Link
Copy Source

Done on feature/103-uploads-origin, both halves — the code half is complete, and the origin half is now one variable away.

Decisions taken

The mitigations were measured, not reasoned about. The issue was right that Content-Disposition: attachment is wrong for photographs rendered inline, and right that an <img src="evil.svg"> does not execute script. So none of that was used. What went in instead affects direct navigation and leaves inline rendering alone, and there is an integration test per header against a real product photo still being served.

An extension allowlist rather than headers alone. This is the part that goes beyond what the issue proposed, and it is the strongest of the lot: the app now serves only the three extensions the upload path can produce, and answers 404 for everything else. A .html or a .svg on disk is not a file this application hands out at all, so it never gets as far as needing a header to constrain it. An allowlist rather than a denylist because a denylist has to anticipate every type a browser might execute, which changes across browsers and years, while this only has to know three.

The 404 is deliberately the same answer as a file that does not exist, so a stranger cannot use the response to learn which paths are real.

Headers on what is served: X-Content-Type-Options: nosniff, an explicit Content-Type taken from the allowlist rather than sniffed or guessed from a submitted name, Content-Security-Policy: default-src 'none'; sandbox for the directly-navigated case, and Cross-Origin-Resource-Policy: cross-origin — which is needed the moment these are served from a hostname of their own, or a browser refuses the cross-origin <img> load. Writes get 405.

Render-time base, as the issue preferred. UPLOADS_BASE_URL is sent to the frontend at runtime through /api/config and joined onto stored paths by a uploadUrl helper that every <img src> now goes through — storefront, cart and admin, so the third open question ("does the admin panel need the same treatment?") is answered yes and for free. Stored paths stay site-relative, so there is no migration and no hostname baked into data that outlives it.

Runtime rather than a build-time variable, so one image serves every environment — the same reason paypalClientId and demoMode already come from there.

Answering "separate hostname, or headers?" — both, in that order of importance. The hostname is the real fix and it is still the real fix; the headers are honestly partial, and they are what protects the app's own origin, which goes on serving these files either way because the separate host points at the same directory.

Two things that need you

1. The hostname, when you want it. Everything is in place; it needs an NPM host for something like uploads.redefined-designs.bermudalamb.synology.me pointing at this same container, a certificate that covers it, and then UPLOADS_BASE_URL set on the Portainer stack. Until then the server warns at boot that the defence is off rather than staying silent. Note this interacts with #117.

UPLOADS_BASE_URL already has a line in docker-compose.prod.yml while still empty, deliberately — a stack variable with no line there is substituted into the file and never reaches the container, which is exactly how UPLOADS_DIR went missing on 2026-08-23.

2. Check the production volume for legacy files before deploying. The extension allowlist means anything on disk that is not .jpg, .png or .webp stops being served. Nothing the upload path has ever accepted since #95 can be affected, but files predating it can. Worth confirming it comes back empty:

ls /volume1/configs/redefined-designs/uploads | grep -viE '\.(jpg|png|webp)$'

If anything shows up, it is worth looking at what it is before deciding — that is precisely the class of file this issue exists to stop serving, but it might also be a real product photo with an odd extension.

Verification

264 unit tests and 251 integration tests pass, including ten new integration cases covering each header, the 404 for hostile markup, the 404 for an unrecognised extension on a file that genuinely is a PNG, case-insensitive extensions, and the 405. Verified through a real server behind the dev proxy as well as through supertest: a PNG returns 200 with all four headers, and a stored .html returns 404.

There is no unit test for uploadUrl because the frontend has no unit test runner; it is exercised by the end-to-end suite through every image on the storefront.

Done on `feature/103-uploads-origin`, both halves — the code half is complete, and the origin half is now one variable away. ## Decisions taken **The mitigations were measured, not reasoned about.** The issue was right that `Content-Disposition: attachment` is wrong for photographs rendered inline, and right that an `<img src="evil.svg">` does not execute script. So none of that was used. What went in instead affects direct navigation and leaves inline rendering alone, and there is an integration test per header against a real product photo still being served. **An extension allowlist rather than headers alone.** This is the part that goes beyond what the issue proposed, and it is the strongest of the lot: the app now serves only the three extensions the upload path can produce, and answers 404 for everything else. A `.html` or a `.svg` on disk is not a file this application hands out at all, so it never gets as far as needing a header to constrain it. An allowlist rather than a denylist because a denylist has to anticipate every type a browser might execute, which changes across browsers and years, while this only has to know three. The 404 is deliberately the same answer as a file that does not exist, so a stranger cannot use the response to learn which paths are real. **Headers on what is served:** `X-Content-Type-Options: nosniff`, an explicit `Content-Type` taken from the allowlist rather than sniffed or guessed from a submitted name, `Content-Security-Policy: default-src 'none'; sandbox` for the directly-navigated case, and `Cross-Origin-Resource-Policy: cross-origin` — which is needed the moment these are served from a hostname of their own, or a browser refuses the cross-origin `<img>` load. Writes get 405. **Render-time base, as the issue preferred.** `UPLOADS_BASE_URL` is sent to the frontend at runtime through `/api/config` and joined onto stored paths by a `uploadUrl` helper that every `<img src>` now goes through — storefront, cart and admin, so the third open question ("does the admin panel need the same treatment?") is answered yes and for free. Stored paths stay site-relative, so there is no migration and no hostname baked into data that outlives it. Runtime rather than a build-time variable, so one image serves every environment — the same reason `paypalClientId` and `demoMode` already come from there. **Answering "separate hostname, or headers?" — both, in that order of importance.** The hostname is the real fix and it is still the real fix; the headers are honestly partial, and they are what protects the app's own origin, which goes on serving these files either way because the separate host points at the same directory. ## Two things that need you **1. The hostname, when you want it.** Everything is in place; it needs an NPM host for something like `uploads.redefined-designs.bermudalamb.synology.me` pointing at this same container, a certificate that covers it, and then `UPLOADS_BASE_URL` set on the Portainer stack. Until then the server warns at boot that the defence is off rather than staying silent. Note this interacts with #117. `UPLOADS_BASE_URL` already has a line in `docker-compose.prod.yml` while still empty, deliberately — a stack variable with no line there is substituted into the file and never reaches the container, which is exactly how `UPLOADS_DIR` went missing on 2026-08-23. **2. Check the production volume for legacy files before deploying.** The extension allowlist means anything on disk that is not `.jpg`, `.png` or `.webp` stops being served. Nothing the upload path has ever accepted since #95 can be affected, but files predating it can. Worth confirming it comes back empty: ``` ls /volume1/configs/redefined-designs/uploads | grep -viE '\.(jpg|png|webp)$' ``` If anything shows up, it is worth looking at what it is before deciding — that is precisely the class of file this issue exists to stop serving, but it might also be a real product photo with an odd extension. ## Verification 264 unit tests and 251 integration tests pass, including ten new integration cases covering each header, the 404 for hostile markup, the 404 for an unrecognised extension on a file that genuinely is a PNG, case-insensitive extensions, and the 405. Verified through a real server behind the dev proxy as well as through supertest: a PNG returns 200 with all four headers, and a stored `.html` returns 404. There is no unit test for `uploadUrl` because the frontend has no unit test runner; it is exercised by the end-to-end suite through every image on the storefront.
bermudalamb added reference feature/103-uploads-origin 2026-08-24 17:38:20 -05:00
bermudalamb moved this to In Progress in Customer and Admin UI review findings on 2026-08-24 17:38:26 -05:00
bermudalamb moved this to Ready for Release in Customer and Admin UI review findings on 2026-09-09 13:33:58 -05:00
Sign in to join this conversation.
feature/103-uploads-origin
Branches Tags
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Customer and Admin UI review findings
Ready for Release
Assignees
Clear assignees
calamb (Chelsea Lamb) bermudalamb (Thom Lamb)
No Assignees bermudalamb
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#103