fix(security,reliability): address code-review findings

HIGH - async handlers: Express 4 does not catch rejected promises and Node 22 exits on unhandled rejections — a malformed body ({} to /cropper/dequeue etc.) could hang a request or kill the process. Every cropper route is now wrapped (throw → 500 via next), request bodies are validated up front (400), and JSON.parse of queue items is guarded. Server-wide error middleware added, honouring err.status (413 for oversized bodies). - error pages HTML-escape all interpolated values — the login failure page rendered upstream (portal) error text raw. MEDIUM - outbound calls carry AbortSignal timeouts (5s OpenFGA verify, 15s cropper upstreams) — a hung upstream can no longer pin a request. - body limits scoped: /cropper gets 50mb (picture data URLs), everything else 1mb (previously 50mb globally, memory-DoS surface pre-auth). - cropper save validates the client-supplied domain against the catalog; picture fetches validate the Minio object key (id-domains/ prefix, no ..). LOW - static assets served before the session middleware (no PocketBase load per asset); landing header builds DOM nodes with textContent instead of innerHTML; picture load checks upstream ok. 5 new tests (40 total) covering malformed bodies, malformed queue items, unknown domains, object-key traversal, and error-page escaping.
2026-10-01 03:12:02 +01:00
parent 3044986b04
commit 643e2c60be
6 changed files with 216 additions and 31 deletions
@@ -70,6 +70,7 @@ export function createCropperRouter(cfg) {
    carvekitKey,
    removebgUrl = REMOVEBG_URL,
    noderedBase = 'https://nodered.a2loft.net',
+    domainCodes = new Set(),
    requireApi,
    cache: cropCache,
  } = cfg;
@@ -95,6 +96,25 @@ export function createCropperRouter(cfg) {
  };
  router.use(cropGate);

+  // Express 4 does not catch async rejections: an unhandled throw would hang
+  // the request (and, on Node's default, exit the process). Wrap every route
+  // handler so failures become a 500 via next(err).
+  for (const method of ['get', 'post']) {
+    const register = router[method].bind(router);
+    router[method] = (path, ...handlers) =>
+      register(path, ...handlers.map((h) => (h.length >= 3 ? h : (req, res, next) => Promise.resolve(h(req, res, next)).catch(next))));
+  }
+
+  /** Reject malformed bodies up front instead of throwing on destructuring. */
+  const requireMessageBody = (req, res) => {
+    if (!req.body || typeof req.body.message !== 'object' || req.body.message === null) {
+      res.status(400).json({ ok: false, message: 'A message body is required.' });
+      return false;
+    }
+    return true;
+  };
+
+  const UPSTREAM_TIMEOUT_MS = 15000;
  const base = `${cfg.a2loftBase.replace(/\/+$/, '')}/identity`;
  const nodered = (path) => `${noderedBase.replace(/\/+$/, '')}${path}`;

