README.md

@@ -179,6 +179,27 @@ Production runs as a single Docker image (multi-stage build — the frontend is
 
 The container applies pending migrations before starting the server, so deployed code can never be ahead of the database schema. A failed migration stops the container rather than letting it serve against a schema it doesn't match — check `docker logs` on the app container if it doesn't come up.
 
+### The admin authorization boundary
+
+Worth reading before adding any admin route, because the control is invisible from the code.
+
+Authorization for the admin panel and the admin API lives in a single `auth_request` regex in the Nginx Proxy Manager config — `^/(admin|api/admin)` in production, and `location /` in QA, where the whole site is gated. That config is not in this repository. Three consequences follow, and none of them are visible from Express:
+
+- **An admin route added at a path the regex does not match is not covered by it.** `/api/reports` or `/api/internal/...` would be publicly reachable the moment it shipped.
+- **Anything that reaches the container directly bypasses authentik entirely**, because the gate is in the proxy in front of it. QA publishes port 32751 on the NAS and production publishes its own.
+- **Locally there is no gate at all**, so `/admin` and the whole admin API are open by design and no developer ever sees the boundary being enforced.
+
+`ADMIN_GATE_SECRET` is the application-layer half of this, and it is optional:
+
+| State | Behaviour |
+| --- | --- |
+| Unset | Every admin route is reachable, exactly as before. The server logs an `[admin-gate]` warning at boot saying so, so the state is visible rather than silent. This is what local development and the test suite run in. |
+| Set | Every admin router requires an `X-Admin-Gate` header matching the value, and returns 403 without it. |
+
+To turn it on, the secret has to be set in **two places at once** — the stack's environment, and a `proxy_set_header X-Admin-Gate "<secret>";` line on the gated location in Nginx Proxy Manager. Setting it in only one of them makes the admin panel return 403 until the other catches up. That failure is loud and recoverable, unlike the one it replaces.
+
+The middleware is attached to each admin **router** rather than to a path prefix. That is deliberate: an admin router added later at some other path inherits the gate, and because the proxy only injects the header on paths its regex matches, that router refuses on its first request rather than being quietly public. A 403 in that situation means the proxy regex needs widening — it is the boundary telling you it has drifted.
+
 ### Promoting a reviewed change to production
 
 Only after the change has been reviewed in QA.

backend/src/app.ts

@@ -15,6 +15,7 @@ import cartRouter from './routes/cart';
 import shippingAddressesRouter from './routes/shippingAddresses';
 import clientErrorsRouter from './routes/clientErrors';
 import { attachCustomer } from './middleware/customerAuth';
+import { requireAdminGate } from './middleware/adminGate';
 import { asyncRoute } from './asyncRoute';
 
 const app = express();
@@ -46,11 +47,16 @@ app.use('/api/items', itemsRouter);
 app.use('/api/filters', filtersRouter);
 app.use('/api/cart', cartRouter);
 app.use('/api/checkout/cart', cartCheckoutRouter);
-app.use('/api/admin/customers', adminCustomersRouter);
-app.use('/api/admin/settings', adminSettingsRouter);
-app.use('/api/admin/categories', adminCategoriesRouter);
-app.use('/api/admin/tags', adminTagsRouter);
-app.use('/api/admin', adminRouter);
+// requireAdminGate is attached to each admin router rather than to a path
+// prefix. Attached to the router, an admin router added later at some other
+// path still inherits it — and since the proxy only injects the header on the
+// paths its regex matches, that router refuses loudly on its first request
+// instead of being quietly public. See middleware/adminGate.ts and #63.
+app.use('/api/admin/customers', requireAdminGate, adminCustomersRouter);
+app.use('/api/admin/settings', requireAdminGate, adminSettingsRouter);
+app.use('/api/admin/categories', requireAdminGate, adminCategoriesRouter);
+app.use('/api/admin/tags', requireAdminGate, adminTagsRouter);
+app.use('/api/admin', requireAdminGate, adminRouter);
 app.use('/api/customers/me/addresses', shippingAddressesRouter);
 app.use('/api/customers', customersRouter);
 app.use('/api/client-errors', clientErrorsRouter);

backend/src/middleware/adminGate.ts

@@ -0,0 +1,61 @@
+import { Request, Response, NextFunction } from 'express';
+import crypto from 'crypto';
+
+// The header Nginx Proxy Manager injects on the authentik-gated location. The
+// name is not a secret and does not need to be — the value is.
+export const ADMIN_GATE_HEADER = 'x-admin-gate';
+
+// Hashed before comparing for two reasons. timingSafeEqual throws on buffers of
+// unequal length, so comparing raw values would turn a short header into a 500
+// instead of a 403; and a length check before comparing would leak the secret's
+// length. Digests are always 32 bytes, so neither problem arises.
+function digest(value: string): Buffer {
+  return crypto.createHash('sha256').update(value, 'utf8').digest();
+}
+
+/**
+ * Defence in depth for the admin API.
+ *
+ * Authorization for `/admin` and `/api/admin` lives entirely in one
+ * `auth_request` regex in an Nginx Proxy Manager config outside this
+ * repository. That control is real and it works, but it is invisible from the
+ * code, untested here, and bypassed completely by anything that reaches the
+ * published container port directly. See #63.
+ *
+ * With `ADMIN_GATE_SECRET` set, the proxy injects the matching header and this
+ * refuses anything that arrives without it.
+ *
+ * Unset — or empty, which cannot mean "enforce" without letting an empty header
+ * through — this is a no-op and the API is proxy-protected exactly as before.
+ * That keeps local development and the existing admin tests working untouched,
+ * and means shipping the image before configuring the proxy cannot take the
+ * admin panel down. `server.ts` warns at boot when it is inactive, so the
+ * inactive state is visible rather than silent.
+ *
+ * Mounted on the admin routers rather than on a path prefix, deliberately. An
+ * admin router added later at a path the proxy regex does not match will
+ * receive no header and refuse loudly on the first request, instead of being
+ * quietly public — which is the failure #63 was most concerned about.
+ */
+export function requireAdminGate(req: Request, res: Response, next: NextFunction): void {
+  const secret = process.env.ADMIN_GATE_SECRET;
+  if (!secret) {
+    next();
+    return;
+  }
+
+  const provided = req.get(ADMIN_GATE_HEADER);
+  if (typeof provided !== 'string' || !crypto.timingSafeEqual(digest(provided), digest(secret))) {
+    // Logged because a 403 from behind a proxy is otherwise very hard to
+    // diagnose — most often it means the proxy config and the stack's secret
+    // have drifted apart. The value sent is deliberately not echoed.
+    console.warn(
+      `[admin-gate] refused ${req.method} ${req.originalUrl} — ` +
+        `${provided === undefined ? 'no' : 'incorrect'} ${ADMIN_GATE_HEADER} header`
+    );
+    res.status(403).json({ error: 'forbidden' });
+    return;
+  }
+
+  next();
+}

backend/src/server.ts

@@ -65,4 +65,17 @@ setInterval(() => void sweepExpiredCarts(), 5 * 60 * 1000);
 cron.schedule('0 9 * * *', () => void sendCartReminders());
 
 const PORT = parseInt(process.env.PORT || '3000', 10);
+// Said at boot rather than left to be discovered. Without the secret the admin
+// API is protected only by the reverse proxy's auth_request regex, which lives
+// outside this repository and is bypassed entirely by anything reaching this
+// container's published port directly. That is a defensible way to run — it is
+// how this app has always run — but it should be a visible choice rather than a
+// silent one. See middleware/adminGate.ts and #63.
+if (!process.env.ADMIN_GATE_SECRET) {
+  console.warn(
+    '[admin-gate] ADMIN_GATE_SECRET is not set — /api/admin is protected only by the reverse ' +
+      'proxy. Anything able to reach this container directly can administer the store.'
+  );
+}
+
 app.listen(PORT, () => console.log(`redefined-designs listening on ${PORT}`));

backend/tests/integration/adminGate.integration.test.ts

@@ -0,0 +1,94 @@
+import request from 'supertest';
+import app from '../../src/app';
+import { pool } from '../../src/db';
+import { resetDb, closeDb } from './setup/testDb';
+
+// The unit tests prove the middleware decides correctly. This proves it is
+// actually attached to the admin routers — a correct function nobody mounted
+// would pass every unit test and protect nothing.
+//
+// ADMIN_GATE_SECRET is read per request rather than at import, so it can be set
+// here without reloading the app. It is restored after each test so this file
+// cannot change how anything running later in the same process behaves.
+
+const SECRET = 'integration-gate-secret';
+const original = process.env.ADMIN_GATE_SECRET;
+let warn: jest.SpyInstance;
+
+beforeEach(async () => {
+  await resetDb();
+  warn = jest.spyOn(console, 'warn').mockImplementation(() => undefined);
+});
+
+afterEach(() => {
+  warn.mockRestore();
+  if (original === undefined) {
+    delete process.env.ADMIN_GATE_SECRET;
+  } else {
+    process.env.ADMIN_GATE_SECRET = original;
+  }
+});
+
+afterAll(async () => {
+  await pool.end();
+  await closeDb();
+});
+
+describe('the admin gate, mounted on the real routers', () => {
+  it('refuses an admin request with no gate header once a secret is configured', async () => {
+    process.env.ADMIN_GATE_SECRET = SECRET;
+
+    const res = await request(app).get('/api/admin/items');
+
+    expect(res.status).toBe(403);
+    expect(res.body.error).toBe('forbidden');
+  });
+
+  it('serves the same request when it carries the gate header', async () => {
+    process.env.ADMIN_GATE_SECRET = SECRET;
+
+    const res = await request(app).get('/api/admin/items').set('X-Admin-Gate', SECRET);
+
+    expect(res.status).toBe(200);
+    expect(Array.isArray(res.body)).toBe(true);
+  });
+
+  // Every admin router has to be covered, not just the one mounted last. These
+  // are separate app.use calls, so one of them missing the middleware is an
+  // easy and completely silent mistake.
+  it.each([
+    ['/api/admin/customers', 'customers'],
+    ['/api/admin/categories', 'categories'],
+    ['/api/admin/tags', 'tags'],
+    ['/api/admin/settings', 'settings'],
+    ['/api/admin/items', 'inventory']
+  ])('gates %s', async (path) => {
+    process.env.ADMIN_GATE_SECRET = SECRET;
+
+    const refused = await request(app).get(path);
+    expect(refused.status).toBe(403);
+
+    const allowed = await request(app).get(path).set('X-Admin-Gate', SECRET);
+    expect(allowed.status).not.toBe(403);
+  });
+
+  // The state every existing admin test relies on, and the state production
+  // runs in until the proxy is configured.
+  it('leaves admin reachable when no secret is configured', async () => {
+    delete process.env.ADMIN_GATE_SECRET;
+
+    const res = await request(app).get('/api/admin/items');
+
+    expect(res.status).toBe(200);
+  });
+
+  // The storefront must not be caught by the gate — it is public by design and
+  // the proxy does not add the header to it in production.
+  it('does not gate the public storefront', async () => {
+    process.env.ADMIN_GATE_SECRET = SECRET;
+
+    const res = await request(app).get('/api/items');
+
+    expect(res.status).toBe(200);
+  });
+});

backend/tests/unit/adminGate.test.ts

@@ -0,0 +1,140 @@
+import { Request, Response, NextFunction } from 'express';
+import { requireAdminGate } from '../../src/middleware/adminGate';
+
+// The gate is the whole point of #63, so both directions are tested: it must
+// let the right request through, and it must refuse the wrong one. A gate that
+// only ever refuses is as broken as one that only ever allows — it would take
+// the admin panel down rather than protect it.
+
+interface Harness {
+  req: Request;
+  res: Response;
+  next: NextFunction;
+  status: jest.Mock;
+  json: jest.Mock;
+}
+
+function harness(header?: string): Harness {
+  const status = jest.fn().mockReturnThis();
+  const json = jest.fn().mockReturnThis();
+  const req = {
+    method: 'POST',
+    originalUrl: '/api/admin/items',
+    get: (name: string) => (name.toLowerCase() === 'x-admin-gate' ? header : undefined)
+  } as unknown as Request;
+  const res = { status, json } as unknown as Response;
+  return { req, res, next: jest.fn() as unknown as NextFunction, status, json };
+}
+
+describe('requireAdminGate', () => {
+  const original = process.env.ADMIN_GATE_SECRET;
+  let warn: jest.SpyInstance;
+
+  beforeEach(() => {
+    warn = jest.spyOn(console, 'warn').mockImplementation(() => undefined);
+  });
+
+  afterEach(() => {
+    warn.mockRestore();
+    // Restored rather than deleted unconditionally, so this file cannot change
+    // the behaviour of anything that runs after it in the same process.
+    if (original === undefined) {
+      delete process.env.ADMIN_GATE_SECRET;
+    } else {
+      process.env.ADMIN_GATE_SECRET = original;
+    }
+  });
+
+  describe('when no secret is configured', () => {
+    // Today's behaviour, and local dev and every admin test depend on it.
+    it('lets the request through', () => {
+      delete process.env.ADMIN_GATE_SECRET;
+      const h = harness();
+
+      requireAdminGate(h.req, h.res, h.next);
+
+      expect(h.next).toHaveBeenCalledTimes(1);
+      expect(h.status).not.toHaveBeenCalled();
+    });
+
+    // An empty value cannot mean "enforce", because then any caller sending an
+    // empty header would pass. It has to mean the same as unset.
+    it('treats an empty secret as unconfigured rather than as a secret', () => {
+      process.env.ADMIN_GATE_SECRET = '';
+      const h = harness('');
+
+      requireAdminGate(h.req, h.res, h.next);
+
+      expect(h.next).toHaveBeenCalledTimes(1);
+      expect(h.status).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('when a secret is configured', () => {
+    const SECRET = 'a-real-gate-secret';
+
+    beforeEach(() => {
+      process.env.ADMIN_GATE_SECRET = SECRET;
+    });
+
+    it('lets a request carrying the right header through', () => {
+      const h = harness(SECRET);
+
+      requireAdminGate(h.req, h.res, h.next);
+
+      expect(h.next).toHaveBeenCalledTimes(1);
+      expect(h.status).not.toHaveBeenCalled();
+    });
+
+    it('refuses a request with no header at all', () => {
+      const h = harness();
+
+      requireAdminGate(h.req, h.res, h.next);
+
+      expect(h.next).not.toHaveBeenCalled();
+      expect(h.status).toHaveBeenCalledWith(403);
+      expect(h.json).toHaveBeenCalledWith({ error: 'forbidden' });
+    });
+
+    it('refuses a request with the wrong header', () => {
+      const h = harness('not-the-secret-but-same-length');
+
+      requireAdminGate(h.req, h.res, h.next);
+
+      expect(h.next).not.toHaveBeenCalled();
+      expect(h.status).toHaveBeenCalledWith(403);
+    });
+
+    // The trap in timingSafeEqual: it throws on buffers of unequal length, so
+    // comparing raw values would turn a short header into a 500 instead of a
+    // 403. Hashing both sides first is what makes the lengths always equal.
+    it('refuses a header of a different length without throwing', () => {
+      const h = harness('short');
+
+      expect(() => requireAdminGate(h.req, h.res, h.next)).not.toThrow();
+      expect(h.next).not.toHaveBeenCalled();
+      expect(h.status).toHaveBeenCalledWith(403);
+    });
+
+    it('refuses a header that merely starts with the secret', () => {
+      const h = harness(`${SECRET}-extra`);
+
+      requireAdminGate(h.req, h.res, h.next);
+
+      expect(h.status).toHaveBeenCalledWith(403);
+    });
+
+    // A silent 403 is very hard to diagnose from the other side of a proxy.
+    it('logs the refusal without echoing the value it was sent', () => {
+      const h = harness('some-guessed-value');
+
+      requireAdminGate(h.req, h.res, h.next);
+
+      expect(warn).toHaveBeenCalledTimes(1);
+      const logged = warn.mock.calls[0][0] as string;
+      expect(logged).toContain('[admin-gate]');
+      expect(logged).toContain('/api/admin/items');
+      expect(logged).not.toContain('some-guessed-value');
+    });
+  });
+});