feat(security): stop the app origin serving anything it does not recognise, and make the uploads origin configurable (#103) #173
+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