Skip to content

Commit 30fdb02

Browse files
committed
fix(observability-map): attribute auth-boundary guards per export
Every input auth-boundary read was entry-point-wide, so one guarded export spoke for the whole file: calleeNames is the union of both bodies, checkedCallees was too, and usesBuilder was an OR over both initializer callees. A file whose loader called requireUser and whose action called nothing read as guarded in the body, and a createLoaderApiRoute loader authenticated a hand-written action beside it. This is the same defect auth-scope was fixed for a round earlier, in its sibling check. scanFile now splits calleeNames, calleeTexts, checkedCallees, statementCount and hasTryCatch per export, filled from one push site each so the union and the split cannot drift apart. usesBuilder had no other caller and is gone. routeExports is the single enumeration of a file's exports, shared with auth-scope, which had grown its own [loader, action] literal. Triviality had to follow, or the fix trades a false pass for a false accusation: naive per-export attribution moved auth.github.ts and auth.google.ts to fail, both being a one-line redirect-stub loader beside a guarded action that the entry-point-wide rule called non-trivial. isTrivial is now one rule over two views. The per-export view matches the side-effect hints against that export's own callee paths: the whole file is defeatable (the corpus's log-caller-scope-userid puts the word logger in the file and un-excuses the untouched loader) and nothing at all guts the check (five fixtures go from fail to not-applicable, because calleeNames keeps only a call's last segment and prisma.x.findMany reads as findMany). login.mfa's action verifies a TOTP or recovery code, which is a login-surface proof of possession like the verify* guards already listed, so it joins them rather than being accused once its loader stops speaking for it. Real tree unmoved: global 19, 62 auth-boundary applicable, 59 passing, no route changing any check. scan.ts also picks up routeModuleFiles here, shared with the corpus harness, because it sits in the same hunk as the per-export return shape.
1 parent 4ea20b0 commit 30fdb02

10 files changed

Lines changed: 766 additions & 132 deletions

File tree

internal-packages/observability-map/src/checks/authBoundary.ts

Lines changed: 68 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,16 @@
11
import type { CheckResult, EntryPoint } from "../types.js";
22
import { classifySensitivity } from "../sensitivity.js";
3-
import { isTrivial } from "../triviality.js";
4-
import { usesBuilder } from "./errorClassification.js";
3+
import { routeExports, type ExportName, type RouteExport } from "../routeExports.js";
4+
import { isTrivialExport } from "../triviality.js";
5+
import { BUILDERS } from "./errorClassification.js";
56

67
const ID = "auth-boundary";
78

89
/**
9-
* The guard helpers this webapp actually has, matched against `calleeNames`, which is scoped to the
10-
* loader/action bodies and follows one hop into a same-file helper. A guard the route only imports
11-
* and never calls does not count.
10+
* The guard helpers this webapp actually has, matched against the calling export's own
11+
* `loaderCalleeNames`/`actionCalleeNames`, each scoped to that export's handlers and following one
12+
* hop into a same-file helper. A guard the route only imports and never calls does not count, and
13+
* neither does one the OTHER export calls.
1214
*
1315
* A name list rather than the three patterns it replaces, because all three over-matched and this
1416
* is the one check where a false pass hides a security gap:
@@ -66,16 +68,22 @@ export const GUARDS = new Set([
6668
// Local helpers, each declared inside the one route that uses it.
6769
"authenticateAdmin",
6870
"authenticatePlainRequest",
69-
// Proof of possession: a callback URL carrying an HMAC is authenticated by checking that HMAC.
71+
// Proof of possession: a callback URL carrying an HMAC is authenticated by checking that HMAC,
72+
// and a login-surface second factor is authenticated by checking the code presented.
73+
// `login.mfa`'s action is the second half of a login, so like `authenticate` above it establishes
74+
// identity from the credential rather than requiring an already authenticated caller. It reached
75+
// this list when per-export attribution stopped its loader's `isAuthenticated` speaking for it.
7076
"verifyHttpCallbackHash",
7177
"verifyWebhook",
7278
"verifyUserActorToken",
79+
"verifyTotpForLogin",
80+
"verifyRecoveryCodeForLogin",
7381
]);
7482

7583
/**
7684
* Guards that answer with null instead of throwing. Calling one is not evidence of a boundary,
77-
* because the route is free to ignore the answer, so these are only credited when the body
78-
* demonstrably reads what they returned (`EntryPoint.checkedCallees`).
85+
* because the route is free to ignore the answer, so these are only credited when THAT EXPORT's
86+
* handlers demonstrably read what they returned (`EntryPoint.loaderCheckedCallees`).
7987
*
8088
* The distinction is the whole reason this set is separate from `GUARDS`. `requireUserId` redirects
8189
* on its own, so calling it IS the boundary; `getUserId` hands back `string | null` and a route
@@ -85,10 +93,43 @@ export const GUARDS = new Set([
8593
* rather than something a hand-read established once.
8694
*
8795
* What it still cannot see is whether the test that reads the answer guards anything. See
88-
* `EntryPoint.checkedCallees` for the exact shape of that residual.
96+
* `EntryPoint.loaderCheckedCallees` for the exact shape of that residual.
8997
*/
9098
export const SOFT_GUARDS = new Set(["getUser", "getUserId"]);
9199

