The inventory image upload accepts any file type, in the picker and on the server #95

New Issue

2026-08-21T13:51:06-05:00

bermudalamb commented

2026-08-21 13:51:06 -05:00

The Upload control in Admin → Inventory opens a file picker with no type filter, so choosing a product photo means scrolling past every file in the folder. It should default to image types, with "all files" still available as the secondary choice.

The ask

frontend/src/admin/Admin.tsx renders the control with no accept:

<Upload fileList={fileList} beforeUpload={() => false} onChange={...}
  maxCount={6} multiple listType="picture-card">

Adding accept="image/*" makes the native picker default to images while leaving the operating system's own "All files" option in place — which is exactly the primary/secondary behaviour asked for, and it comes for free rather than needing to be built.

But `accept` is a hint, not a control, and the server has none

Worth stating plainly, because adding accept alone would make this look solved while changing nothing about what can actually be uploaded.

accept only preselects a filter in the picker. The operating system still offers "All files", drag-and-drop bypasses it entirely, and anything calling the API directly never sees it at all. The server is where the restriction has to exist, and today there isn't one:

const upload = multer({
  storage,
  limits: { fileSize: MAX_IMAGE_BYTES, files: MAX_IMAGES_PER_REQUEST, ... }
});

There is no fileFilter. Size and count are bounded; type is not. POST /api/admin/items accepts a PDF, a zip, or an executable and stores it as an item image.

Two details that make this more than untidy

The stored extension comes from the client. multer.diskStorage's filename callback takes path.extname(file.originalname) and appends it to a random UUID. The random name is deliberate and good — the comment explains it is a CSPRNG so paths cannot be guessed — but the extension is whatever the caller sent.

/uploads is served by express.static. So a file stored as .html is served as text/html from the site's own origin, and a .svg is served as image/svg+xml — both of which can carry script and would run as same-origin. That is stored cross-site scripting against the storefront.

Being accurate about the reach: uploading requires the admin API, which sits behind authentik, so this is not something an anonymous visitor can do today. It needs an admin account — or the gap #63 exists to close, since that boundary is one regex in a proxy config outside this repo and is bypassed by anything reaching the container port directly. So this is defence in depth rather than an open hole, but it is a cheap one to add and the consequence if the boundary ever slips is disproportionate.

Suggested shape

accept="image/*" on the Upload, which is the requested behaviour.
A multer fileFilter allowing only the image mime types actually wanted, rejecting anything else with a 400 through the existing error translation.
Derive the stored extension from the accepted type rather than from the client's filename, so the name on disk cannot disagree with what the file is.
Decide explicitly whether SVG is allowed. It is an image and it can contain script, so "images only" does not settle it. Excluding it is the simpler answer for product photos.

Verification

An integration test per rejection — a non-image refused with 400, an accepted image still stored — and a check that the stored filename's extension comes from the type rather than the submitted name. The current tests cover size and count limits but not type, which is why this went unnoticed.

Severity

Low as a convenience, medium as a gap. Nothing is broken, no incident has occurred, and the admin boundary does hold today — but an upload path with no type validation, whose output is served statically from the same origin, is worth closing while it is a five-line change.