@@ -130,7 +150,12 @@ export function createCropperRouter(cfg) {
    if (!message) {
      return res.json({ ok: false, message: 'Queue is currently empty.' });
    }
-    const data = JSON.parse(message.messageText);
+    let data;
+    try {
+      data = JSON.parse(message.messageText);
+    } catch {
+      return res.json({ ok: false, message: 'Queue item is malformed.' });
+    }
    const { messageId, popReceipt, dequeueCount } = message;
    res.json({
      ok: true,
@@ -140,9 +165,12 @@ export function createCropperRouter(cfg) {

  router.get('/picture/load/:id', async (req, res) => {
    try {
-      const r = await fetchImpl(`${base}/dims/picture/${encodeURIComponent(req.params.id)}`);
-      const { data } = await r.json();
-      if (!data) return res.json({ ok: false, message: 'No upload with the specified id.' });
+      const r = await fetchImpl(`${base}/dims/picture/${encodeURIComponent(req.params.id)}`, {
+        signal: AbortSignal.timeout(UPSTREAM_TIMEOUT_MS),
+      });
+      const body = await r.json().catch(() => null);
+      const data = body && body.data;
+      if (!r.ok || !data) return res.json({ ok: false, message: 'No upload with the specified id.' });
      const { upload_id, file_name, caption, domain, raw_file_id } = data;
      return res.json({
        ok: true,
@@ -163,6 +191,7 @@ export function createCropperRouter(cfg) {
      const review = await (
        await fetchImpl(nodered(`/idid/picture/review/${encodeURIComponent(req.params.review_id)}`), {
          headers: { authorization: `Basic ${Buffer.from(`${noderedAuth.username}:${noderedAuth.password}`).toString('base64')}` },
+          signal: AbortSignal.timeout(UPSTREAM_TIMEOUT_MS),
        })
      ).json();
      if (!review.ok) {
@@ -171,6 +200,7 @@ export function createCropperRouter(cfg) {
      const agents = await (
        await fetchImpl(nodered('/unical/agents'), {
          headers: { authorization: `Basic ${Buffer.from(`${noderedAuth.username}:${noderedAuth.password}`).toString('base64')}` },
+          signal: AbortSignal.timeout(UPSTREAM_TIMEOUT_MS),
        })
      ).json();
      const agent = (Array.isArray(agents) ? agents : []).find(
@@ -184,9 +214,12 @@ export function createCropperRouter(cfg) {

  router.get('/picture/:file_name', async (req, res) => {
    const file_name = req.params.file_name.replace(/_/gi, '/');
+    if (!file_name.startsWith('id-domains/') || file_name.includes('..')) {
+      return res.status(400).json({ ok: false, message: 'Invalid picture reference.' });
+    }
    try {
      const url = await minio.presignedGetObject(BUCKET, file_name);
-      const r = await fetchImpl(url);
+      const r = await fetchImpl(url, { signal: AbortSignal.timeout(UPSTREAM_TIMEOUT_MS) });
      const buf = Buffer.from(await r.arrayBuffer());
      res.json({ ok: true, data: `data:image/png;base64,${buf.toString('base64')}` });
    } catch (error) {
@@ -195,13 +228,15 @@ export function createCropperRouter(cfg) {
  });

  router.post('/picture/review', async (req, res) => {
+    if (!requireMessageBody(req, res)) return;
    const {
-      message: { pop_receipt, message_id },
+      message: { pop_receipt, message_id } = {},
      deleteFromQueue,
      ...body
    } = req.body;
    try {
      const r = await fetchImpl(nodered('/zeebe/picture/review'), {
+        signal: AbortSignal.timeout(UPSTREAM_TIMEOUT_MS),
        method: 'POST',
        headers: {
          'content-type': 'application/json',
@@ -230,8 +265,9 @@ export function createCropperRouter(cfg) {
  });

  router.post('/dequeue', async (req, res) => {
+    if (!requireMessageBody(req, res)) return;
    const {
-      message: { pop_receipt, message_id },
+      message: { pop_receipt, message_id } = {},
    } = req.body;
    try {
      const result = await deleteMessage(message_id, pop_receipt);
@@ -250,6 +286,7 @@ export function createCropperRouter(cfg) {
      const fd = new FormData();
      fd.append('image_file_b64', req.body.image_file_b64);
      const r = await fetchImpl(removebgUrl, {
+        signal: AbortSignal.timeout(UPSTREAM_TIMEOUT_MS),
        method: 'POST',
        headers: { 'x-api-key': carvekitKey },
        body: fd,
@@ -271,6 +308,7 @@ export function createCropperRouter(cfg) {

  // ── save (the one a2loft call that needs the operator OTP) ──────────────
  router.post('/profile/picture', async (req, res) => {
+    if (!requireMessageBody(req, res)) return;
    if (!requireOtp(req, res)) return;
    let live;
    try {
@@ -297,14 +335,18 @@ export function createCropperRouter(cfg) {
      message: {
        pop_receipt,
        message_id,
-        message: { upload_id, domain, agentDeviceHash },
+        message: { upload_id, domain, agentDeviceHash } = {},
      },
    } = req.body;
+    if (!domainCodes.has(domain)) {
+      return res.status(400).json({ ok: false, message: 'Unknown domain.' });
+    }
    const q = new URLSearchParams({ phone: req.session.phone, otp: req.query.otp });
    try {
      const r = await fetchImpl(
        `${base}/${encodeURIComponent(domain)}/profile/picture?${q.toString()}`,
        {
+          signal: AbortSignal.timeout(UPSTREAM_TIMEOUT_MS),
          method: 'POST',
          headers: { 'content-type': 'application/json' },
          body: JSON.stringify({
@@ -4,6 +4,8 @@
 // Results are cached per operator phone for DOMAIN_TTL_MS.
 export const DOMAIN_TTL_MS = 5 * 60 * 1000;
 export const CACHE_FRESH = 'CACHE_FRESH';
+/** Outbound OpenFGA verify timeout — a hung portal must not pin a request. */
+export const VERIFY_TIMEOUT_MS = 5000;

 export function stripPhonePrefix(phone) {
  return (phone || '').replace(/^\+/, '');
@@ -19,6 +21,7 @@ async function verifyOnce(fetchImpl, portalUrl, phone, object, relation = 'captu
      relation,
      object,
    }),
+    signal: AbortSignal.timeout(VERIFY_TIMEOUT_MS),
  });
  if (r.status !== 200) throw new Error(`verify status ${r.status}`);
  const out = await r.json();
@@ -76,11 +76,21 @@
      .then(function (session) {
        var host = document.getElementById('nav-session');
        if (!session) return; // keep the Sign in link
-        var first = (session.firstName || session.phone || '?').trim();
-        host.innerHTML =
-          '<span class="user-chip"><span class="avatar">' + first.slice(0, 2).toUpperCase() + '</span>' +
-          (session.firstName || session.phone) + '</span>' +
-          '<a class="linklike" href="/auth/logout">Sign out</a>';
+        var name = (session.firstName || session.phone || '?').trim();
+        host.textContent = '';
+        var chip = document.createElement('span');
+        chip.className = 'user-chip';
+        var avatar = document.createElement('span');
+        avatar.className = 'avatar';
+        avatar.textContent = name.slice(0, 2).toUpperCase(); // textContent, never innerHTML
+        var label = document.createElement('span');
+        label.textContent = name;
+        chip.append(avatar, label);
+        var signOut = document.createElement('a');
+        signOut.className = 'linklike';
+        signOut.href = '/auth/logout';
+        signOut.textContent = 'Sign out';
+        host.append(chip, signOut);
      })
      .catch(function () { /* keep Sign in */ });
  </script>
@@ -70,7 +70,7 @@ function errorPage(res, status, title, message, actions = {}) {
 <head>
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width, initial-scale=1">
-  <title>${title} · YOUreka Tools</title>
+  <title>${esc(title)} · YOUreka Tools</title>
  <link rel="stylesheet" href="/assets/style.css">
 </head>
 <body class="app">
@@ -90,10 +90,10 @@ function errorPage(res, status, title, message, actions = {}) {
  <main class="container narrow center-v">
    <div class="card message-card">
      <div class="message-icon error">${ICONS[icon] || ICONS.alert}</div>
-      <h1>${title}</h1>
-      <p class="muted">${message}</p>
-      <a class="btn btn-primary" href="${primary.href}">${primary.label}</a>
-      ${secondary ? `<a class="btn btn-ghost" href="${secondary.href}">${secondary.label}</a>` : ''}
+      <h1>${esc(title)}</h1>
+      <p class="muted">${esc(message)}</p>
+      <a class="btn btn-primary" href="${esc(primary.href)}">${esc(primary.label)}</a>
+      ${secondary ? `<a class="btn btn-ghost" href="${esc(secondary.href)}">${esc(secondary.label)}</a>` : ''}
    </div>
  </main>
 </body>
@@ -107,6 +107,16 @@ const ICONS = {
    '<svg viewBox="0 0 24 24" width="28" height="28" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><rect x="3" y="11" width="18" height="11" rx="2"/><path d="M7 11V7a5 5 0 0 1 10 0v4"/></svg>',
 };

+/** HTML-escape interpolated values — error pages may carry upstream text. */
+function esc(value) {
+  return String(value ?? '')
+    .replace(/&/g, '&amp;')
+    .replace(/</g, '&lt;')
+    .replace(/>/g, '&gt;')
+    .replace(/"/g, '&quot;')
+    .replace(/'/g, '&#39;');
+};
+
 // ── app factory ──────────────────────────────────────────────────────────────
 export function createApp(overrides = {}) {
  const cfg = {
@@ -127,7 +137,9 @@ export function createApp(overrides = {}) {
  const app = express();
  app.disable('x-powered-by');
  app.set('trust proxy', 1); // https termination upstream → correct Secure-cookie flag
-  app.use(express.json({ limit: '50mb' })); // cropper pictures travel as data URLs
+  // cropper pictures travel as data URLs (large); everything else stays small
+  app.use('/cropper', express.json({ limit: '50mb' }));
+  app.use(express.json({ limit: '1mb' }));

  const setSessionCookie = (req, res, value) =>
    res.cookie('session', value, {
@@ -147,6 +159,9 @@ export function createApp(overrides = {}) {
      maxAge: SESSION_MAX_AGE_MS,
    });

+  // ── static assets (no session work per asset) ─────────────────────────────
+  app.use(express.static(path.join(__dirname, 'public')));
+
  // ── session middleware: recover / refresh the YOUreka Auth session ────────
  app.use(async (req, res, next) => {
    req.session = null;
@@ -181,7 +196,6 @@ export function createApp(overrides = {}) {
  };

  // ── static assets & landing page ──────────────────────────────────────────
-  app.use(express.static(path.join(__dirname, 'public')));

  // ── auth: YOUreka portal web-auth flow ────────────────────────────────────
  app.get('/auth/login', async (req, res) => {
@@ -339,6 +353,7 @@ export function createApp(overrides = {}) {
      createCropperRouter({
        ...cropperClients,
        a2loftBase: cfg.a2loftBase,
+        domainCodes,
        removebgUrl: cfg.removebgUrl,
        noderedBase: cfg.noderedBase,
        ...cropGateContext,
@@ -411,6 +426,22 @@ export function createApp(overrides = {}) {
    }
  });

+  // ── error handler (async handlers are wrapped; this catches next(err)) ─────
+  // eslint-disable-next-line no-unused-vars
+  app.use((err, req, res, next) => {
+    const status = err.status || err.statusCode || 500;
+    if (status >= 500) console.error('[error]', req.method, req.path, err && err.message);
+    if (res.headersSent) return next(err);
+    const message =
+      status === 413 ? 'The request is too large.'
+      : status >= 500 ? 'An unexpected error occurred. Please try again.'
+      : err.message || 'The request could not be processed.';
+    if (req.path === '/cropper' || req.path.startsWith('/auth/')) {
+      return errorPage(res, status, status === 413 ? 'Request too large' : 'Something went wrong', message);
+    }
+    res.status(status).json({ ok: false, message });
+  });
+
  return app;
 }

@@ -86,13 +86,15 @@ async function startFakeA2loft() {
 const fakeMinio = () => ({
  presignedGetObject: async (_bucket, name) => `https://storage.example/${name}`,
 });
-const fakeQueue = () => {
+const fakeQueue = (messageText) => {
  const calls = { getMessage: 0, deleteMessage: 0 };
  const item = {
    messageId: 'm-1',
    popReceipt: 'pop-1',
    dequeueCount: 1,
-    messageText: JSON.stringify({
+    messageText:
+      messageText ??
+      JSON.stringify({
        upload_id: 'up-1',
        file_name: 'id-domains/ucth/raw-pictures/x/img.jpg',
        caption: '08068287222',
@@ -155,7 +157,7 @@ async function startFakeNodered() {
  return { calls, server, stop: () => stopServer(server) };
 }

-async function boot({ cropAllowed = true, cropperClients } = {}) {
+async function boot({ cropAllowed = true, cropperClients, queue } = {}) {
  const portal = await startFakePortal({ cropAllowed });
  const a2loft = await startFakeA2loft();
  const removebg = await startFakeRemovebg();
@@ -182,7 +184,7 @@ async function boot({ cropAllowed = true, cropperClients } = {}) {
    fetch: testFetch,
    cropperClients: cropperClients !== undefined ? cropperClients : {
      minio: fakeMinio(),
-      queue: fakeQueue(),
+      queue: queue || fakeQueue(),
      noderedAuth: { username: 'nr', password: 'nr-secret' },
      carvekitKey: 'carve-key',
    },
@@ -418,3 +420,83 @@ test('baseUrl exposes the identity-prefixed a2loft base', async () => {
    await stop();
  }
 });
+
+
+test('malformed request bodies are rejected, never hang the request', async () => {
+  const { base, a2loft, stop } = await boot();
+  try {
+    const cookies = await login(base);
+    for (const path of ['/cropper/picture/review', '/cropper/dequeue', '/cropper/profile/picture?otp=1']) {
+      const res = await fetch(`${base}${path}`, {
+        method: 'POST',
+        headers: { cookie: cookies, 'content-type': 'application/json' },
+        body: JSON.stringify({}),
+      });
+      assert.equal(res.status, 400, `${path} → ${res.status}`);
+      const body = await res.json();
+      assert.equal(body.ok, false);
+    }
+    assert.equal(a2loft.calls.filter((c) => c.url.includes('/profile/picture')).length, 0);
+  } finally {
+    await stop();
+  }
+});
+
+test('a malformed queue item does not hang the request', async () => {
+  const { base, stop } = await boot({ queue: fakeQueue('not-json') });
+  try {
+    const cookies = await login(base);
+    const res = await fetch(`${base}/cropper/picture/next`, { headers: { cookie: cookies } });
+    assert.equal(res.status, 200);
+    const body = await res.json();
+    assert.equal(body.ok, false);
+    assert.match(body.message, /malformed/i);
+  } finally {
+    await stop();
+  }
+});
+
+test('save rejects an unknown domain without contacting a2loft', async () => {
+  const { base, a2loft, stop } = await boot();
+  try {
+    const cookies = await login(base);
+    const res = await fetch(`${base}/cropper/profile/picture?otp=778899`, {
+      method: 'POST',
+      headers: { cookie: cookies, 'content-type': 'application/json' },
+      body: JSON.stringify({
+        pictureDataUrl: 'data:image/jpeg;base64,QUJD',
+        phone: '08068287222',
+        message: {
+          pop_receipt: 'custom',
+          message_id: 'm-1',
+          message: { upload_id: 'up-1', domain: '../../admin', agentDeviceHash: 'a' },
+        },
+      }),
+    });
+    assert.equal(res.status, 400);
+    assert.equal(a2loft.calls.length, 0, 'a2loft never contacted');
+  } finally {
+    await stop();
+  }
+});
+
+test('picture fetches reject object keys outside the id-domains prefix', async () => {
+  const { base, stop } = await boot();
+  try {
+    const cookies = await login(base);
+    // encoded traversal survives as a single path segment; a plain ../ is
+    // normalised away by the HTTP client before the route ever sees it
+    const traversal = await fetch(`${base}/cropper/picture/..%2F..%2Fsecret`, { headers: { cookie: cookies } });
+    assert.equal(traversal.status, 400, `got ${traversal.status}`);
+    const escapeWithinPrefix = await fetch(`${base}/cropper/picture/id-domains_.._.._secret`, {
+      headers: { cookie: cookies },
+    });
+    assert.equal(escapeWithinPrefix.status, 400, `got ${escapeWithinPrefix.status}`);
+    const ok = await fetch(`${base}/cropper/picture/id-domains_ucth_raw-pictures_x_img.jpg`, {
+      headers: { cookie: cookies },
+    });
+    assert.equal(ok.status, 200);
+  } finally {
+    await stop();
+  }
+});
@@ -11,7 +11,7 @@ import {
 } from './helpers.js';

 // Fake YOUreka portal: web-auth session + openfga verify.
-async function startFakePortal({ cookieStatus = 'Activated', allowed = true } = {}) {
+async function startFakePortal({ cookieStatus = 'Activated', allowed = true, authErrorMessage } = {}) {
  return startServer((req, res) => {
    let raw = '';
    req.on('data', (c) => (raw += c));
@@ -23,6 +23,9 @@ async function startFakePortal({ cookieStatus = 'Activated', allowed = true } =
      if (req.method === 'POST' && req.url === '/api/web/auth') {
        const body = JSON.parse(raw || '{}');
        if (!body.returnUrl) return json(200, { ok: false, message: 'returnUrl is required' });
+        if (authErrorMessage) {
+          return json(200, { ok: false, message: authErrorMessage });
+        }
        return json(200, { ok: true, sessionId: 's1', loginUrl: 'https://portal.example/login?return=1' });
      }
      if (req.method === 'GET' && req.url.startsWith('/api/web/auth/s1/cookie')) {
@@ -263,3 +266,17 @@ test('proxy validation: auth, otp, and domain are enforced before contacting a2l
    await stop();
  }
 });
+
+
+test('upstream error text is HTML-escaped on the error page', async () => {
+  const { base, stop } = await bootApp({}, { authErrorMessage: '<script>alert(1)</script>portal said no' });
+  try {
+    const res = await fetch(`${base}/auth/login`, manual);
+    assert.equal(res.status, 502);
+    const html = await res.text();
+    assert.ok(!html.includes('<script>alert(1)</script>'), 'script tag must not be interpolated raw');
+    assert.match(html, /&lt;script&gt;alert\(1\)&lt;\/script&gt;/);
+  } finally {
+    await stop();
+  }
+});