fix(admin): serve videos with their real MIME type in the admin photo view (#908) (stable) (#911)

* fix(admin): serve videos with their real MIME type in the admin photo view (#908) (stable)

The admin view route built Content-Type from the filename extension —
image/<ext> — which is invalid for videos (image/mp4). The admin player
fetches this URL into a blob that inherits the type, and browsers
refuse to play a <video> blob labeled image/*: blank/grey preview,
while download (which already uses photo.mime_type) worked fine.

Stored mime_type now wins; videos without one fall back to video/mp4,
images to the extension, and extensionless files to image/jpeg instead
of the equally invalid bare 'image/'.

Also unrefs chunkedUploadService's module-level hourly cleanup interval:
it kept Jest from exiting for any suite requiring adminPhotos (it's why
adminPhotos.reference sits on the CI ignore list). Production behavior
unchanged — the HTTP listener keeps the process alive.

New adminPhotoContentType suite pins all four MIME cases.

* fix(admin): harden admin photo Content-Type resolution (#908 review round)

External review findings, all verified:

- The header is now ALWAYS image/* or video/*. photos.mime_type is
  never echoed verbatim unless it is a video/ type — the chunked-upload
  path stores the client-sent MIME unvalidated, so a stored text/html
  served inline under the app origin was a same-origin XSS hazard.
- MIME-less videos map from the extension via the shared
  EXTENSION_TO_MIME (.mov → video/quicktime, .webm → video/webm)
  instead of a blanket video/mp4 that would mislabel them.
- Images ignore the stored MIME entirely: migration 039 backfilled
  image/jpeg onto every legacy row (PNGs included), so trusting it
  would regress previously-correct extension-derived types. Extension
  wins, normalized (jpg → image/jpeg).

Suite extended to 8 MIME cases including the XSS guard and the
039-backfill immunity.

* fix(admin): validate stored video MIME as a full header-safe token (#908 review round 2)

A prefix check let malformed client-stored values through:
'video/mp4\r\nX: y' makes res.setHeader throw ERR_INVALID_CHAR — a
permanent 500 for that photo — and a bare 'video/' is an invalid type.
Strict /^video\/[\w.+-]+$/ now gates the stored value; anything else
falls back to the extension map. Two new tests pin both shapes.

* fix(admin): map-only image Content-Type — no raw extension interpolation (#908 review round 3)

image/${ext} could synthesize image/svg+xml (scriptable when served
inline) or header-invalid values from client-controlled chunked-upload
filenames. The shared EXTENSION_TO_MIME map is now the allowlist on the
image side too; unmapped extensions serve as image/jpeg — browsers
sniff image bytes in img/blob contexts, so a mislabel is harmless where
an injected type is not.

* fix(admin): own-property lookup in the extension MIME map (#908 review round)

A client-controlled filename ending in .constructor / .__proto__ /
.toString made EXTENSION_TO_MIME[ext] return an inherited Object.prototype
member (truthy), and the downstream extMime.startsWith threw —
a permanent 500 on the admin view for that photo instead of the JPEG /
mp4 fallback. hasOwnProperty-gated now; test pins both a .constructor
image and a .__proto__ video.

* fix(admin): honor safe stored image MIME for auto-imported formats (#908 review round 2)

My previous round made the image side map-only to dodge the migration
039 image/jpeg backfill and image/svg+xml — but that regressed the S3
auto-importer (STORAGE_AUTO_IMPORT), which stores correct types for
avif/bmp/tiff/heic whose extensions aren't in EXTENSION_TO_MIME. Those
now served as image/jpeg (JPEG-labelled non-JPEG bytes).

Precedence is now mapped-extension (still corrects the 039 backfill on
PNGs) -> stored MIME IF in a safe raster allowlist (avif/bmp/tiff/heic
+ the mapped ones) -> image/jpeg. Allowlist, not a regex: image/svg+xml
stays excluded (scriptable inline). Tests pin avif preserved and svg
degraded to jpeg.

* fix(admin): allow any header-safe raster MIME, deny svg/xml (#908 review round 3)

The round-2 hand-listed Set kept missing formats the S3 auto-importer
stores (apng/ico/jxl beyond avif/bmp/tiff). Replace it with a regex:
honor image/<token> EXCEPT the scriptable svg / *+xml family. Covers
every current and future raster type in one rule while still blocking
inline-scriptable svg and header injection. Tests pin apng + x-icon
preserved, svg still degraded to jpeg.

---------

Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
2026-07-30 12:14:58 +02:00
committed by GitHub
co-authored by Paul Nothaft
parent 6891769124
commit d68d84e5c8
3 changed files with 303 additions and 3 deletions
@@ -0,0 +1,238 @@
/**
* Admin photo view route Content-Type (#908).
*
* The route built `image/<ext>` from the filename, producing invalid
* types like image/mp4 for videos. AdminAuthenticatedVideo fetches this
* URL into a blob whose type inherits the header, and browsers refuse to
* play a <video> blob labeled image/* — blank/grey admin video preview.
*
* Pins (incl. external-review hardening):
* - the header is ALWAYS image/* or video/*: a stored non-media MIME
* (chunked uploads store the client-sent type unvalidated) is never
* echoed — text/html inline under the app origin would be XSS
* - stored video/ MIME wins; MIME-less videos map from the extension
* (.mov → video/quicktime), unknown video extensions get video/mp4
* - images IGNORE the stored MIME (migration 039 backfilled image/jpeg
* onto every legacy row, PNGs included) and use the extension,
* normalized (jpg → image/jpeg); extensionless files get image/jpeg
*/
const path = require('path');
const fs = require('fs');
const os = require('os');
process.env.NODE_ENV = 'test';
process.env.TEST_DATABASE_PATH = path.join(
fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-admin-ct-')), 'db.sqlite',
);
process.env.JWT_SECRET = process.env.JWT_SECRET || 'admin-ct-test-secret';
process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-admin-ct-storage-'));
const request = require('supertest');
const express = require('express');
const bcrypt = require('bcrypt');
const jwt = require('jsonwebtoken');
const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
const SLUG = 'admin-ct-test-event';
describe('admin photo view Content-Type (#908)', () => {
let db;
let cleanup;
let app;
let eventId;
let adminToken;
const addPhoto = async (filename, extra = {}) => {
const dir = path.join(process.env.STORAGE_PATH, 'events/active', SLUG);
fs.mkdirSync(dir, { recursive: true });
fs.writeFileSync(path.join(dir, filename), Buffer.from(`bytes-${filename}`));
const r = await db('photos').insert({
event_id: eventId,
filename,
path: `${SLUG}/${filename}`,
type: 'individual',
uploaded_at: new Date().toISOString(),
...extra,
}).returning('id');
return r[0]?.id ?? r[0];
};
const getPhotoRes = (photoId) => request(app)
.get(`/api/admin/photos/${eventId}/photo/${photoId}`)
.set('Authorization', `Bearer ${adminToken}`);
beforeAll(async () => {
({ db, cleanup } = await bootCrmDb());
await seedMinimal(db);
const inserted = await db('events').insert({
slug: SLUG,
event_type: 'wedding',
event_name: 'Admin CT Test',
event_date: '2026-08-01',
host_email: '[email protected]',
admin_email: '[email protected]',
password_hash: 'x',
share_link: `/gallery/${SLUG}/share`,
share_token: 'admin-ct-share',
expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(),
is_active: 1,
is_archived: 0,
is_draft: 0,
created_at: new Date().toISOString(),
}).returning('id');
eventId = inserted[0]?.id ?? inserted[0];
const superRole = await db('roles').where({ name: 'super_admin' }).first();
const [rootId] = await db('admin_users').insert({
username: 'admin-ct-admin',
email: '[email protected]',
password_hash: await bcrypt.hash('AdminCt123', 4),
role_id: superRole.id,
is_active: 1,
created_at: new Date(),
updated_at: new Date(),
}).returning('id').then((r) => [r[0]?.id || r[0]]);
adminToken = jwt.sign(
{ id: rootId, username: 'admin-ct-admin', type: 'admin', role: 'super_admin', loginTime: Date.now() },
process.env.JWT_SECRET,
{ expiresIn: '1h', issuer: 'picpeak-auth' }
);
app = express();
app.use(express.json());
app.use('/api/admin/photos', require('../../src/routes/adminPhotos'));
}, 120000);
afterAll(async () => { if (cleanup) await cleanup(); });
it('serves a video with its stored mime_type, not image/<ext>', async () => {
const id = await addPhoto('clip.mp4', { media_type: 'video', mime_type: 'video/mp4' });
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('video/mp4');
});
it('maps MIME-less videos from their extension (.mov → video/quicktime)', async () => {
const id = await addPhoto('clip-nomime.mov', { media_type: 'video' });
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('video/quicktime');
});
it('falls back to video/mp4 for a video with an unknown extension', async () => {
const id = await addPhoto('clip-unknown.xyz', { media_type: 'video' });
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('video/mp4');
});
it('rejects malformed video/ MIME values that would break setHeader', async () => {
// Header-invalid chars in the stored value must not 500 the route —
// fall back to the extension map instead.
const id = await addPhoto('crlf.mp4', {
media_type: 'video',
mime_type: 'video/mp4\r\nX-Evil: 1',
});
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('video/mp4');
expect(res.headers['x-evil']).toBeUndefined();
const bare = await addPhoto('bare.webm', { media_type: 'video', mime_type: 'video/' });
const res2 = await getPhotoRes(bare);
expect(res2.status).toBe(200);
expect(res2.headers['content-type']).toBe('video/webm');
});
it('preserves an auto-imported avif via the safe stored-MIME allowlist', async () => {
// .avif isn't in EXTENSION_TO_MIME; s3AutoImporter stores image/avif.
// Map-only would mislabel it image/jpeg — the allowlist keeps it.
const id = await addPhoto('imported.avif', { mime_type: 'image/avif' });
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('image/avif');
});
it('preserves other importer raster types too (apng, x-icon)', async () => {
const apng = await addPhoto('anim.apng', { mime_type: 'image/apng' });
expect((await getPhotoRes(apng)).headers['content-type']).toBe('image/apng');
const ico = await addPhoto('fav.ico', { mime_type: 'image/x-icon' });
expect((await getPhotoRes(ico)).headers['content-type']).toBe('image/x-icon');
});
it('does NOT honor a stored scriptable image type (image/svg+xml)', async () => {
// svg is inline-scriptable and must never be echoed — allowlist excludes it.
const id = await addPhoto('vector.svg', { mime_type: 'image/svg+xml' });
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('image/jpeg');
});
it('never echoes a stored non-media MIME type (inline XSS guard)', async () => {
const id = await addPhoto('evil.png', { mime_type: 'text/html' });
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('image/png');
});
it('ignores the migration-039 image/jpeg backfill on legacy PNG rows', async () => {
const id = await addPhoto('legacy.png', { mime_type: 'image/jpeg' });
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('image/png');
});
it('normalizes jpg to the canonical image/jpeg', async () => {
const id = await addPhoto('shot.jpg');
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('image/jpeg');
});
it('keeps the extension fallback for images without a stored mime_type', async () => {
const id = await addPhoto('shot.png');
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('image/png');
});
it('handles Object.prototype key extensions without a 500 (.constructor)', async () => {
// The extension-to-MIME lookup must be own-property only — a raw
// index access returns an inherited function for these keys and the
// downstream startsWith throws. Serve image/jpeg instead of 500.
const id = await addPhoto('payload.constructor');
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('image/jpeg');
const id2 = await addPhoto('payload.__proto__', { media_type: 'video' });
const res2 = await getPhotoRes(id2);
expect(res2.status).toBe(200);
expect(res2.headers['content-type']).toBe('video/mp4');
});
it('does not synthesize types from unmapped image extensions', async () => {
// Raw interpolation would produce image/svg+xml (scriptable inline)
// or arbitrary strings from client-controlled filenames — the shared
// map is the allowlist, everything else is served as image/jpeg.
const svg = await addPhoto('vector.svg+xml');
const res = await getPhotoRes(svg);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('image/jpeg');
const weird = await addPhoto('weird.xyz');
const res2 = await getPhotoRes(weird);
expect(res2.status).toBe(200);
expect(res2.headers['content-type']).toBe('image/jpeg');
});
it('extensionless files get image/jpeg, never a bare image/', async () => {
const id = await addPhoto('noext');
const res = await getPhotoRes(id);
expect(res.status).toBe(200);
expect(res.headers['content-type']).toBe('image/jpeg');
});
});