fix: document allowedAppIdsGate's real non-app-gate behavior (PUT-1667) (#3669)
Maintain Release Merge PR / update-release-pr (push) Canceled after 0s
Notify HeyPuter / notify (push) Canceled after 0s
release-please / release-please (push) Canceled after 0s

The JSDoc claimed non-app actors are rejected; they are passed through, and
the check reads `actor.app` rather than `effectiveApp`, so an app in the
token chain is invisible to it. Callers depend on the pass-through — routes
combining `adminOnly` + `allowedAppIds` are reached with an admin's browser
session, and the dev-account surface is called from the desktop's own
session — so the docs are corrected instead of the behavior, on the gate and
on the `allowedAppIds` route option. Tests cover the gate per actor shape
(user session, worker, full-access token, app-issued access token,
app-under-user allowed/disallowed) and the admin composition.
This commit is contained in:
Daniel Salazar
2026-08-30 12:55:39 -07:00
committed by GitHub
parent afe46a17a0
commit 5065bcbf0a
3 changed files with 118 additions and 10 deletions
+90 -3
View File
@@ -766,15 +766,62 @@ describe('requireVerifiedAccount', () => {
// ── allowedAppIdsGate ───────────────────────────────────────────────
describe('allowedAppIdsGate', () => {
it('passes through when the actor has no app (user-only actor)', () => {
// The gate only narrows app-under-user actors; user-only actors
// are handled by `requireUserActorGate` separately.
it('passes a bare user session through (the gate is not an app gate)', () => {
// Routes rely on this: `adminOnly` + `allowedAppIds` has to stay
// reachable by an admin's root token, not only by an allowed app.
const got = runGate(allowedAppIdsGate(['app-allowed']), {
actor: { user: { uuid: 'u-1' } },
});
expect(got).toBeUndefined();
});
it('passes a worker session through', () => {
const got = runGate(allowedAppIdsGate(['app-allowed']), {
actor: {
user: { uuid: 'u-1' },
session: { uid: 'sess-1', kind: 'worker' },
},
});
expect(got).toBeUndefined();
});
it('passes a full-access personal access token through', () => {
// No app anywhere in the chain, so there is nothing to match.
const got = runGate(allowedAppIdsGate(['app-allowed']), {
actor: {
user: { uuid: 'u-1' },
accessToken: {
uid: 'tok-1',
fullAccess: true,
issuer: { user: { uuid: 'u-1' } },
},
},
});
expect(got).toBeUndefined();
});
it('passes any access token issued by an app, allowed or not', () => {
// The check reads `actor.app`, which an access-token actor never
// carries — its app lives on `effectiveApp`, one hop through the
// issuer. Both directions pass, which is why an appId-gated route
// cannot treat this gate as proof of app identity.
const gate = allowedAppIdsGate(['app-allowed']);
const tokenIssuedBy = (appUid: string) => ({
actor: {
user: { uuid: 'u-1' },
accessToken: {
uid: 'tok-1',
issuer: {
user: { uuid: 'u-1' },
app: { uid: appUid },
},
},
},
});
expect(runGate(gate, tokenIssuedBy('app-allowed'))).toBeUndefined();
expect(runGate(gate, tokenIssuedBy('app-other'))).toBeUndefined();
});
it('passes when the actor.app.uid is in the allow-list', () => {
const got = runGate(allowedAppIdsGate(['app-allowed']), {
actor: {
@@ -804,6 +851,46 @@ describe('allowedAppIdsGate', () => {
});
expectHttpError(got, 403, 'forbidden');
});
it('passes an anonymous request through (requireAuth rejects it first)', () => {
const got = runGate(allowedAppIdsGate(['app-allowed']), {});
expect(got).toBeUndefined();
});
// The composition the admin surfaces are wired with: `adminOnly` defers
// app actors here, so the pair means "root token OR allowed app". Both
// halves are load-bearing — the admin dashboard calls these routes with a
// browser session, the marketplace app with its own token.
describe('composed with adminOnlyGate({ appGated: true })', () => {
const runChain = (actor: Actor): NextArg => {
const first = runGate(adminOnlyGate([], { appGated: true }), {
actor,
});
if (first !== undefined) return first;
return runGate(allowedAppIdsGate(['app-allowed']), { actor });
};
it("admits an admin's root token", () => {
const got = runChain({ user: { uuid: 'u-1', username: 'admin' } });
expect(got).toBeUndefined();
});
it('admits an admin acting through an allowed app', () => {
const got = runChain({
user: { uuid: 'u-1', username: 'admin' },
app: { uid: 'app-allowed' },
});
expect(got).toBeUndefined();
});
it('rejects an admin acting through another app', () => {
const got = runChain({
user: { uuid: 'u-1', username: 'admin' },
app: { uid: 'app-other' },
});
expectHttpError(got, 403, 'forbidden');
});
});
});
// ── requirePhoneVerifiedGate / requireCardVerifiedGate ──────────────
+21 -6
View File
@@ -413,7 +413,8 @@ export const assertVerifiedAccount = (
*/
export const assertPhoneVerified = (
user:
{ phone?: unknown; requires_phone_verification?: unknown } | undefined,
| { phone?: unknown; requires_phone_verification?: unknown }
| undefined,
): void => {
if (user?.phone && !user?.requires_phone_verification) return;
throw new HttpError(403, 'Please verify your phone number to continue', {
@@ -474,18 +475,32 @@ export const assertNotSuspended = (
};
/**
* Reject unless the actor is acting through one of the named apps.
* App-under-user actors are permitted iff `actor.app.uid` is in the allowList;
* non-app actors are rejected.
* Narrow WHICH apps may reach a route: an app-under-user actor whose
* `actor.app.uid` is missing from the allowList is rejected with 403.
*
* Implies `requireAuth`. Doesn't pair sensibly with `requireUserActor` (a
* user-only actor has no app), but if both are set we reject loudly here.
* It is NOT an app gate, despite the name. Every actor with no `app` of its own
* passes through — browser sessions, full-access personal access tokens, worker
* sessions, and access tokens issued BY an app, since the check reads
* `actor.app` and never `actor.effectiveApp`, leaving an app in the token chain
* invisible here. Routes depend on that pass-through: `adminOnly` +
* `allowedAppIds` means "a root token OR a token scoped to an allowed app", and
* the admin and dev-account surfaces are called both ways.
*
* So use it to keep other apps out, never as proof that an app is present. The
* default-on `requireNonAccessTokenGate` is what keeps app-issued tokens off
* these routes; a route setting `allowAccessToken` loses that cover and has to
* check `effectiveApp` itself.
*
* Implies `requireAuth`. Pairing it with `requireUserActor` leaves the
* allowList dead — that gate rejects every app, and everything it admits passes
* here.
*/
export const allowedAppIdsGate = (
allowedAppUids: readonly string[],
): RequestHandler => {
const allowList = new Set(allowedAppUids);
return (req, _res, next) => {
// Deliberately `app`, not `effectiveApp`: see the note above.
const appUid = req.actor?.app?.uid;
if (appUid && !allowList.has(appUid)) {
next(
+7 -1
View File
@@ -168,8 +168,14 @@ export interface RouteOptions {
adminOnly?: boolean | string[];
/**
* Reject unless the actor is acting through one of these apps. Implies
* Reject an app-under-user actor whose app is not in this list. Implies
* `requireAuth`.
*
* Not an app gate: actors carrying no app of their own browser sessions,
* full-access personal access tokens, workers, and access tokens issued by
* an app pass through untouched. `adminOnly` + `allowedAppIds` relies on
* that. See `allowedAppIdsGate` before using this to scope a route to
* apps.
*/
allowedAppIds?: string[];