The Upload control in Admin → Inventory opens a file picker with no type filter, so choosing a product photo means scrolling past every file in the folder. It should default to image types, with "all files" still available as the secondary choice. ## The ask `frontend/src/admin/Admin.tsx` renders the control with no `accept`: ```tsx <Upload fileList={fileList} beforeUpload={() => false} onChange={...} maxCount={6} multiple listType="picture-card"> ``` Adding `accept="image/*"` makes the native picker default to images while leaving the operating system's own "All files" option in place — which is exactly the primary/secondary behaviour asked for, and it comes for free rather than needing to be built. ## But `accept` is a hint, not a control, and the server has none Worth stating plainly, because adding `accept` alone would make this look solved while changing nothing about what can actually be uploaded. `accept` only preselects a filter in the picker. The operating system still offers "All files", drag-and-drop bypasses it entirely, and anything calling the API directly never sees it at all. The server is where the restriction has to exist, and today there isn't one: ```ts const upload = multer({ storage, limits: { fileSize: MAX_IMAGE_BYTES, files: MAX_IMAGES_PER_REQUEST, ... } }); ``` There is **no `fileFilter`**. Size and count are bounded; type is not. `POST /api/admin/items` accepts a PDF, a zip, or an executable and stores it as an item image. ## Two details that make this more than untidy **The stored extension comes from the client.** `multer.diskStorage`'s filename callback takes `path.extname(file.originalname)` and appends it to a random UUID. The random name is deliberate and good — the comment explains it is a CSPRNG so paths cannot be guessed — but the extension is whatever the caller sent. **`/uploads` is served by `express.static`.** So a file stored as `.html` is served as `text/html` from the site's own origin, and a `.svg` is served as `image/svg+xml` — both of which can carry script and would run as same-origin. That is stored cross-site scripting against the storefront. Being accurate about the reach: uploading requires the admin API, which sits behind authentik, so this is not something an anonymous visitor can do today. It needs an admin account — or the gap #63 exists to close, since that boundary is one regex in a proxy config outside this repo and is bypassed by anything reaching the container port directly. So this is defence in depth rather than an open hole, but it is a cheap one to add and the consequence if the boundary ever slips is disproportionate. ## Suggested shape - `accept="image/*"` on the Upload, which is the requested behaviour. - A multer `fileFilter` allowing only the image mime types actually wanted, rejecting anything else with a 400 through the existing error translation. - Derive the stored extension from the accepted type rather than from the client's filename, so the name on disk cannot disagree with what the file is. - Decide explicitly whether SVG is allowed. It is an image and it can contain script, so "images only" does not settle it. Excluding it is the simpler answer for product photos. ## Verification An integration test per rejection — a non-image refused with 400, an accepted image still stored — and a check that the stored filename's extension comes from the type rather than the submitted name. The current tests cover size and count limits but not type, which is why this went unnoticed. ## Severity Low as a convenience, medium as a gap. Nothing is broken, no incident has occurred, and the admin boundary does hold today — but an upload path with no type validation, whose output is served statically from the same origin, is worth closing while it is a five-line change.

bermudalamb added this to the Make the password-reset email editable from Admin project 2026-08-21 16:09:25 -05:00

bermudalamb self-assigned this 2026-08-21 16:09:37 -05:00

bermudalamb referenced this issue

2026-08-21 16:24:20 -05:00

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

bermudalamb commented

2026-08-21 16:40:36 -05:00

Implemented on `feature/95-upload-type-validation` — one commit, not pushed

Decisions this issue left open

JPEG, PNG, WebP. SVG excluded deliberately despite being an image — it executes script when navigated to directly, which is the exposure #103 describes, and a photograph of a one-of-a-kind item is never a vector drawing. GIF excluded as simply not wanted for product stills; adding it later means adding its signature too.

Both the declared type and the bytes are checked, not just the type. file.mimetype is whatever the caller wrote in the multipart headers, so an allowlist on it alone stops honest mistakes and nothing else. The byte check is what stops evil.html renamed to photo.jpg and declared image/jpeg.

The implementation detail worth knowing

The byte check cannot live in fileFilter. That runs before multer has read the stream — there are no bytes to look at yet. So it runs after the write, and a failure removes every file from the request rather than only the offending one: accepting the good half of a refused upload would leave files on the volume that nothing references. Handles are closed before anything is unlinked, because an open handle makes the unlink fail on Windows.

The stored name now takes its extension from the validated type instead of path.extname(file.originalname), so the name on disk cannot disagree with what the file is. The random UUID is untouched — that was already right.

The picker

