From 102cef523ff2cb18fb600839dee165aeec34fbe3 Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Thu, 16 Jul 2026 21:03:34 -0400 Subject: [PATCH] fix: app id whitelisting (#3398) --- .../http/middleware/stepUpSession.test.ts | 54 +++++++++++++++++++ .../core/http/middleware/stepUpSession.ts | 26 +++++++-- src/backend/server.ts | 13 +++-- 3 files changed, 87 insertions(+), 6 deletions(-) diff --git a/src/backend/core/http/middleware/stepUpSession.test.ts b/src/backend/core/http/middleware/stepUpSession.test.ts index 9e3f1415a..e7fc0713f 100644 --- a/src/backend/core/http/middleware/stepUpSession.test.ts +++ b/src/backend/core/http/middleware/stepUpSession.test.ts @@ -177,6 +177,60 @@ describe('createStepUpGate', () => { expect(next.mock.calls[0][0]?.statusCode).toBe(403); }); + // App-gated routes (adminOnly + allowedAppIds): an admin acting through an + // allowlisted app can't elevate — apps have no password/TOTP and are blocked + // from /auth/elevate. The exemption keys off the token carrying an allowed + // app id, not the route flag. + it('exempts a token that carries an allowlisted app id', () => { + const ts = tokenService(); + const gate = createStepUpGate({ + tokenService: ts, + allowedAppUids: ['app-xyz'], + }); + const next = vi.fn(); + const req = reqWith(undefined, USER_UUID, { + actor: { user: { uuid: USER_UUID }, app: { uid: 'app-xyz' } }, + } as never); + gate(req, {} as Response, next); + expect(next).toHaveBeenCalledWith(); + }); + + it('still requires step-up for an app id NOT in the allowlist', () => { + const ts = tokenService(); + const gate = createStepUpGate({ + tokenService: ts, + allowedAppUids: ['other-app'], + }); + const next = vi.fn(); + const req = reqWith(undefined, USER_UUID, { + actor: { user: { uuid: USER_UUID }, app: { uid: 'app-xyz' } }, + } as never); + gate(req, {} as Response, next); + expect(next.mock.calls[0][0]?.statusCode).toBe(403); + }); + + it('still requires step-up when the route has no allowedAppIds', () => { + const ts = tokenService(); + const gate = createStepUpGate({ tokenService: ts }); + const next = vi.fn(); + const req = reqWith(undefined, USER_UUID, { + actor: { user: { uuid: USER_UUID }, app: { uid: 'app-xyz' } }, + } as never); + gate(req, {} as Response, next); + expect(next.mock.calls[0][0]?.statusCode).toBe(403); + }); + + it('still requires step-up on the human/root-token path (no app id in token)', () => { + const ts = tokenService(); + const gate = createStepUpGate({ + tokenService: ts, + allowedAppUids: ['app-xyz'], + }); + const next = vi.fn(); + gate(reqWith(undefined, USER_UUID), {} as Response, next); + expect(next.mock.calls[0][0]?.statusCode).toBe(403); + }); + it('accepts the elevation via the x-puter-elevation header (API clients)', () => { const ts = tokenService(); const gate = createStepUpGate({ tokenService: ts }); diff --git a/src/backend/core/http/middleware/stepUpSession.ts b/src/backend/core/http/middleware/stepUpSession.ts index 5c25c222b..f7c6cc65b 100644 --- a/src/backend/core/http/middleware/stepUpSession.ts +++ b/src/backend/core/http/middleware/stepUpSession.ts @@ -133,7 +133,7 @@ export function verifyStepUpSession( * Require an elevated session. Runs after the privilege gate it supplements * (`adminOnlyGate`), so it only adds the re-authentication requirement. * - * Deliberately unconditional — no exemptions, no environment check: + * Narrow by design — the only exemption is the app-gated path: * * - Not env-conditional, so the flow exercised locally is the one that ships. * - No carve-out for full-access tokens. That looks safe (a deliberately @@ -143,17 +143,37 @@ export function verifyStepUpSession( * - No carve-out based on how the credential arrived (cookie vs bearer). The * holder of a token chooses which header to put it in, so that distinction * is attacker-controlled and worthless as a gate. + * - `allowedAppUids`: the exemption is keyed off the *token*, not the route. + * An actor whose token carries one of these allowlisted app ids (an admin + * acting through an allowlisted app) is exempt — that actor can't elevate at + * all (apps have no password/TOTP and are blocked from `/auth/elevate`), so + * step-up is unsatisfiable for it. A token WITHOUT an allowlisted app id — a + * root/human session — still requires step-up, exactly as it would on a + * route with no `allowedAppIds`. So this is not a session or token-kind + * carve-out: reaching the exempt path needs an admin's OAuth grant to a + * specific allowlisted app. * - * The invariant: reaching a privileged endpoint requires proving the password - * or a TOTP code within the elevation's lifetime. Nothing else substitutes. + * The invariant for the human path: reaching a privileged endpoint requires + * proving the password or a TOTP code within the elevation's lifetime. * * A caller without a valid elevation proof is rejected with * `elevation_required`; `factor` tells the client which credential to collect. */ export function createStepUpGate(deps: { tokenService: TokenService; + allowedAppUids?: readonly string[]; }): RequestHandler { return (req, _res, next) => { + // Exempt only an actor whose token carries one of the route's + // allowlisted app ids: an admin acting through an allowlisted app can't + // elevate, so step-up is unsatisfiable for it. Any other actor — most + // importantly a root/human session with no app id in its token — falls + // through and must present the elevation proof. + const appUid = req.actor?.app?.uid; + if (appUid && deps.allowedAppUids?.includes(appUid)) { + next(); + return; + } if (verifyStepUpSession(req, deps)) { next(); return; diff --git a/src/backend/server.ts b/src/backend/server.ts index 5c5a276de..7a9a5977b 100644 --- a/src/backend/server.ts +++ b/src/backend/server.ts @@ -901,10 +901,17 @@ export class PuterServer { }), ); // An admin username on a leaked session isn't enough — also require - // a recent re-authentication (full-access tokens are exempt; see - // createStepUpGate). + // a recent re-authentication. Exempt only a token that carries one + // of the route's allowlisted app ids: an admin acting through an + // allowlisted app can't elevate (apps have no password/TOTP; see + // createStepUpGate). A root/human session — no app id in the token — + // still requires step-up, and `allowedAppIdsGate` still enforces the + // allowlist for the app path. mwChain.push( - createStepUpGate({ tokenService: this.services.token }), + createStepUpGate({ + tokenService: this.services.token, + allowedAppUids: opts.allowedAppIds, + }), ); }