fix(security): let cors() own Access-Control-Allow-Origin on protected images (#1118)

Closes #1116.

secureImageMiddleware set its own Access-Control-Allow-Origin, overwriting the
one cors(corsOptions) had already computed. server.js:247 mounts cors() on all
of /api with credentials:true, so by the time the route handler ran the correct
header was already there — and the local assignment replaced it with a worse
answer in BOTH directions:

  unresolved -> '*'. Combined with the credentials:true that cors() sets, that
    is an invalid pair browsers reject outright. Unreachable on Docker until
    #1104 stopped compose injecting FRONTEND_URL; reachable on a fresh install
    from then until the wizard stores general_site_url.

  resolved   -> the frontend origin, even when the request legitimately came
    from the allowlisted ADMIN_URL. A split admin host got a header naming the
    wrong origin and the browser rejected a request cors() had allowed.

Deleting the line fixes both. cors() already validates the request Origin
against the allowlist, sets Vary: Origin, omits the header entirely for a
disallowed or absent Origin, and pairs correctly with credentials. Methods,
Headers and Max-Age stay here: they are route-specific and cors() does not
contradict them.

Observed against a running instance before and after:

  allowlisted Origin    ACAO: <that origin> + Vary: Origin + credentials:true
  disallowed Origin     no ACAO
  no Origin header      no ACAO

Six tests, mounted on a real Express app with server.js's middleware order.
Deliberately NOT a unit test against a response double: the first version of
this fix was a guarded assignment that looked correct in isolation and still
overwrote cors() whenever an origin resolved. A double cannot see middleware
composition, which is exactly how that slipped through.

Mutation-checked both ways — restoring the original `|| '*'` fails 5 of 6, and
restoring the guarded assignment fails 3 of 6 including the admin-origin case.
This commit is contained in:
Paul Nothaft
2026-08-21 19:25:56 +02:00
committed by GitHub
parent d241919604
commit 00776234fd
2 changed files with 112 additions and 6 deletions
@@ -2,7 +2,6 @@ const { db } = require('../database/db');
const secureImageService = require('../services/secureImageService');
const logger = require('../utils/logger');
const { formatBoolean } = require('../utils/dbCompat');
const { getFrontendBaseUrlSync } = require('../utils/frontendUrl');
/**
* Enhanced secure image middleware with comprehensive protection
@@ -272,11 +271,18 @@ class SecureImageMiddleware {
'X-Protected-Content': 'true',
'X-Download-Policy': 'restricted',
// CORS restrictions
// Deliberately NOT derived from the request: reflecting the caller's
// Origin would defeat the allowlist. Env, else the configured
// general_site_url (cached), else the previous wildcard behaviour.
'Access-Control-Allow-Origin': getFrontendBaseUrlSync() || '*',
// Access-Control-Allow-ORIGIN is deliberately absent (#1116).
// cors(corsOptions) already runs on all of /api (server.js:247) and owns
// the whole policy: it validates the request Origin against the
// allowlist, sets Vary: Origin, and pairs with credentials:true. Setting
// the header again here only overwrote that with a worse answer —
// unresolved -> '*', which combined with the credentials:true from
// cors() is an invalid pair every browser rejects outright
// resolved -> the frontend origin, even when the request legitimately
// came from the allowlisted ADMIN_URL, so a split admin host got a
// header for the wrong origin and the read failed
// Methods/Headers/Max-Age stay: they are route-specific and cors() does
// not contradict them.
'Access-Control-Allow-Methods': 'GET',
'Access-Control-Allow-Headers': 'Authorization, Content-Type',
'Access-Control-Max-Age': '3600'