accept="image/jpeg,image/png,image/webp", not image/*. Offering a type the API will refuse turns a picker choice into a 400 the admin has to decode. Your requested "all files" secondary is unaffected — that is the operating system's own option, which accept never removes, and which is exactly why this is a convenience rather than a control.

Verification

Nine integration tests, and they are the first in this project to upload real file content — which is very likely why none of this was noticed. UPLOADS_DIR was configured for the integration suite but had never needed to exist, because no test had ever attached a file.

Case	Result
A genuine 1x1 PNG	accepted, stored as `.png`
PNG named `evil.html`	accepted, stored as `.png` — the name cannot lie
`application/pdf`	400, naming what is allowed
`image/svg+xml`	400
HTML bytes declared `image/jpeg`	400, "does not contain"
After a content refusal	nothing left on the volume
Valid + invalid in one request	both discarded
After any refusal	no item created

Plus 21 unit tests on the pure signature checks, including a RIFF container that is not WebP — a four-byte check would pass a .wav.

162 unit, 178 integration, 94 end-to-end on a fresh container. Backend lint holds at 4 warnings, having caught the now-unused path import.

What this does not do

It does not make uploads safe to serve from the app's origin — it makes them much less likely to be dangerous. #103 is still the structural fix, and this change is what makes it defence in depth rather than the only thing standing between a stored file and same-origin script.

Not pushed, per the usual arrangement.

## Implemented on `feature/95-upload-type-validation` — one commit, not pushed ## Decisions this issue left open **JPEG, PNG, WebP.** SVG excluded deliberately despite being an image — it executes script when navigated to directly, which is the exposure #103 describes, and a photograph of a one-of-a-kind item is never a vector drawing. GIF excluded as simply not wanted for product stills; adding it later means adding its signature too. **Both the declared type and the bytes are checked**, not just the type. `file.mimetype` is whatever the caller wrote in the multipart headers, so an allowlist on it alone stops honest mistakes and nothing else. The byte check is what stops `evil.html` renamed to `photo.jpg` and declared `image/jpeg`. ## The implementation detail worth knowing **The byte check cannot live in `fileFilter`.** That runs before multer has read the stream — there are no bytes to look at yet. So it runs after the write, and a failure removes **every** file from the request rather than only the offending one: accepting the good half of a refused upload would leave files on the volume that nothing references. Handles are closed before anything is unlinked, because an open handle makes the unlink fail on Windows. The stored name now takes its extension from the validated type instead of `path.extname(file.originalname)`, so the name on disk cannot disagree with what the file is. The random UUID is untouched — that was already right. ## The picker `accept="image/jpeg,image/png,image/webp"`, not `image/*`. Offering a type the API will refuse turns a picker choice into a 400 the admin has to decode. Your requested "all files" secondary is unaffected — that is the operating system's own option, which `accept` never removes, and which is exactly why this is a convenience rather than a control. ## Verification **Nine integration tests, and they are the first in this project to upload real file content** — which is very likely why none of this was noticed. `UPLOADS_DIR` was configured for the integration suite but had never needed to exist, because no test had ever attached a file. | Case | Result | | --- | --- | | A genuine 1x1 PNG | accepted, stored as `.png` | | PNG named `evil.html` | accepted, stored as `.png` — the name cannot lie | | `application/pdf` | 400, naming what is allowed | | `image/svg+xml` | 400 | | HTML bytes declared `image/jpeg` | 400, "does not contain" | | After a content refusal | nothing left on the volume | | Valid + invalid in one request | both discarded | | After any refusal | no item created | Plus **21 unit tests** on the pure signature checks, including a RIFF container that is not WebP — a four-byte check would pass a `.wav`. **162 unit, 178 integration, 94 end-to-end** on a fresh container. Backend lint holds at 4 warnings, having caught the now-unused `path` import. ## What this does not do It does not make uploads safe to serve from the app's origin — it makes them much less likely to be dangerous. **#103 is still the structural fix**, and this change is what makes it defence in depth rather than the only thing standing between a stored file and same-origin script. Not pushed, per the usual arrangement.

bermudalamb referenced this issue from a commit

2026-08-21 16:43:37 -05:00

feat(backend): accept only real images in the inventory upload (#95)

bermudalamb referenced this issue

2026-08-21 16:44:43 -05:00

feat(backend): accept only real images in the inventory upload (#95) #104

bermudalamb added reference feature/95-upload-type-validation

2026-08-21 16:45:23 -05:00

bermudalamb referenced this issue from a commit

2026-08-21 16:46:07 -05:00

Merge pull request 'feat(backend): accept only real images in the inventory upload (#95)' (#104) from feature/95-upload-type-validation into main

bermudalamb moved this to Review in Make the password-reset email editable from Admin on 2026-08-21 16:47:33 -05:00