00776234fd
Closes #1116. secureImageMiddleware set its own Access-Control-Allow-Origin, overwriting the one cors(corsOptions) had already computed. server.js:247 mounts cors() on all of /api with credentials:true, so by the time the route handler ran the correct header was already there — and the local assignment replaced it with a worse answer in BOTH directions: unresolved -> '*'. Combined with the credentials:true that cors() sets, that is an invalid pair browsers reject outright. Unreachable on Docker until #1104 stopped compose injecting FRONTEND_URL; reachable on a fresh install from then until the wizard stores general_site_url. resolved -> the frontend origin, even when the request legitimately came from the allowlisted ADMIN_URL. A split admin host got a header naming the wrong origin and the browser rejected a request cors() had allowed. Deleting the line fixes both. cors() already validates the request Origin against the allowlist, sets Vary: Origin, omits the header entirely for a disallowed or absent Origin, and pairs correctly with credentials. Methods, Headers and Max-Age stay here: they are route-specific and cors() does not contradict them. Observed against a running instance before and after: allowlisted Origin ACAO: <that origin> + Vary: Origin + credentials:true disallowed Origin no ACAO no Origin header no ACAO Six tests, mounted on a real Express app with server.js's middleware order. Deliberately NOT a unit test against a response double: the first version of this fix was a guarded assignment that looked correct in isolation and still overwrote cors() whenever an origin resolved. A double cannot see middleware composition, which is exactly how that slipped through. Mutation-checked both ways — restoring the original `|| '*'` fails 5 of 6, and restoring the guarded assignment fails 3 of 6 including the admin-origin case.
101 lines
4.2 KiB
JavaScript
101 lines
4.2 KiB
JavaScript
/**
|
|
* CORS posture of the protected-image responses (#1116).
|
|
*
|
|
* secureImageMiddleware used to set its own Access-Control-Allow-Origin,
|
|
* overwriting the one cors(corsOptions) had already computed for the request.
|
|
* That was worse in both directions:
|
|
*
|
|
* unresolved -> '*', which with the credentials:true that cors() sets is an
|
|
* invalid pair every browser rejects outright
|
|
* resolved -> the frontend origin, even when the request legitimately came
|
|
* from the allowlisted ADMIN_URL
|
|
*
|
|
* The header now belongs to cors() alone. These tests are mounted on a real
|
|
* Express app with the same middleware order as server.js — a unit test against
|
|
* a response double cannot see middleware composition, which is precisely how
|
|
* the first version of this fix looked correct while still being wrong.
|
|
*/
|
|
|
|
process.env.NODE_ENV = 'test';
|
|
|
|
const express = require('express');
|
|
const cors = require('cors');
|
|
const request = require('supertest');
|
|
|
|
jest.mock('../src/database/db', () => ({ db: jest.fn() }));
|
|
|
|
// A frontend origin IS resolvable here, deliberately. With the resolver empty
|
|
// (the default in tests) merely GUARDING the assignment looks identical to
|
|
// removing it — the admin-origin case below is what tells them apart, and it
|
|
// is the common one in production.
|
|
jest.mock('../src/utils/frontendUrl', () => ({
|
|
getFrontendBaseUrlSync: () => 'https://gallery.example.com',
|
|
}));
|
|
|
|
jest.useFakeTimers(); // the module schedules a cleanup setInterval at require time
|
|
const secureImageMiddleware = require('../src/middleware/secureImageMiddleware');
|
|
|
|
const FRONTEND = 'https://gallery.example.com';
|
|
const ADMIN = 'https://admin.example.com';
|
|
|
|
/** Mirrors server.js: cors() on /api, then the route sets its own headers. */
|
|
function buildApp() {
|
|
const app = express();
|
|
app.use('/api', cors({
|
|
origin: (origin, cb) => cb(null, !origin || [FRONTEND, ADMIN].includes(origin)),
|
|
credentials: true,
|
|
}));
|
|
app.get('/api/secure-images/:id', (req, res) => {
|
|
secureImageMiddleware.setSecurityHeaders(res);
|
|
res.status(200).send('ok');
|
|
});
|
|
return app;
|
|
}
|
|
|
|
afterAll(() => {
|
|
jest.clearAllTimers();
|
|
jest.useRealTimers();
|
|
});
|
|
|
|
describe('protected-image CORS headers', () => {
|
|
it('never emits a wildcard origin', async () => {
|
|
// '*' alongside the credentials:true that cors() sets is invalid, and the
|
|
// browser drops the whole response.
|
|
const res = await request(buildApp()).get('/api/secure-images/1').set('Origin', FRONTEND);
|
|
expect(res.headers['access-control-allow-origin']).not.toBe('*');
|
|
});
|
|
|
|
it('preserves the cors() answer for an allowlisted origin', async () => {
|
|
const res = await request(buildApp()).get('/api/secure-images/1').set('Origin', FRONTEND);
|
|
expect(res.headers['access-control-allow-origin']).toBe(FRONTEND);
|
|
expect(res.headers['access-control-allow-credentials']).toBe('true');
|
|
});
|
|
|
|
it('does not repoint an allowlisted admin origin at the frontend', async () => {
|
|
// The regression the old code caused, and the case a guarded assignment
|
|
// still gets wrong: the resolver returns the FRONTEND origin here, so any
|
|
// code that writes it would stamp the wrong origin on an admin request
|
|
// that cors() had already allowed, and the browser would reject it.
|
|
const res = await request(buildApp()).get('/api/secure-images/1').set('Origin', ADMIN);
|
|
expect(res.headers['access-control-allow-origin']).toBe(ADMIN);
|
|
});
|
|
|
|
it('stays absent for a disallowed origin', async () => {
|
|
const res = await request(buildApp()).get('/api/secure-images/1').set('Origin', 'https://evil.example.com');
|
|
expect(res.headers).not.toHaveProperty('access-control-allow-origin');
|
|
});
|
|
|
|
it('stays absent when there is no Origin at all', async () => {
|
|
const res = await request(buildApp()).get('/api/secure-images/1');
|
|
expect(res.headers).not.toHaveProperty('access-control-allow-origin');
|
|
});
|
|
|
|
it('still sets the route-specific security headers', async () => {
|
|
const res = await request(buildApp()).get('/api/secure-images/1').set('Origin', FRONTEND);
|
|
expect(res.headers['x-content-type-options']).toBe('nosniff');
|
|
expect(res.headers['x-frame-options']).toBe('DENY');
|
|
expect(res.headers['cache-control']).toContain('no-store');
|
|
expect(res.headers['access-control-allow-methods']).toBe('GET');
|
|
});
|
|
});
|