feat(security): stop the app origin serving anything it does not recognise, and make the uploads origin configurable (#103)
The uploads directory is the only place in this application where content someone else authored is served over HTTP. #95 stopped a dangerous file being stored; this stops a stored file doing damage if one ever gets there anyway — through a gap, a path added later, a restore, or a file written before that validation existed. Two halves, complementary rather than alternative. The app's own origin now serves uploads defensively. An allowlist of the three extensions the upload path can produce, so a `.html` or a `.svg` on disk is simply not a file this application hands out — 404, the same answer as a file that is not there, so the response cannot be used to learn which paths exist. An allowlist rather than a denylist because a denylist has to anticipate every type a browser might execute, which is a moving target across browsers and years, while this only has to know three. The content type is stated explicitly from that same list rather than sniffed or guessed from a name someone else chose, paired with `nosniff`. `default-src 'none'; sandbox` gives a directly-navigated file no capabilities at all, which is the only way one of these can do harm — an `<img>` embed does not execute script. Writes get 405 rather than falling through to a 404 that suggests the path is wrong. The other half is the separate origin, which is the real fix, because the origin is the whole unit of trust in a browser. That needs a hostname and a certificate, which live outside this repository, so what is here is the switch: `UPLOADS_BASE_URL`, sent to the frontend at runtime through `/api/config` and joined onto stored paths by `uploadUrl`. Empty means the app's own origin, which is the default and what local development has, so nothing changes until it is pointed somewhere. Stored paths stay site-relative. A stored value outlives any hostname baked into it, and rewriting them would be a migration to undo the day the hostname changes. Runtime rather than built in, so one image serves every environment — the same reason `paypalClientId` and `demoMode` are already there. `UPLOADS_BASE_URL` has a line in `docker-compose.prod.yml` while still empty, deliberately: a Portainer 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. Unset warns at boot, in the same shape as the admin gate — a working configuration with one defence switched off is worth saying out loud. Set without a scheme is refused outright, because a bare hostname joins onto a stored path as if it were relative and breaks every image on the site rather than failing visibly. The compose guard now resolves `${VAR:-default}` to its default, which is what the container actually receives when the stack variable behind it is unset. A bare `${VAR}` is still left opaque, so a required variable referenced that way goes on counting as present — that check is about the line existing, not about the stack being filled in. Closes #103
This commit is contained in:
+22
-2
@@ -18,6 +18,7 @@ import clientErrorsRouter from './routes/clientErrors';
|
|||||||
import { attachCustomer } from './middleware/customerAuth';
|
import { attachCustomer } from './middleware/customerAuth';
|
||||||
import { requireAdminGate } from './middleware/adminGate';
|
import { requireAdminGate } from './middleware/adminGate';
|
||||||
import { asyncRoute } from './asyncRoute';
|
import { asyncRoute } from './asyncRoute';
|
||||||
|
import { uploadsRouter } from './uploads';
|
||||||
|
|
||||||
const app = express();
|
const app = express();
|
||||||
// Express advertises itself in X-Powered-By by default, which hands an
|
// Express advertises itself in X-Powered-By by default, which hands an
|
||||||
@@ -32,7 +33,14 @@ app.use(cookieParser());
|
|||||||
// globally an unforwarded rejection here would hang every request in the app —
|
// globally an unforwarded rejection here would hang every request in the app —
|
||||||
// including the routes that wrap their own handlers correctly.
|
// including the routes that wrap their own handlers correctly.
|
||||||
app.use(asyncRoute(attachCustomer));
|
app.use(asyncRoute(attachCustomer));
|
||||||
app.use('/uploads', express.static(process.env.UPLOADS_DIR || '/app/uploads'));
|
app.use('/uploads', uploadsRouter(process.env.UPLOADS_DIR || '/app/uploads'));
|
||||||
|
|
||||||
|
// Trimmed with a loop rather than a `/+$/` regex, which backtracks.
|
||||||
|
function trimTrailingSlashes(value: string): string {
|
||||||
|
let trimmed = value;
|
||||||
|
while (trimmed.endsWith('/')) trimmed = trimmed.slice(0, -1);
|
||||||
|
return trimmed;
|
||||||
|
}
|
||||||
|
|
||||||
app.get('/api/config', (_req, res) => {
|
app.get('/api/config', (_req, res) => {
|
||||||
const clientId = process.env.PAYPAL_CLIENT_ID;
|
const clientId = process.env.PAYPAL_CLIENT_ID;
|
||||||
@@ -40,7 +48,19 @@ app.get('/api/config', (_req, res) => {
|
|||||||
res.json({
|
res.json({
|
||||||
paypalClientId: isPlaceholder ? null : clientId,
|
paypalClientId: isPlaceholder ? null : clientId,
|
||||||
demoMode: process.env.DEMO_MODE !== 'false',
|
demoMode: process.env.DEMO_MODE !== 'false',
|
||||||
currency: process.env.SITE_CURRENCY || 'USD'
|
currency: process.env.SITE_CURRENCY || 'USD',
|
||||||
|
// Where uploaded images should be fetched from (#103). Empty means the
|
||||||
|
// app's own origin, which is both the default and what local development
|
||||||
|
// has — there is no second hostname on a laptop. Set it to a hostname of
|
||||||
|
// its own in production and user-supplied files stop sharing an origin with
|
||||||
|
// the application, which is the whole unit of trust in a browser.
|
||||||
|
//
|
||||||
|
// Sent at runtime rather than built in, so one image serves every
|
||||||
|
// environment, the same reason paypalClientId and demoMode are here.
|
||||||
|
//
|
||||||
|
// Trailing slash trimmed so callers can join with a stored path, which
|
||||||
|
// always begins with one, without producing a double.
|
||||||
|
uploadsBaseUrl: trimTrailingSlashes(process.env.UPLOADS_BASE_URL ?? '')
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -162,16 +162,49 @@ function checkAdminGate(env: NodeJS.ProcessEnv): string[] {
|
|||||||
];
|
];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Optional, and the same shape as the admin gate above: unset is a working
|
||||||
|
// configuration with one defence switched off, which is worth saying out loud
|
||||||
|
// rather than leaving to be discovered. Set, it has to be an absolute origin —
|
||||||
|
// a value missing its scheme joins into a relative path and silently breaks
|
||||||
|
// every image on the site, which is a worse outcome than either extreme.
|
||||||
|
function checkUploadsOrigin(env: NodeJS.ProcessEnv): EnvValidation {
|
||||||
|
if (!isPresent(env, 'UPLOADS_BASE_URL')) {
|
||||||
|
return {
|
||||||
|
errors: [],
|
||||||
|
warnings: [
|
||||||
|
'UPLOADS_BASE_URL is not set — uploaded files are served from this application on its ' +
|
||||||
|
'own origin, so anything reaching the uploads directory shares an origin with the site.'
|
||||||
|
]
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
const value = (env.UPLOADS_BASE_URL ?? '').trim();
|
||||||
|
if (!value.startsWith('https://') && !value.startsWith('http://')) {
|
||||||
|
return {
|
||||||
|
errors: [
|
||||||
|
'UPLOADS_BASE_URL must be an absolute origin including the scheme, such as ' +
|
||||||
|
'https://uploads.example.com. Without one it joins into a relative path and every ' +
|
||||||
|
'image on the site breaks.'
|
||||||
|
],
|
||||||
|
warnings: []
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
return { errors: [], warnings: [] };
|
||||||
|
}
|
||||||
|
|
||||||
export function validateEnv(env: NodeJS.ProcessEnv): EnvValidation {
|
export function validateEnv(env: NodeJS.ProcessEnv): EnvValidation {
|
||||||
const mail = checkMail(env);
|
const mail = checkMail(env);
|
||||||
|
const uploads = checkUploadsOrigin(env);
|
||||||
|
|
||||||
return {
|
return {
|
||||||
errors: [
|
errors: [
|
||||||
...checkAlwaysRequired(env),
|
...checkAlwaysRequired(env),
|
||||||
...checkDemoMode(env),
|
...checkDemoMode(env),
|
||||||
...checkPayPal(env),
|
...checkPayPal(env),
|
||||||
...mail.errors
|
...mail.errors,
|
||||||
|
...uploads.errors
|
||||||
],
|
],
|
||||||
warnings: [...mail.warnings, ...checkAdminGate(env)]
|
warnings: [...mail.warnings, ...checkAdminGate(env), ...uploads.warnings]
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -37,6 +37,22 @@ const EXTENSION_FOR_TYPE: Readonly<Record<string, string>> = {
|
|||||||
'image/webp': '.webp'
|
'image/webp': '.webp'
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The content type a stored file should be served as, from its extension.
|
||||||
|
*
|
||||||
|
* The inverse of `extensionFor`, and derived from the same record so the two
|
||||||
|
* cannot drift. Returns null for anything else, which is what lets the uploads
|
||||||
|
* route refuse to serve a file it does not recognise — the case that matters is
|
||||||
|
* a file written before this validation existed, or one that arrived through a
|
||||||
|
* gap, since nothing the current upload path accepts can produce another
|
||||||
|
* extension.
|
||||||
|
*/
|
||||||
|
export function typeForExtension(extension: string): string | null {
|
||||||
|
const lowered = extension.toLowerCase();
|
||||||
|
const found = Object.entries(EXTENSION_FOR_TYPE).find(([, ext]) => ext === lowered);
|
||||||
|
return found?.[0] ?? null;
|
||||||
|
}
|
||||||
|
|
||||||
export function isAllowedImageType(mimetype: string): boolean {
|
export function isAllowedImageType(mimetype: string): boolean {
|
||||||
return ALLOWED_IMAGE_TYPES.includes(mimetype);
|
return ALLOWED_IMAGE_TYPES.includes(mimetype);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -0,0 +1,104 @@
|
|||||||
|
/**
|
||||||
|
* Serving user-uploaded files, defensively.
|
||||||
|
*
|
||||||
|
* Everything under the uploads directory was put there by someone other than
|
||||||
|
* the people who wrote this application, and it is served over HTTP. #95 stops
|
||||||
|
* a dangerous file being *stored*; this stops a stored file *doing damage* if
|
||||||
|
* one ever gets there anyway — through a gap, a path added later, or a file
|
||||||
|
* written before that validation existed.
|
||||||
|
*
|
||||||
|
* The real fix for that class is a separate origin, because the origin is the
|
||||||
|
* whole unit of trust in a browser (#103). That needs a hostname and a
|
||||||
|
* certificate, which live outside this repository, so what is here is the half
|
||||||
|
* that works either way: the app's own origin stops serving anything it does
|
||||||
|
* not recognise, and serves what it does recognise in a form that cannot be
|
||||||
|
* talked into executing.
|
||||||
|
*
|
||||||
|
* These two are complementary rather than alternatives. The separate origin
|
||||||
|
* still points at these same files, so the rules below apply there too.
|
||||||
|
*/
|
||||||
|
|
||||||
|
import express, { Request, Response, NextFunction, Router } from 'express';
|
||||||
|
import path from 'path';
|
||||||
|
import { typeForExtension } from './uploadTypes';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A directly-navigated upload gets no capabilities at all.
|
||||||
|
*
|
||||||
|
* `default-src 'none'` leaves a document unable to load or run anything, and
|
||||||
|
* `sandbox` with no allowances drops it into an opaque origin, so even a file
|
||||||
|
* that somehow renders as markup cannot reach the site's cookies or DOM.
|
||||||
|
*
|
||||||
|
* This does nothing to an `<img>` embed, which is the only way these files are
|
||||||
|
* legitimately used — a policy on an image response constrains the image's own
|
||||||
|
* (nonexistent) subresource loads, not the page displaying it.
|
||||||
|
*/
|
||||||
|
const UPLOAD_CSP = "default-src 'none'; sandbox";
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Whether a request should reach the files at all.
|
||||||
|
*
|
||||||
|
* Only GET and HEAD: express.static ignores the rest anyway, but answering 405
|
||||||
|
* says so rather than falling through to a 404 that suggests the path is wrong.
|
||||||
|
*/
|
||||||
|
function methodAllowed(method: string): boolean {
|
||||||
|
return method === 'GET' || method === 'HEAD';
|
||||||
|
}
|
||||||
|
|
||||||
|
export function uploadsRouter(directory: string): Router {
|
||||||
|
const router = express.Router();
|
||||||
|
|
||||||
|
router.use((req: Request, res: Response, next: NextFunction) => {
|
||||||
|
if (!methodAllowed(req.method)) {
|
||||||
|
res.set('Allow', 'GET, HEAD');
|
||||||
|
res.status(405).json({ error: 'method not allowed' });
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
// An allowlist rather than a denylist of dangerous extensions. A denylist
|
||||||
|
// has to anticipate every type a browser might execute, which is a moving
|
||||||
|
// target across browsers and years; this only has to know the three types
|
||||||
|
// the upload path can produce, and everything else — including a `.html` or
|
||||||
|
// a `.svg` sitting on disk from before there was any validation — is simply
|
||||||
|
// not a file this application will hand out.
|
||||||
|
const contentType = typeForExtension(path.extname(req.path));
|
||||||
|
if (contentType === null) {
|
||||||
|
// 404 rather than 403: whether a file exists at that path is not
|
||||||
|
// something a stranger needs to be able to distinguish.
|
||||||
|
res.status(404).json({ error: 'not found' });
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Set here rather than only in setHeaders below, so a request that never
|
||||||
|
// reaches a file still carries them.
|
||||||
|
res.set('X-Content-Type-Options', 'nosniff');
|
||||||
|
res.set('Content-Security-Policy', UPLOAD_CSP);
|
||||||
|
next();
|
||||||
|
});
|
||||||
|
|
||||||
|
router.use(
|
||||||
|
express.static(directory, {
|
||||||
|
// No directory listings and no index.html, both of which would be content
|
||||||
|
// this application did not write being served as if it had.
|
||||||
|
index: false,
|
||||||
|
// A dotfile in an upload directory is never something to hand out.
|
||||||
|
dotfiles: 'ignore',
|
||||||
|
setHeaders: (res: Response, filePath: string) => {
|
||||||
|
const contentType = typeForExtension(path.extname(filePath));
|
||||||
|
if (contentType !== null) {
|
||||||
|
// Stated explicitly rather than left to express.static's extension
|
||||||
|
// lookup. Paired with nosniff, the type a browser sees is then the
|
||||||
|
// one this application chose, from a list of three, and never a guess
|
||||||
|
// made from the bytes.
|
||||||
|
res.set('Content-Type', contentType);
|
||||||
|
}
|
||||||
|
// Required once these are served from a hostname of their own: without
|
||||||
|
// it a resource-policy-conscious browser refuses the cross-origin
|
||||||
|
// `<img>` load. Harmless while the origin is shared.
|
||||||
|
res.set('Cross-Origin-Resource-Policy', 'cross-origin');
|
||||||
|
}
|
||||||
|
})
|
||||||
|
);
|
||||||
|
|
||||||
|
return router;
|
||||||
|
}
|
||||||
@@ -0,0 +1,145 @@
|
|||||||
|
import request from 'supertest';
|
||||||
|
import { promises as fs } from 'fs';
|
||||||
|
import path from 'path';
|
||||||
|
import app from '../../src/app';
|
||||||
|
import { pool } from '../../src/db';
|
||||||
|
import { closeDb } from './setup/testDb';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What the application will and will not hand back out of the uploads
|
||||||
|
* directory.
|
||||||
|
*
|
||||||
|
* #95 covers what can be *stored*; this covers what can be *served*, which is a
|
||||||
|
* separate question because a file can reach that directory without going
|
||||||
|
* through the upload route — one written before the validation existed, one
|
||||||
|
* restored from a backup, one put there by a path added later. The uploads
|
||||||
|
* directory is the only place in this application where content someone else
|
||||||
|
* authored is served over HTTP, so it gets its own rules (#103).
|
||||||
|
*
|
||||||
|
* The files here are written straight to disk rather than uploaded, precisely
|
||||||
|
* because the interesting cases are the ones the upload route would refuse.
|
||||||
|
*/
|
||||||
|
|
||||||
|
const UPLOADS_DIR = process.env.UPLOADS_DIR as string;
|
||||||
|
|
||||||
|
const REAL_PNG = Buffer.from(
|
||||||
|
'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
|
||||||
|
'base64'
|
||||||
|
);
|
||||||
|
|
||||||
|
// What a stored cross-site scripting payload looks like: same-origin markup
|
||||||
|
// carrying script, reachable by navigating straight to it.
|
||||||
|
const HOSTILE_HTML = Buffer.from('<!doctype html><script>alert(document.cookie)</script>', 'utf8');
|
||||||
|
const HOSTILE_SVG = Buffer.from(
|
||||||
|
'<svg xmlns="http://www.w3.org/2000/svg"><script>alert(document.cookie)</script></svg>',
|
||||||
|
'utf8'
|
||||||
|
);
|
||||||
|
|
||||||
|
const written: string[] = [];
|
||||||
|
|
||||||
|
async function place(name: string, bytes: Buffer): Promise<string> {
|
||||||
|
await fs.writeFile(path.join(UPLOADS_DIR, name), bytes);
|
||||||
|
written.push(name);
|
||||||
|
return `/uploads/${name}`;
|
||||||
|
}
|
||||||
|
|
||||||
|
beforeAll(async () => {
|
||||||
|
await fs.mkdir(UPLOADS_DIR, { recursive: true });
|
||||||
|
});
|
||||||
|
|
||||||
|
afterAll(async () => {
|
||||||
|
await Promise.all(
|
||||||
|
written.map((name) => fs.rm(path.join(UPLOADS_DIR, name), { force: true }))
|
||||||
|
);
|
||||||
|
await pool.end();
|
||||||
|
await closeDb();
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('serving uploaded files', () => {
|
||||||
|
it('serves a stored image', async () => {
|
||||||
|
const url = await place('serving-ok.png', REAL_PNG);
|
||||||
|
const res = await request(app).get(url);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body).toEqual(REAL_PNG);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Stated by this application from a list of three, rather than sniffed from
|
||||||
|
// the bytes or guessed from a name someone else chose.
|
||||||
|
it('states the content type explicitly', async () => {
|
||||||
|
const url = await place('serving-type.png', REAL_PNG);
|
||||||
|
const res = await request(app).get(url);
|
||||||
|
expect(res.headers['content-type']).toContain('image/png');
|
||||||
|
expect(res.headers['x-content-type-options']).toBe('nosniff');
|
||||||
|
});
|
||||||
|
|
||||||
|
// Not a header for the image's own sake — an image loads no subresources.
|
||||||
|
// It constrains the document a browser makes when someone navigates directly
|
||||||
|
// to the file, which is the only way one of these can do harm.
|
||||||
|
it('sends a policy that leaves a directly-navigated file with no capabilities', async () => {
|
||||||
|
const url = await place('serving-csp.png', REAL_PNG);
|
||||||
|
const res = await request(app).get(url);
|
||||||
|
expect(res.headers['content-security-policy']).toContain("default-src 'none'");
|
||||||
|
expect(res.headers['content-security-policy']).toContain('sandbox');
|
||||||
|
});
|
||||||
|
|
||||||
|
// Needed the moment these are served from a hostname of their own, or a
|
||||||
|
// browser refuses the cross-origin <img> load.
|
||||||
|
it('allows the file to be embedded cross-origin', async () => {
|
||||||
|
const url = await place('serving-corp.png', REAL_PNG);
|
||||||
|
const res = await request(app).get(url);
|
||||||
|
expect(res.headers['cross-origin-resource-policy']).toBe('cross-origin');
|
||||||
|
});
|
||||||
|
|
||||||
|
// The case the whole file exists for. Both of these execute script when
|
||||||
|
// navigated to, and neither can be produced by the upload route — so a file
|
||||||
|
// like this on disk means an earlier control failed, and this is the one that
|
||||||
|
// still holds.
|
||||||
|
it('refuses to serve markup that would run as the site', async () => {
|
||||||
|
const html = await place('serving-hostile.html', HOSTILE_HTML);
|
||||||
|
const svg = await place('serving-hostile.svg', HOSTILE_SVG);
|
||||||
|
|
||||||
|
expect((await request(app).get(html)).status).toBe(404);
|
||||||
|
expect((await request(app).get(svg)).status).toBe(404);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses an extension it does not recognise, whatever the file contains', async () => {
|
||||||
|
// Genuinely a PNG, and still not served: the rule is about the name the
|
||||||
|
// browser will judge the response by, not about the bytes.
|
||||||
|
const url = await place('serving-mislabelled.txt', REAL_PNG);
|
||||||
|
expect((await request(app).get(url)).status).toBe(404);
|
||||||
|
});
|
||||||
|
|
||||||
|
// 404 rather than 403, so a stranger cannot use the response to learn which
|
||||||
|
// paths exist.
|
||||||
|
it('answers the same for a refused type as for a file that is not there', async () => {
|
||||||
|
const missing = await request(app).get('/uploads/serving-absent.png');
|
||||||
|
const refused = await request(app).get('/uploads/serving-absent.html');
|
||||||
|
expect(missing.status).toBe(404);
|
||||||
|
expect(refused.status).toBe(404);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('accepts an extension in any case', async () => {
|
||||||
|
const url = await place('serving-shouted.PNG', REAL_PNG);
|
||||||
|
const res = await request(app).get(url);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.headers['content-type']).toContain('image/png');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses to be written to', async () => {
|
||||||
|
const res = await request(app).post('/uploads/serving-ok.png').send('anything');
|
||||||
|
expect(res.status).toBe(405);
|
||||||
|
expect(res.headers.allow).toBe('GET, HEAD');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('GET /api/config', () => {
|
||||||
|
// Runtime rather than built in, so one image serves every environment. Empty
|
||||||
|
// is the default and means the app's own origin, which is what local
|
||||||
|
// development has.
|
||||||
|
it('names the origin uploaded images should be fetched from', async () => {
|
||||||
|
const res = await request(app).get('/api/config');
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body).toHaveProperty('uploadsBaseUrl');
|
||||||
|
expect(typeof res.body.uploadsBaseUrl).toBe('string');
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -63,6 +63,21 @@ const DEPLOYMENTS = [
|
|||||||
}
|
}
|
||||||
] as const;
|
] as const;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What the container receives for a `${VAR:-default}` entry when the stack
|
||||||
|
* variable behind it is unset — which is the case worth modelling, since a
|
||||||
|
* variable that is always set needs no default.
|
||||||
|
*
|
||||||
|
* A bare `${VAR}` is deliberately left alone. It stays an opaque non-empty
|
||||||
|
* string so that a required variable referenced that way still counts as
|
||||||
|
* present, which is the whole contract described below: what is checked here is
|
||||||
|
* that the *line exists*, not that the stack behind it is filled in.
|
||||||
|
*/
|
||||||
|
function resolveDefault(value: string): string {
|
||||||
|
const withDefault = /^\$\{[A-Z_0-9]+:-(.*)\}$/.exec(value);
|
||||||
|
return withDefault?.[1] ?? value;
|
||||||
|
}
|
||||||
|
|
||||||
// Only real environment entries — `- NAME=value` at an indented list position.
|
// Only real environment entries — `- NAME=value` at an indented list position.
|
||||||
// A mention inside a comment cannot match, because a comment line starts with #.
|
// A mention inside a comment cannot match, because a comment line starts with #.
|
||||||
function environmentEntries(source: string): Map<string, string> {
|
function environmentEntries(source: string): Map<string, string> {
|
||||||
@@ -79,7 +94,7 @@ function environmentEntries(source: string): Map<string, string> {
|
|||||||
// but RegExpExecArray cannot say so, and #101 made the compiler insist.
|
// but RegExpExecArray cannot say so, and #101 made the compiler insist.
|
||||||
const [, name, value] = match ?? [];
|
const [, name, value] = match ?? [];
|
||||||
if (name !== undefined && value !== undefined) {
|
if (name !== undefined && value !== undefined) {
|
||||||
entries.set(name, value.trim());
|
entries.set(name, resolveDefault(value.trim()));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return entries;
|
return entries;
|
||||||
|
|||||||
@@ -187,3 +187,32 @@ describe('validateEnv', () => {
|
|||||||
expect(errors.some((e) => e.includes('UPLOADS_DIR'))).toBe(true);
|
expect(errors.some((e) => e.includes('UPLOADS_DIR'))).toBe(true);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #103. Optional, like the admin gate: unset is a working configuration with
|
||||||
|
// one defence switched off, and set-but-wrong is worse than either.
|
||||||
|
describe('UPLOADS_BASE_URL', () => {
|
||||||
|
it('warns when it is unset, since the isolation is simply off', () => {
|
||||||
|
const { errors, warnings } = validateEnv(MINIMAL);
|
||||||
|
expect(errors).not.toContainEqual(expect.stringContaining('UPLOADS_BASE_URL'));
|
||||||
|
expect(warnings).toContainEqual(expect.stringContaining('UPLOADS_BASE_URL is not set'));
|
||||||
|
});
|
||||||
|
|
||||||
|
it('is satisfied by an absolute origin', () => {
|
||||||
|
const { errors, warnings } = validateEnv(withEnv({ UPLOADS_BASE_URL: 'https://uploads.example.com' }));
|
||||||
|
expect(errors).not.toContainEqual(expect.stringContaining('UPLOADS_BASE_URL'));
|
||||||
|
expect(warnings).not.toContainEqual(expect.stringContaining('UPLOADS_BASE_URL'));
|
||||||
|
});
|
||||||
|
|
||||||
|
// A hostname with no scheme joins onto a stored path as if it were relative,
|
||||||
|
// which breaks every image on the site rather than failing visibly. Refusing
|
||||||
|
// to start is the kinder outcome.
|
||||||
|
it('refuses a value with no scheme', () => {
|
||||||
|
const { errors } = validateEnv(withEnv({ UPLOADS_BASE_URL: 'uploads.example.com' }));
|
||||||
|
expect(errors).toContainEqual(expect.stringContaining('absolute origin'));
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses a path rather than an origin', () => {
|
||||||
|
const { errors } = validateEnv(withEnv({ UPLOADS_BASE_URL: '/uploads' }));
|
||||||
|
expect(errors).toContainEqual(expect.stringContaining('absolute origin'));
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -3,7 +3,8 @@ import {
|
|||||||
SIGNATURE_BYTES,
|
SIGNATURE_BYTES,
|
||||||
extensionFor,
|
extensionFor,
|
||||||
isAllowedImageType,
|
isAllowedImageType,
|
||||||
signatureMatches
|
signatureMatches,
|
||||||
|
typeForExtension
|
||||||
} from '../../src/uploadTypes';
|
} from '../../src/uploadTypes';
|
||||||
|
|
||||||
// Heads long enough to satisfy the WebP check, which needs twelve bytes.
|
// Heads long enough to satisfy the WebP check, which needs twelve bytes.
|
||||||
@@ -105,3 +106,42 @@ describe('signatureMatches', () => {
|
|||||||
expect(SIGNATURE_BYTES).toBeGreaterThanOrEqual(12);
|
expect(SIGNATURE_BYTES).toBeGreaterThanOrEqual(12);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// The inverse of extensionFor, and the gate on what the uploads route will
|
||||||
|
// serve. #103: anything it does not recognise is not handed out, which is how a
|
||||||
|
// file written before there was any validation stops being reachable.
|
||||||
|
describe('typeForExtension', () => {
|
||||||
|
it('names the type for each extension the upload path can produce', () => {
|
||||||
|
expect(typeForExtension('.jpg')).toBe('image/jpeg');
|
||||||
|
expect(typeForExtension('.png')).toBe('image/png');
|
||||||
|
expect(typeForExtension('.webp')).toBe('image/webp');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('is the inverse of extensionFor for every allowed type', () => {
|
||||||
|
for (const type of ALLOWED_IMAGE_TYPES) {
|
||||||
|
const extension = extensionFor(type);
|
||||||
|
expect(extension).not.toBeNull();
|
||||||
|
expect(typeForExtension(extension as string)).toBe(type);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it('matches case-insensitively, since an extension on disk may be shouted', () => {
|
||||||
|
expect(typeForExtension('.JPG')).toBe('image/jpeg');
|
||||||
|
});
|
||||||
|
|
||||||
|
// The whole point. Each of these is a file type that executes when navigated
|
||||||
|
// to, and none of them can be produced by the current upload path — so
|
||||||
|
// finding one on disk means something went wrong earlier, and refusing to
|
||||||
|
// serve it is the last line.
|
||||||
|
it('refuses the types that would run as the site', () => {
|
||||||
|
expect(typeForExtension('.html')).toBeNull();
|
||||||
|
expect(typeForExtension('.htm')).toBeNull();
|
||||||
|
expect(typeForExtension('.svg')).toBeNull();
|
||||||
|
expect(typeForExtension('.js')).toBeNull();
|
||||||
|
expect(typeForExtension('.xml')).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses a file with no extension at all', () => {
|
||||||
|
expect(typeForExtension('')).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -153,6 +153,20 @@ services:
|
|||||||
- USPS_CLIENT_SECRET=${USPS_CLIENT_SECRET}
|
- USPS_CLIENT_SECRET=${USPS_CLIENT_SECRET}
|
||||||
|
|
||||||
- ADMIN_GATE_SECRET=${ADMIN_GATE_SECRET}
|
- ADMIN_GATE_SECRET=${ADMIN_GATE_SECRET}
|
||||||
|
|
||||||
|
# The origin uploaded images are fetched from (#103). Empty means this
|
||||||
|
# application serves them from its own origin, which works and is what
|
||||||
|
# every environment does today — so this is safe to leave unset while the
|
||||||
|
# hostname below does not exist yet.
|
||||||
|
#
|
||||||
|
# Present as a line even while empty, deliberately: a Portainer stack
|
||||||
|
# variable with no line here is substituted into this file and never
|
||||||
|
# reaches the container, which is exactly how UPLOADS_DIR went missing.
|
||||||
|
#
|
||||||
|
# Setting it needs an Nginx Proxy Manager host for the name, pointing at
|
||||||
|
# this same container, and a certificate that covers it. Until then the
|
||||||
|
# server warns at boot that the defence is off rather than staying silent.
|
||||||
|
- UPLOADS_BASE_URL=${UPLOADS_BASE_URL:-}
|
||||||
volumes:
|
volumes:
|
||||||
# Production's own uploads directory. QA writes to
|
# Production's own uploads directory. QA writes to
|
||||||
# /volume1/configs/redefined-designs-qa/uploads; sharing this one would
|
# /volume1/configs/redefined-designs-qa/uploads; sharing this one would
|
||||||
|
|||||||
@@ -37,6 +37,7 @@ import CategoryTreeSelect from './CategoryTreeSelect';
|
|||||||
import ItemCard from '../components/ItemCard';
|
import ItemCard from '../components/ItemCard';
|
||||||
import InventoryFilters from './InventoryFilters';
|
import InventoryFilters from './InventoryFilters';
|
||||||
import { ItemFilters, EMPTY_FILTERS } from '../filters';
|
import { ItemFilters, EMPTY_FILTERS } from '../filters';
|
||||||
|
import { uploadUrl } from '../uploadUrl';
|
||||||
|
|
||||||
const { Header, Content } = Layout;
|
const { Header, Content } = Layout;
|
||||||
const { Title } = Typography;
|
const { Title } = Typography;
|
||||||
@@ -195,7 +196,7 @@ function Inventory() {
|
|||||||
render: (images: Item['images']) =>
|
render: (images: Item['images']) =>
|
||||||
images[0] ? (
|
images[0] ? (
|
||||||
<span style={{ position: 'relative', display: 'inline-block' }}>
|
<span style={{ position: 'relative', display: 'inline-block' }}>
|
||||||
<img src={images[0].image_path} alt="" style={{ width: 60 }} />
|
<img src={uploadUrl(images[0].image_path)} alt="" style={{ width: 60 }} />
|
||||||
{images.length > 1 && (
|
{images.length > 1 && (
|
||||||
<Tag style={{ position: 'absolute', bottom: -4, right: -8, fontSize: 10 }}>+{images.length - 1}</Tag>
|
<Tag style={{ position: 'absolute', bottom: -4, right: -8, fontSize: 10 }}>+{images.length - 1}</Tag>
|
||||||
)}
|
)}
|
||||||
@@ -312,7 +313,7 @@ function Inventory() {
|
|||||||
<Space wrap>
|
<Space wrap>
|
||||||
{editingItem.images.map(img => (
|
{editingItem.images.map(img => (
|
||||||
<div key={img.id} style={{ position: 'relative' }}>
|
<div key={img.id} style={{ position: 'relative' }}>
|
||||||
<AntImage src={img.image_path} width={80} height={80} style={{ objectFit: 'cover' }} />
|
<AntImage src={uploadUrl(img.image_path)} width={80} height={80} style={{ objectFit: 'cover' }} />
|
||||||
<Button size="small" danger icon={<DeleteOutlined />} style={{ position: 'absolute', top: 0, right: 0 }}
|
<Button size="small" danger icon={<DeleteOutlined />} style={{ position: 'absolute', top: 0, right: 0 }}
|
||||||
onClick={() => handleDeleteImage(editingItem.id, img.id)} />
|
onClick={() => handleDeleteImage(editingItem.id, img.id)} />
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
+9
-1
@@ -1,5 +1,6 @@
|
|||||||
import type { ItemFilters } from './filters';
|
import type { ItemFilters } from './filters';
|
||||||
import { filtersToSearchParams } from './filters';
|
import { filtersToSearchParams } from './filters';
|
||||||
|
import { setUploadsBase } from './uploadUrl';
|
||||||
|
|
||||||
export interface ItemTag {
|
export interface ItemTag {
|
||||||
id: number;
|
id: number;
|
||||||
@@ -44,11 +45,18 @@ export interface SiteConfig {
|
|||||||
paypalClientId: string | null;
|
paypalClientId: string | null;
|
||||||
demoMode: boolean;
|
demoMode: boolean;
|
||||||
currency: string;
|
currency: string;
|
||||||
|
/** Origin for uploaded images. Empty means the app's own — see uploadUrl. */
|
||||||
|
uploadsBaseUrl: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function fetchConfig(): Promise<SiteConfig> {
|
export async function fetchConfig(): Promise<SiteConfig> {
|
||||||
const res = await fetch('/api/config');
|
const res = await fetch('/api/config');
|
||||||
return res.json();
|
const config = (await res.json()) as SiteConfig;
|
||||||
|
// Applied here rather than by each caller, so no caller can fetch the config
|
||||||
|
// and forget to — the uploads origin is a property of the deployment, not of
|
||||||
|
// whichever screen happened to ask for it.
|
||||||
|
setUploadsBase(config.uploadsBaseUrl);
|
||||||
|
return config;
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function fetchItems(filters?: ItemFilters): Promise<Item[]> {
|
export async function fetchItems(filters?: ItemFilters): Promise<Item[]> {
|
||||||
|
|||||||
@@ -27,6 +27,7 @@ import { useCart } from './CartContext';
|
|||||||
import { useCustomerAuth } from '../customer/CustomerAuthContext';
|
import { useCustomerAuth } from '../customer/CustomerAuthContext';
|
||||||
import { useNow } from './useNow';
|
import { useNow } from './useNow';
|
||||||
import { timeRemaining, isExpiringSoon, hasLapsedItem } from './reservation';
|
import { timeRemaining, isExpiringSoon, hasLapsedItem } from './reservation';
|
||||||
|
import { uploadUrl } from '../uploadUrl';
|
||||||
|
|
||||||
// The display has one-minute resolution, so half a minute keeps it honest
|
// The display has one-minute resolution, so half a minute keeps it honest
|
||||||
// without being busy. Once a second would be wasted work.
|
// without being busy. Once a second would be wasted work.
|
||||||
@@ -197,7 +198,7 @@ export default function Cart() {
|
|||||||
renderItem={item => (
|
renderItem={item => (
|
||||||
<List.Item actions={[<Button key="remove" danger size="small" onClick={() => handleRemove(item.item_id)}>Remove</Button>]}>
|
<List.Item actions={[<Button key="remove" danger size="small" onClick={() => handleRemove(item.item_id)}>Remove</Button>]}>
|
||||||
<List.Item.Meta
|
<List.Item.Meta
|
||||||
avatar={item.images[0] && <img src={item.images[0].image_path} alt="" style={{ width: 60, height: 60, objectFit: 'cover' }} />}
|
avatar={item.images[0] && <img src={uploadUrl(item.images[0].image_path)} alt="" style={{ width: 60, height: 60, objectFit: 'cover' }} />}
|
||||||
title={item.name}
|
title={item.name}
|
||||||
description={
|
description={
|
||||||
<Text type={isExpiringSoon(item.added_at, item.expires_at, now) ? 'danger' : 'secondary'}>
|
<Text type={isExpiringSoon(item.added_at, item.expires_at, now) ? 'danger' : 'secondary'}>
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ import { useCustomerAuth } from '../customer/CustomerAuthContext';
|
|||||||
import AuthPromptModal from '../customer/AuthPromptModal';
|
import AuthPromptModal from '../customer/AuthPromptModal';
|
||||||
import { useFavorites } from '../customer/FavoritesContext';
|
import { useFavorites } from '../customer/FavoritesContext';
|
||||||
import { addFavorite, removeFavorite, setFavoriteAlerts } from '../customer/favoritesApi';
|
import { addFavorite, removeFavorite, setFavoriteAlerts } from '../customer/favoritesApi';
|
||||||
|
import { uploadUrl } from '../uploadUrl';
|
||||||
|
|
||||||
const { Text, Title } = Typography;
|
const { Text, Title } = Typography;
|
||||||
|
|
||||||
@@ -142,7 +143,7 @@ export default function ItemCard({ item, onChanged, preview = false }: Readonly<
|
|||||||
<Carousel ref={carouselRef} dots={hasMultiple}>
|
<Carousel ref={carouselRef} dots={hasMultiple}>
|
||||||
{item.images.map(img => (
|
{item.images.map(img => (
|
||||||
<div key={img.id}>
|
<div key={img.id}>
|
||||||
<img src={img.image_path} alt={item.name} className="card-cover-img" />
|
<img src={uploadUrl(img.image_path)} alt={item.name} className="card-cover-img" />
|
||||||
</div>
|
</div>
|
||||||
))}
|
))}
|
||||||
</Carousel>
|
</Carousel>
|
||||||
|
|||||||
@@ -25,6 +25,7 @@ import { CartProvider } from './cart/CartContext';
|
|||||||
import { FavoritesProvider } from './customer/FavoritesContext';
|
import { FavoritesProvider } from './customer/FavoritesContext';
|
||||||
import { ThemeModeProvider, useThemeMode } from './theme/ThemeContext';
|
import { ThemeModeProvider, useThemeMode } from './theme/ThemeContext';
|
||||||
import './styles.css';
|
import './styles.css';
|
||||||
|
import { fetchConfig } from './api';
|
||||||
|
|
||||||
// The brand accent is monochrome, so it inverts between themes rather than
|
// The brand accent is monochrome, so it inverts between themes rather than
|
||||||
// switching to a different hue.
|
// switching to a different hue.
|
||||||
@@ -214,6 +215,16 @@ function Root() {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Fired at entry purely for its side effect: it sets the origin uploaded
|
||||||
|
// images are fetched from (#103). Not awaited, because nothing should wait on
|
||||||
|
// it — anything that renders first gets a site-relative path, which the app's
|
||||||
|
// own origin still serves.
|
||||||
|
//
|
||||||
|
// Rejections are swallowed on purpose. A config that cannot be fetched is a
|
||||||
|
// broken deployment which every other request will report; failing here would
|
||||||
|
// only replace the app with an error page before it has drawn anything.
|
||||||
|
void fetchConfig().catch(() => {});
|
||||||
|
|
||||||
ReactDOM.createRoot(document.getElementById('root')!).render(
|
ReactDOM.createRoot(document.getElementById('root')!).render(
|
||||||
<React.StrictMode>
|
<React.StrictMode>
|
||||||
<ThemeModeProvider>
|
<ThemeModeProvider>
|
||||||
|
|||||||
@@ -0,0 +1,43 @@
|
|||||||
|
/**
|
||||||
|
* Where an uploaded image is fetched from.
|
||||||
|
*
|
||||||
|
* Image paths are stored in the database as site-relative — `/uploads/<id>.jpg`
|
||||||
|
* — and that is deliberate: a stored value outlives any hostname it might be
|
||||||
|
* baked with, and rewriting them would be a migration to undo the day the host
|
||||||
|
* changes. So the origin is joined on here instead, from a value the server
|
||||||
|
* sends at runtime (#103).
|
||||||
|
*
|
||||||
|
* Empty base means the app's own origin, which is the default and is what local
|
||||||
|
* development has — there is no second hostname on a laptop. Point it at one in
|
||||||
|
* production and user-supplied files stop sharing an origin with the
|
||||||
|
* application.
|
||||||
|
*
|
||||||
|
* The base is cached in a module variable rather than threaded through context,
|
||||||
|
* because it is a deployment constant: one value, fetched once, never changing
|
||||||
|
* while the tab is open. Anything rendered before the fetch resolves gets the
|
||||||
|
* relative path, which still works — the app's origin goes on serving these
|
||||||
|
* files, hardened, and the separate host points at the same directory. It
|
||||||
|
* simply misses the isolation for that first paint.
|
||||||
|
*/
|
||||||
|
|
||||||
|
let base = '';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Called once, from the config the app already fetches at startup. Trailing
|
||||||
|
* slashes are trimmed on the server, and again here, so that a base configured
|
||||||
|
* either way joins cleanly with a path that always begins with one.
|
||||||
|
*/
|
||||||
|
export function setUploadsBase(value: string | undefined | null): void {
|
||||||
|
// Trimmed with a loop rather than a `/+$/` regex, which backtracks.
|
||||||
|
let trimmed = value ?? '';
|
||||||
|
while (trimmed.endsWith('/')) trimmed = trimmed.slice(0, -1);
|
||||||
|
base = trimmed;
|
||||||
|
}
|
||||||
|
|
||||||
|
export function uploadUrl(storedPath: string): string {
|
||||||
|
// Absolute already, or empty. Either way there is nothing to join: returning
|
||||||
|
// it untouched means a value that was somehow stored absolute keeps working
|
||||||
|
// rather than being mangled into a nonsense URL.
|
||||||
|
if (!base || !storedPath.startsWith('/')) return storedPath;
|
||||||
|
return `${base}${storedPath}`;
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user