100+
type GuardedExport = { name: ExportName; guarded: boolean; how: string; export: RouteExport };
101+
102+
/**
103+
* The exports this file declares, each with its own verdict.
104+
*
105+
* Per export, because the exposure is per export, and this is the same defect `auth-scope` was
106+
* fixed for one round earlier. Every input here was entry-point-wide: `calleeNames` is the union of
107+
* both bodies, `checkedCallees` was too, and `usesBuilder` was an OR over the two initializer
108+
* callees. So a file whose loader called `requireUser` and whose action called nothing read as
109+
* "guarded in the body", and a file whose loader was `createLoaderApiRoute(...)` credited its
110+
* hand-written action with the builder's authentication. Three inputs, one bug, and it is a false
111+
* PASS on the one check where that hides a security gap.
112+
*
113+
* `routeExports` lists only the exports the file actually declares, so an export that calls nothing
114+
* at all is judged rather than skipped: an empty body is exactly the unguarded case. It is shared
115+
* with `auth-scope`, which grew its own copy of the same `[loader, action]` literal.
116+
*/
117+
function guardedExports(ep: EntryPoint): GuardedExport[] {
118+
return routeExports(ep).map((e) => {
119+
const verdict = (guarded: boolean, how: string) => ({ name: e.name, guarded, how, export: e });
120+
if (e.initializerCallee !== null && BUILDERS.has(e.initializerCallee)) {
121+
return verdict(true, "authenticated by the builder");
122+
}
123+
if (e.calleeNames.some((n) => GUARDS.has(n))) {
124+
return verdict(true, "guarded in the body");
125+
}
126+
if (e.checkedCallees.some((n) => SOFT_GUARDS.has(n))) {
127+
return verdict(true, "resolves the caller and reads the answer");
128+
}
129+
return verdict(false, "");
130+
});
131+
}
132+
92133
/**
93134
* Whether a route that handles credentials, tokens or money checks who is asking.
94135
*
@@ -123,26 +164,30 @@ export const authBoundary = {
123164
if (!sensitivity.sensitive) {
124165
return { id: ID, status: "not-applicable", detail: "not sensitive" };
125166
}
126-
if (usesBuilder(ep)) {
127-
return { id: ID, status: "pass", detail: "authenticated by the builder" };
128-
}
129-
if (ep.calleeNames.some((n) => GUARDS.has(n))) {
130-
return { id: ID, status: "pass", detail: "guarded in the body" };
131-
}
132-
if (ep.checkedCallees.some((n) => SOFT_GUARDS.has(n))) {
133-
return { id: ID, status: "pass", detail: "resolves the caller and reads the answer" };
134-
}
135-
if (isTrivial(ep)) {
167+
// Never empty: `scanFile` returns null unless the file declares a loader or an action.
168+
const exports = guardedExports(ep);
169+
const guarded = exports.filter((e) => e.guarded);
170+
// Triviality excuses per export, matching the attribution: the reasoning below is about one
171+
// body being the place a guard would have to be, and reading it entry-point-wide let a busy
172+
// action make a redirect-stub loader answerable for a guard it has nothing to guard.
173+
const accused = exports.filter((e) => !e.guarded && !isTrivialExport(e.export));
174+
if (accused.length > 0) {
136175
return {
137176
id: ID,
138-
status: "not-applicable",
139-
detail: "cannot verify: no privileged work in the body, any guard is behind an import",
177+
status: "fail",
178+
detail: `sensitive (${sensitivity.reasons.join(", ")}) with no auth guard in the body: ${accused
179+
.map((e) => e.name)
180+
.join(", ")}`,
140181
};
141182
}
183+
if (guarded.length > 0) {
184+
const how = [...new Set(guarded.map((e) => e.how))].join(" and ");
185+
return { id: ID, status: "pass", detail: how };
186+
}
142187
return {
143188
id: ID,
144-
status: "fail",
145-
detail: `sensitive (${sensitivity.reasons.join(", ")}) with no auth guard in the body`,
189+
status: "not-applicable",
190+
detail: "cannot verify: no privileged work in the body, any guard is behind an import",
146191
};
147192
},
148193
};

internal-packages/observability-map/src/checks/authScope.ts

Lines changed: 15 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { CheckResult, EntryPoint } from "../types.js";
22
import { classifySensitivity } from "../sensitivity.js";
3+
import { routeExports } from "../routeExports.js";
34
import { BUILDERS } from "./errorClassification.js";
45

56
const ID = "auth-scope";
@@ -12,30 +13,22 @@ type BuilderExport = { name: string; callee: string; scoped: boolean; why: strin
1213
* Per export, because the exposure is per export. `authorization` is declared on the builder call
1314
* one export made, and a caller filter is written in the handler one export runs, so neither says
1415
* anything about the other half of the file.
16+
*
17+
* The enumeration itself is `routeExports`, shared with `auth-boundary`, which had to be given the
18+
* same per-export treatment a round later and wrote a second copy of this literal to get it.
1519
*/
1620
function builderExports(ep: EntryPoint): BuilderExport[] {
17-
const all = [
18-
{
19-
name: "loader",
20-
callee: ep.loaderInitializerCallee,
21-
authorization: ep.loaderBuilderOptions.includes("authorization"),
22-
filters: ep.loaderScopesByCaller,
23-
},
24-
{
25-
name: "action",
26-
callee: ep.actionInitializerCallee,
27-
authorization: ep.actionBuilderOptions.includes("authorization"),
28-
filters: ep.actionScopesByCaller,
29-
},
30-
];
31-
return all
32-
.filter((e) => e.callee !== null && BUILDERS.has(e.callee))
33-
.map((e) => ({
34-
name: e.name,
35-
callee: e.callee!,
36-
scoped: e.authorization || e.filters,
37-
why: e.authorization ? "an authorization gate" : "a filter on the caller's identity",
38-
}));
21+
return routeExports(ep)
22+
.filter((e) => e.initializerCallee !== null && BUILDERS.has(e.initializerCallee))
23+
.map((e) => {
24+
const authorization = e.builderOptions.includes("authorization");
25+
return {
26+
name: e.name,
27+
callee: e.initializerCallee!,
28+
scoped: authorization || e.scopesByCaller,
29+
why: authorization ? "an authorization gate" : "a filter on the caller's identity",
30+
};
31+
});
3932
}
4033

4134
/**

internal-packages/observability-map/src/checks/errorClassification.ts

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -118,13 +118,6 @@ function swallows(clause: CatchEvidence): boolean {
118118
return !decides(clause) && !inert(clause);
119119
}
120120

121-
export function usesBuilder(ep: EntryPoint): boolean {
122-
return (
123-
(ep.loaderInitializerCallee !== null && BUILDERS.has(ep.loaderInitializerCallee)) ||
124-
(ep.actionInitializerCallee !== null && BUILDERS.has(ep.actionInitializerCallee))
125-
);
126-
}
127-
128121
/**
129122
* Who decides what a failure means, and on what evidence.
130123
*

0 commit comments

Comments
 (0)