openvibers/openvibe.rent Commits

Rent: a crafted sign-in state can no longer crash the process; /internal/ and /metrics refused case-insensitively (#5)

OP OpenVibers committed 10 Oct 2026 · 9abd7086e7 · 4 files, +39 −7

From an independent security review of Rent.

- **HIGH:** the sign-in callback compared the OAuth state's string length, then called `timingSafeEqual`. A multibyte state of the same string length made it throw inside an async handler nobody caught, and the process exited. Anyone could trigger it, signed out. Now:
  - byte lengths are compared first, and anything unequal or missing is refused with a 400;
  - the router catches async errors.
- **MEDIUM:** nginx refuses `/internal/` and `/metrics` case-insensitively; Express routes `/Internal/events` too.

Tests: `test/auth-ops.test.js` (a multibyte state is refused while the server keeps answering) and `test/nginx-auth-limit.test.js`. Suite: 18/18 files pass; the vhost passes `nginx -t`.

Changes

ddiff --git a/deploy/nginx/openvibe.rent.conf b/deploy/nginx/openvibe.rent.conf
iindex 1f00e71..1756c91 100644
---- a/deploy/nginx/openvibe.rent.conf
++++ b/deploy/nginx/openvibe.rent.conf
@@@ -99,11 +99,14 @@ server {
      }
  
      # ── Prometheus metrics: host-local scrapers only (http://127.0.0.1:5010/metrics) ──
--    location = /metrics { return 404; }
++    # Case-insensitive: Express matches routes case-insensitively, so a case-sensitive location would let
++    # /Metrics through to the app.
++    location ~* ^/metrics(/|$) { return 404; }
  
      # ── OpenVibe.Events' delivery route: loopback only (POST /internal/events) ──
      # Events posts to http://127.0.0.1:5010/internal/events directly. Never proxied from outside (ADR-033).
--    location /internal/ { return 404; }
++    # Case-insensitive: Express matches routes case-insensitively, so /Internal/events would otherwise reach the app.
++    location ~* ^/internal(/|$) { return 404; }
  
      # ── Pages, docs, robots.txt, sitemap, static assets ─────
      location / {
ddiff --git a/server/auth/sso.js b/server/auth/sso.js
iindex de75276..557dbf2 100644
---- a/server/auth/sso.js
++++ b/server/auth/sso.js
@@@ -14,8 +14,8 @@
   * or logged.
   */
  const crypto = require('crypto');
--const express = require('express');
  const cache = require('openvibe-shared/cache-policy');
++const { asyncRouter } = require('../http/router');
  
  const ACCESS_COOKIE = 'rent_at';
  const REFRESH_COOKIE = 'rent_rt';
@@@ -166,7 +166,9 @@ function createSso({ config, keys, fetchImpl = globalThis.fetch, now = () => Dat
      }
  
      function routes() {
--        const r = express.Router();
++        // asyncRouter, not express.Router: a rejected handler (e.g. a state check that throws) goes to the app's
++        // error handler instead of becoming an unhandled rejection that takes the process down.
++        const r = asyncRouter();
          const flowOpts = () => ({ ...cookieBase(), path: '/auth', maxAge: 10 * 60 * 1000 });
  
          r.get('/login', (req, res) => {
@@@ -189,8 +191,13 @@ function createSso({ config, keys, fetchImpl = globalThis.fetch, now = () => Dat
                  if ((flow && flow.silent) || error === 'login_required' || error === 'interaction_required') return res.redirect(next);
                  return res.status(400).type('text/plain').send(`Sign-in was not completed (${String(error).slice(0, 60)}).`);
              }
--            if (!flow || typeof flow.state !== 'string' || typeof flow.verifier !== 'string' || typeof state !== 'string'
--                || state.length !== flow.state.length || !crypto.timingSafeEqual(Buffer.from(state), Buffer.from(flow.state))) {
++            // Compare UTF-8 byte lengths, never the UTF-16 string lengths: a multibyte state of the same string
++            // length would make timingSafeEqual throw (RangeError on unequal buffer lengths). Any missing or
++            // non-string piece, or unequal byte lengths, refuses before timingSafeEqual is ever called.
++            const stateBuf = typeof state === 'string' ? Buffer.from(state) : null;
++            const flowBuf = flow && typeof flow.state === 'string' ? Buffer.from(flow.state) : null;
++            if (!flow || !flowBuf || typeof flow.verifier !== 'string' || !stateBuf
++                || stateBuf.length !== flowBuf.length || !crypto.timingSafeEqual(stateBuf, flowBuf)) {
                  return res.status(400).type('text/plain').send('Sign-in state did not match. Please try signing in again.');
              }
              if (!code) return res.status(400).type('text/plain').send('The Network sent no authorization code.');
ddiff --git a/test/auth-ops.test.js b/test/auth-ops.test.js
iindex 01d65a0..2a0ad54 100644
---- a/test/auth-ops.test.js
++++ b/test/auth-ops.test.js
@@@ -55,6 +55,22 @@ const { boot, check, done } = require('./helpers/boot');
          assert.strictEqual(noCookie.status, 400);
      });
  
++    await check('a multibyte state of the right string length is refused without taking the server down', async () => {
++        const login = await t.get('/auth/login');
++        const loc = new URL(login.headers.get('location'));
++        const flowCookie = login.headers.get('set-cookie').split(';')[0];
++        const realState = loc.searchParams.get('state');
++        // 32 'é' (U+00E9) are 32 UTF-16 code units, as the flow state, but 64 UTF-8 bytes. Comparing string
++        // lengths passed, then timingSafeEqual threw on the unequal buffers and, unhandled, killed the process.
++        const multibyte = 'é'.repeat(realState.length);
++        assert.strictEqual(multibyte.length, realState.length);
++        const r = await t.get(`/auth/callback?code=x&state=${encodeURIComponent(multibyte)}`, { cookie: flowCookie });
++        assert.strictEqual(r.status, 400, r.text);
++        assert.match(r.text, /state did not match/i);
++        const ping = await t.get('/api/v1/ping');
++        assert.strictEqual(ping.status, 200, 'the server keeps answering after the refusal');
++    });
++
      await check('next= only accepts same-site paths', async () => {
          const r = await t.get('/auth/login?next=//evil.example/x');
          const flow = decodeURIComponent(r.headers.get('set-cookie').match(/rent_oauth=([^;]+)/)[1]);
ddiff --git a/test/nginx-auth-limit.test.js b/test/nginx-auth-limit.test.js
iindex d1c33bb..5158200 100644
---- a/test/nginx-auth-limit.test.js
++++ b/test/nginx-auth-limit.test.js
@@@ -17,4 +17,10 @@ assert.match(auth, /limit_req zone=rent_auth burst=10 nodelay;/);
  for (const b of [probe, auth]) assert.match(b, /limit_req_status 429;/);
  const upstream = (b) => (b.match(/proxy_pass (\S+);/) || [])[1];
  assert.ok(upstream(probe) && upstream(probe) === upstream(auth), 'the same upstream');
--console.log('nginx auth limit: /auth/me on the API zone, 429 when limited');
++// /internal/ and /metrics are refused case-insensitively: Express matches routes case-insensitively, so a
++// case-sensitive location would let /Internal/events or /Metrics through to the app.
++assert.match(conf, /location ~\* \^\/internal\(\/\|\$\) \{ return 404; \}/);
++assert.match(conf, /location ~\* \^\/metrics\(\/\|\$\) \{ return 404; \}/);
++assert.ok(!/location = \/metrics \{/.test(conf), 'no case-sensitive exact /metrics location');
++assert.ok(!/location \/internal\/ \{/.test(conf), 'no case-sensitive /internal/ location');
++console.log('nginx auth limit: /auth/me on the API zone, 429 when limited; /internal and /metrics case-insensitively refused');