From 82aeb6e0e0c142c06c492b8840f7fa03b03b398c Mon Sep 17 00:00:00 2001 From: sunny-wego Date: Tue, 22 Sep 2026 12:17:12 +0800 Subject: [PATCH 1/4] test(api): pin the credential invariants in src/api.ts from scripts/ `src/api.ts` is 1,724 lines, is where every outbound credential is attached, and is not in CODEOWNERS. This repository is public, so a plausible-looking change to where or under what condition a credential is sent needs one ordinary reviewer rather than a release signer. Adding `src/api.ts` to CODEOWNERS is the wrong fix: it puts the three signers on a file that churns constantly, which is the bottleneck the deliberately missing `*` line exists to avoid. Instead, assert the credential SHAPE from `scripts/`, which is already owned. The gate and the thing that can weaken the gate then sit at different review bars: loosening credential attachment in `src/api.ts` fails this test, and silencing this test needs a signer. The same assertions in `src/api.test.ts` would buy nothing, because both files are unowned. Four invariants, over the parsed source of `src/api.ts`: 1. Every `Authorization` value is a template of exactly `Bearer ` plus one bare identifier, and that identifier is a parameter of the enclosing function - never a literal, never `process.env`, never module scope. 2. `x-wego-id-token` is set exactly once, in the THEN branch of `identityAssertion.allowed && identityAssertion.token`. 3. `refreshIdentityAssertion` assigns no property but `token`, and carries the previous state forward by spreading it. "Consent is never re-decided" was a comment; now it is a test. 4. A closed allowlist of every header name the file may attach, by `headers.set` and by inline `fetch` header object alike. This is the one that matters: it fails BY DEFAULT on anything new, so the credential path nobody has thought of yet goes red until a signer reads it. Parsed with `typescript` (already a devDependency, it backs `bun run typecheck`) rather than matched with regexes: invariants 2 and 3 are claims about SCOPE, and a `headers.set` moved one line out of a guard's block looks identical to grep. Every assertion was watched fail before being trusted - twelve mutations of `src/api.ts`, three per invariant, each reverted after: a hardcoded bearer literal, a module-scope token, a smuggled second interpolation; the id-token set degated, ungated, and duplicated into an `else`; a refresh that flips `allowed`, one that assigns it on a second line, one that drops the spread; a brand-new header, a dynamic header name, and a new credential added to an inline header object. All twelve went red. No behaviour in `src/api.ts` is changed by this commit, and every invariant holds on current `main`. The test's limits are written into its header, because a reader who over-trusts it is a worse outcome than not having it: it asserts what it asserts, a novel credential path can satisfy every rule, a reviewer still has to think, and it does not protect `src/api.ts` from a signer. Friction and detection, not prevention. Also owns `contract/openapi.json`, for REVIEW VISIBILITY and not for runtime reach - the comment says which, because "for security" without a named mechanism is not a reason. Nothing in that file executes: it is read at build time to emit `src/api-types.d.ts`, which is uncommitted, imported only with `import type`, and erased before it could reach the binary; endpoints come from the already-owned `src/target.ts`. What it does control is the api<->cli contract check itself, whose generator and drift test were both already owned while their input was not - and the CI drift step is `continue-on-error` by design, so a hand-edited contract warns and merges. At ~7,300 lines a changed enum or a relaxed `required` is invisible in review. Low urgency, recorded as such. Closes REPO-2 of the 2026-09-22 audit; refs wego/foundations#221. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017HfAf9uJdm3Q76vvk2Tidz Co-Authored-By: Claude --- .github/CODEOWNERS | 38 +++ scripts/api-credential-shape.test.ts | 370 +++++++++++++++++++++++++++ 2 files changed, 408 insertions(+) create mode 100644 scripts/api-credential-shape.test.ts diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index dcc98aa..aebc8dd 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -111,6 +111,44 @@ src/config-dir.ts @sunny-wego @yeouchien-wego @chuyeowego src/target.ts @sunny-wego @yeouchien-wego @chuyeowego src/config.ts @sunny-wego @yeouchien-wego @chuyeowego +# THE INPUT TO THE api<->cli CONTRACT CHECK. Owned for REVIEW VISIBILITY, not +# for runtime reach - and the distinction is the reason this comment exists. +# +# NOTHING IN THIS FILE EXECUTES. It is read at build time by +# `scripts/generate-api-types.ts`, which emits `src/api-types.d.ts`; that output +# is not committed, is imported only with `import type`, and is erased before a +# byte of it could reach the shipped binary. The endpoints the CLI actually +# talks to come from `src/target.ts`, which is owned above. So this is NOT the +# `bun.lock` case, and it is not claimed to be. +# +# What it does control is the contract CHECK ITSELF. Both halves of that chain +# are already owned - the generator and `scripts/ci-contract-drift.test.ts` - +# while the document they run against was not. That is backwards in the same +# way `bun.lock` was: you cannot own a check and leave its input open. +# +# - Checks A and C (`src/api-contract.ts`, under `bun run typecheck`) compare +# the CLI's Zod types against THIS file. Widening a request schema here +# makes Check C - the outbound-exactness half - stop objecting to a field +# the real API would reject. +# - Check B (`src/api-contract.test.ts`) walks THIS file to confirm the fields +# the CLI's behaviour depends on are still published. An edit here can make +# that walk pass for a path the API does not serve. +# - The CI drift step is `continue-on-error` with an explicit `exit 0` on +# every path, BY DESIGN (the API deploys on its own cadence). So a +# hand-edited contract earns a `::warning::` and merges. Nothing fails. +# +# The practical problem is size: at ~7,300 lines, a changed enum value, a +# relaxed `required`, or a dropped field is invisible in a diff nobody scrolls. +# A refresh commit is this file ALONE (`bun run api-contract:refresh`, per +# CONTRIBUTING), so the owner's question is a cheap one - "is this the +# generator's output, or did someone type it?" - which is exactly the review +# this cannot get today. +# +# LOW URGENCY, recorded as such. No credential and no executable path runs +# through here; the cost of being wrong is a weakened compile-time check, not a +# redirected token. +contract/openapi.json @sunny-wego @yeouchien-wego @chuyeowego + # Deliberately NOT owned, so the omissions read as decisions rather than gaps: # # - `src/commands.ts` and `src/index.ts` wire the login flow together, but they diff --git a/scripts/api-credential-shape.test.ts b/scripts/api-credential-shape.test.ts new file mode 100644 index 0000000..42b82bb --- /dev/null +++ b/scripts/api-credential-shape.test.ts @@ -0,0 +1,370 @@ +/** + * CREDENTIAL SHAPE: the rules `src/api.ts` follows when it attaches a credential + * to an outbound request, asserted against its source text. + * + * WHY THIS FILE IS IN `scripts/` AND NOT BESIDE `src/api.ts`. That is the whole + * point of it, so it goes first. + * + * `src/api.ts` is ~1,700 lines and is deliberately NOT in `.github/CODEOWNERS`. + * Owning it would put the three release signers on a file that churns + * constantly — the bottleneck the missing `*` line in that file exists to avoid. + * But it is also where every outbound credential is attached: the `Bearer` + * access token, and the `x-wego-id-token` identity assertion. This repository is + * public, so anyone may open a pull request, and a plausible-looking change to + * WHERE or UNDER WHAT CONDITION a credential is sent currently needs one + * ordinary reviewer. + * + * `scripts/` IS code-owned. Putting the guard here means the gate and the thing + * that can weaken the gate sit at different review bars: a pull request that + * loosens credential attachment in `src/api.ts` fails this test, and silencing + * this test requires a release signer. That is the protection CODEOWNERS on + * `api.ts` would give, without the bottleneck. + * + * The same assertions placed in `src/api.test.ts` would buy nothing — both files + * are unowned, so one pull request could weaken the gate AND edit its guard + * under a single ordinary review. + * + * WHAT THIS DOES *NOT* PROTECT. A future reader who over-trusts this file is a + * worse outcome than not having it, so, plainly: + * + * - **It asserts what it asserts.** These are four specific structural rules. + * A sufficiently novel credential path can be written to satisfy every one + * of them — a request built outside `fetchOrUnreachable`, a token folded + * into a URL or a request body, a credential handed to a helper in another + * module. None of that is caught here. + * - **A reviewer still has to think.** This narrows what can be done QUIETLY. + * It is friction and detection, not prevention. + * - **It does not protect `src/api.ts` from a release signer**, and is not + * meant to. A signer can change both files in one pull request. The threat + * model is an outsider's pull request seen by one ordinary reviewer. + * - **It reads source text, not behaviour.** It cannot tell you the token a + * parameter carries is the right one, only where the parameter came from. + * Behavioural coverage lives in `src/api.test.ts`; this is the structural + * half, and the two are not substitutes. + * + * WHY AN AST AND NOT A REGEX. Invariants 2 and 3 are claims about SCOPE — "this + * call is inside that `if`", "this function never assigns that property". Line + * proximity is not scope: a `headers.set` moved one line down, out of a guard's + * block, looks identical to grep, and is exactly the regression worth catching. + * `typescript` is already a devDependency (it backs `bun run typecheck`), so + * parsing costs nothing new. + * + * INVARIANT 4 IS THE ONE THAT MATTERS MOST. Invariants 1-3 pin gates we already + * know about. Invariant 4 — the allowlist of header names — catches the + * credential path nobody has thought of yet, because it fails BY DEFAULT on + * anything new: adding a header to `src/api.ts` goes red until somebody edits + * `scripts/`, which is to say until a release signer looks at it. If this file + * ever has to be cut down, cut everything before invariant 4. + */ +import { describe, expect, it } from "bun:test"; +import { readFileSync } from "node:fs"; +import ts from "typescript"; + +const SOURCE_PATH = "src/api.ts"; + +const sourceFile = ts.createSourceFile( + SOURCE_PATH, + readFileSync(SOURCE_PATH, "utf8"), + ts.ScriptTarget.Latest, + /* setParentNodes */ true, + ts.ScriptKind.TS, +); + +/** 1-based line of a node, so a failure names a place rather than a shape. */ +const lineOf = (node: ts.Node): number => + sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile)).line + 1; + +const where = (node: ts.Node): string => `${SOURCE_PATH}:${lineOf(node)}`; + +const normalize = (text: string): string => text.replace(/\s+/g, " ").trim(); + +/** Every node in the file, depth-first. */ +function* walk(node: ts.Node): Generator { + yield node; + for (const child of node.getChildren(sourceFile)) yield* walk(child); +} + +const allNodes = [...walk(sourceFile)]; + +/** The static text of a property name or a string-ish literal, or `undefined` + * when it is computed. A computed name defeats every allowlist below, so + * `undefined` is always treated as a failure rather than quietly skipped. */ +function staticName(node: ts.Node | undefined): string | undefined { + if (node === undefined) return undefined; + if (ts.isIdentifier(node)) return node.text; + if (ts.isStringLiteralLike(node)) return node.text; + return undefined; +} + +/** HTTP header names are case-insensitive on the wire, so every comparison here + * is too: `authorization` must not be a way around a rule about + * `Authorization`. */ +const headerKey = (name: string): string => name.toLowerCase(); + +/** Every `headers.set(...)` call. Narrow on purpose: `res.headers.get(...)` is a + * read of a RESPONSE and is not a credential decision. */ +const headerSetCalls = allNodes.filter( + (node): node is ts.CallExpression => + ts.isCallExpression(node) && + ts.isPropertyAccessExpression(node.expression) && + node.expression.name.text === "set" && + staticName(node.expression.expression) === "headers", +); + +/** Every property of an object literal that is itself the value of a `headers:` + * property — the header bag handed to `fetch` inline, as `authedJsonGet` and + * `authedJsonPost` both do. `{ ...init, headers }` is a shorthand whose value + * is the `Headers` instance already covered above, so it yields nothing. */ +const headerLiteralProps = allNodes + .filter( + (node): node is ts.PropertyAssignment => + ts.isPropertyAssignment(node) && + staticName(node.name) === "headers" && + ts.isObjectLiteralExpression(node.initializer), + ) + .flatMap((node) => + (node.initializer as ts.ObjectLiteralExpression).properties.filter( + (prop): prop is ts.PropertyAssignment => ts.isPropertyAssignment(prop), + ), + ); + +// --------------------------------------------------------------------------- +// Invariant 1 - every Authorization value is `Bearer ${}` +// --------------------------------------------------------------------------- + +/** Both ways a header can be set in this file, reduced to (name, value), then + * narrowed to the Authorization ones. */ +const authorizationSites: { name: ts.Node; value: ts.Expression }[] = [ + ...headerLiteralProps.map((prop) => ({ + name: prop.name as ts.Node, + value: prop.initializer, + })), + ...headerSetCalls.map((call) => ({ + name: call.arguments[0] as ts.Node, + value: call.arguments[1] as ts.Expression, + })), +].filter((site) => headerKey(staticName(site.name) ?? "") === "authorization"); + +describe("invariant 1: Authorization carries a Bearer token from a parameter", () => { + it("sets Authorization somewhere, so the rules below are exercised", () => { + // Without this, removing every call site would make the cases below + // vacuously green: `it.each([])` asserts nothing at all. + expect(authorizationSites.length).toBeGreaterThan(0); + }); + + it.each( + authorizationSites.map((site) => [where(site.value), site] as const), + )("%s is a template of exactly Bearer + one interpolation", (_label, site) => { + // A string literal here would be a hardcoded credential. A concatenation, + // a second span or a tail would let a prefix or suffix smuggle something + // else into the same header. + expect(ts.isTemplateExpression(site.value)).toBe(true); + const template = site.value as ts.TemplateExpression; + expect(template.head.text).toBe("Bearer "); + expect(template.templateSpans).toHaveLength(1); + expect(template.templateSpans[0]?.literal.text).toBe(""); + }); + + it.each( + authorizationSites.map((site) => [where(site.value), site] as const), + )("%s interpolates a bare parameter of the enclosing function", (_label, site) => { + const template = site.value as ts.TemplateExpression; + const interpolated = template.templateSpans[0]?.expression; + + // A bare identifier: not `process.env.X`, not `config.token`, not a call. + // The credential must be PASSED IN, so the decision about which token to + // send stays with the command that made it. + expect(interpolated !== undefined && ts.isIdentifier(interpolated)).toBe( + true, + ); + const name = (interpolated as ts.Identifier).text; + + // ...and passed in to THIS function. A module-scope `let accessToken` + // would satisfy the check above while turning the token into ambient state + // that anything in 1,700 lines can write. + const parameterNames = new Set(); + for ( + let scope: ts.Node | undefined = site.value; + scope !== undefined; + scope = scope.parent + ) { + if ( + ts.isFunctionDeclaration(scope) || + ts.isFunctionExpression(scope) || + ts.isArrowFunction(scope) || + ts.isMethodDeclaration(scope) + ) { + for (const param of scope.parameters) { + const paramName = staticName(param.name); + if (paramName !== undefined) parameterNames.add(paramName); + } + } + } + expect([...parameterNames]).toContain(name); + }); +}); + +// --------------------------------------------------------------------------- +// Invariant 2 - x-wego-id-token only inside the consent guard +// --------------------------------------------------------------------------- + +/** The guard the identity assertion must sit behind, normalized for whitespace + * so reformatting is free and reordering is not. `allowed` is the user's + * consent; `token` is the assertion itself. Both, or nothing is sent. */ +const CONSENT_GUARD = "identityAssertion.allowed && identityAssertion.token"; + +const idTokenSets = headerSetCalls.filter( + (call) => + headerKey(staticName(call.arguments[0]) ?? "") === "x-wego-id-token", +); + +describe("invariant 2: x-wego-id-token is sent only under the consent guard", () => { + it("is set exactly once", () => { + // One call site is what lets "inside the guard" be a complete statement + // about the file. A second would mean the rule below has to hold in two + // places and a reviewer has to notice both — the situation this prevents. + expect(idTokenSets.map(where)).toHaveLength(1); + }); + + it.each( + idTokenSets.map((call) => [where(call), call] as const), + )(`%s sits in the THEN branch of \`if (${CONSENT_GUARD})\``, (_label, call) => { + // Scope, not proximity. An `if` whose ELSE branch holds the call, or an + // `if` the call merely follows, must not count — so this walks the parent + // chain and requires the child to be on the `thenStatement` side. + const guards: string[] = []; + let node: ts.Node = call; + while (node.parent !== undefined) { + const parent: ts.Node = node.parent; + if (ts.isIfStatement(parent) && parent.thenStatement === node) { + guards.push(normalize(parent.expression.getText(sourceFile))); + } + node = parent; + } + expect(guards).toContain(CONSENT_GUARD); + }); +}); + +// --------------------------------------------------------------------------- +// Invariant 3 - a refresh never re-decides consent +// --------------------------------------------------------------------------- + +const refresh = allNodes.find( + (node): node is ts.FunctionDeclaration => + ts.isFunctionDeclaration(node) && + node.name?.text === "refreshIdentityAssertion", +); + +describe("invariant 3: refreshIdentityAssertion never assigns `allowed`", () => { + it("exists as a function declaration with a body", () => { + // If it is renamed or reshaped, the rules below stop applying SILENTLY. + // Failing here sends whoever did that to this file to say why. + expect(refresh?.body).toBeDefined(); + }); + + it("assigns no property other than `token`", () => { + // The consent decision is made once, by `setIdentityAssertion`, out of the + // login flow. A refresh only ever learns a NEW TOKEN. A refresh that could + // also flip `allowed` to `true` would send an identity assertion for a user + // who declined — a consent bypass that reads like a one-word tidy-up. + // + // Stated as an allowlist rather than as "no property named `allowed`": a + // computed key (`{ ["allow" + "ed"]: true }`) has no legitimate use in this + // three-line function, and an allowlist rejects it without having to guess + // at what it evaluates to. + const offenders = [...walk(refresh?.body as ts.Node)] + .filter((node) => { + if ( + ts.isPropertyAssignment(node) || + ts.isShorthandPropertyAssignment(node) + ) { + return staticName(node.name) !== "token"; + } + if ( + ts.isBinaryExpression(node) && + node.operatorToken.kind === ts.SyntaxKind.EqualsToken && + ts.isPropertyAccessExpression(node.left) + ) { + return node.left.name.text === "allowed"; + } + return false; + }) + .map((node) => `${where(node)}: ${normalize(node.getText(sourceFile))}`); + + expect(offenders).toEqual([]); + }); + + it("carries the previous state forward by spreading it", () => { + // The positive half of the rule above. Not spreading would be fail-CLOSED + // today (`allowed` would be absent, and absent is falsy), so this is not a + // security assertion standing on its own — it pins the MECHANISM, so the + // next person here reads "carry forward" rather than "re-derive". + const spreads = [...walk(refresh?.body as ts.Node)] + .filter(ts.isSpreadAssignment) + .map((node) => normalize(node.expression.getText(sourceFile))); + expect(spreads).toContain("identityAssertion"); + }); +}); + +// --------------------------------------------------------------------------- +// Invariant 4 - the closed set of header names +// --------------------------------------------------------------------------- + +/** + * EVERY header name `src/api.ts` may attach via `headers.set`, and what each one + * carries. This list is the point of the whole file: it fails by DEFAULT on + * anything new, so a header nobody anticipated cannot be added quietly. + * + * To add a header: add it here, in the same pull request, with a line saying + * what it carries. That edit is in `scripts/`, so it needs a release signer — + * which is the review a new outbound header deserves. + */ +const ALLOWED_SET_HEADERS: Record = { + "user-agent": "build identification; carries no user data", + "x-wego-session-id": "analytics uuid, pushed in per invocation", + "x-wego-client-id": "analytics uuid, pushed in per invocation", + "x-wego-app-version": "build version", + "x-wego-os-type": "machine fact, not stored telemetry", + "x-wego-os-version": "machine fact, not stored telemetry", + "x-wego-timezone": "utc offset, computed per request", + "x-wego-id-token": "THE identity assertion - gated by invariant 2", +}; + +/** The same closed set, for headers written inline into a `fetch` init object. + * Separate list because it is a separate mechanism: a new credential added + * there would never touch a `headers.set` call. */ +const ALLOWED_LITERAL_HEADERS: Record = { + authorization: "THE access token - shaped by invariant 1", + "content-type": "request body encoding, JSON on the POST path", +}; + +describe("invariant 4: no header leaves this file without being listed here", () => { + it("names every headers.set(...) with a static string", () => { + // `headers.set(name, value)` with a variable name would make the allowlist + // below unenforceable, so the allowlist starts by requiring names it can read. + const dynamic = headerSetCalls + .filter((call) => staticName(call.arguments[0]) === undefined) + .map((call) => `${where(call)}: ${normalize(call.getText(sourceFile))}`); + expect(dynamic).toEqual([]); + }); + + it("sets only allowlisted header names", () => { + const names = headerSetCalls.map((call) => + headerKey(staticName(call.arguments[0]) as string), + ); + expect([...new Set(names)].sort()).toEqual( + Object.keys(ALLOWED_SET_HEADERS).sort(), + ); + }); + + it("writes only allowlisted names into inline header objects", () => { + const names = headerLiteralProps + .map((prop) => staticName(prop.name)) + // A computed key inside a header bag is the same hole as a dynamic + // `headers.set` name. Surface it as an unlistable name rather than skip it. + .map((name) => (name === undefined ? "" : headerKey(name))); + expect([...new Set(names)].sort()).toEqual( + Object.keys(ALLOWED_LITERAL_HEADERS).sort(), + ); + }); +}); From 3967eac81302676a864f1abb4847cd8678fe1514 Mon Sep 17 00:00:00 2001 From: sunny-wego Date: Tue, 22 Sep 2026 12:29:10 +0800 Subject: [PATCH 2/4] test(api): close three bypasses in the credential-shape invariants Review found three ways to weaken `src/api.ts` while keeping this test green. Each is now a mutation in the harness, and each goes red. 1. A LOCAL COULD SHADOW THE PARAMETER. Invariant 1 compared identifier TEXT against the enclosing function's parameter names, so a block-scoped `const accessToken = process.env.WEGO_TOKEN ?? ""` inside the retry loop satisfied it while sourcing the credential from the environment. Name matching cannot answer a question about binding, so this now builds a `ts.Program` and resolves the interpolated identifier's SYMBOL: its declaration must be a parameter, and a parameter of the NEAREST enclosing function - not of an outer one it merely closes over. `noResolve` + `noLib` keep the program to this one file; `typescript` was already a devDependency, and `bun run test` runs only in unprivileged jobs (`ci-cli`, and `release-cli.yml`'s `prepare`, which holds neither `id-token: write` nor the store environment - `workflow-shape.test.ts` is what keeps that true). 2. HEADER WRITES THE COLLECTORS DID NOT SEE. `headers.set` was matched on a receiver named exactly `headers`, so a second bag - `const requestHeaders = new Headers(); requestHeaders.set("x-secret", token)` - was invisible, and the allowlist stayed satisfied. Receivers are now matched two ways (bound to a `new Headers(...)`, or a headerish name), and the number of header bags is itself asserted: exactly one, named `headers`. 3. INLINE HEADER BAGS COULD GO OPAQUE. The inline collector silently dropped `SpreadAssignment`, so `{ ...credentialHeaders, "Content-Type": ... }` added arbitrary headers with the name list unchanged; and a `headers:` whose value was not an object literal (`headers: buildHeaders(token)`) was skipped entirely. Both are now failures in their own right - a bag the collector cannot read is the case an allowlist is worth least in, so it errors rather than passes. Also requires `refreshIdentityAssertion` to actually assign its `token` PARAMETER. The previous allowlist ("no property but `token`") was satisfied by a function that assigned nothing, and by `token: identityAssertion.token`, which refreshes nothing. The mutation harness grew from 12 cases to 17. All 17 go red; `git diff src/api.ts` is clean afterwards. Still no runtime change to `src/api.ts`, and every invariant holds on current `main`. One review point taken as already satisfied rather than actioned: keeping `typescript` out of credential-bearing jobs. `release-cli.yml`'s `prepare` job, the only release job that runs `bun run test`, declares no `permissions` and no `environment`, so it inherits `contents: read` and can neither mint a Fulcio certificate nor reach the blob store; `sign` and `release` hold those and `needs:` prepare. `bun run typecheck` already executes the same package in that same job. Noted in the file header so the next reader does not have to re-derive it. Refs REPO-2, wego/foundations#221. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017HfAf9uJdm3Q76vvk2Tidz Co-Authored-By: Claude --- scripts/api-credential-shape.test.ts | 299 +++++++++++++++++++-------- 1 file changed, 217 insertions(+), 82 deletions(-) diff --git a/scripts/api-credential-shape.test.ts b/scripts/api-credential-shape.test.ts index 42b82bb..7ffbd04 100644 --- a/scripts/api-credential-shape.test.ts +++ b/scripts/api-credential-shape.test.ts @@ -1,6 +1,6 @@ /** * CREDENTIAL SHAPE: the rules `src/api.ts` follows when it attaches a credential - * to an outbound request, asserted against its source text. + * to an outbound request, asserted against its source. * * WHY THIS FILE IS IN `scripts/` AND NOT BESIDE `src/api.ts`. That is the whole * point of it, so it goes first. @@ -37,17 +37,30 @@ * - **It does not protect `src/api.ts` from a release signer**, and is not * meant to. A signer can change both files in one pull request. The threat * model is an outsider's pull request seen by one ordinary reviewer. - * - **It reads source text, not behaviour.** It cannot tell you the token a - * parameter carries is the right one, only where the parameter came from. - * Behavioural coverage lives in `src/api.test.ts`; this is the structural - * half, and the two are not substitutes. + * - **It reads structure, not behaviour.** It cannot tell you the token a + * parameter carries is the right one, only that a parameter is where it came + * from. Behavioural coverage lives in `src/api.test.ts`; this is the + * structural half, and the two are not substitutes. + * - **The header collectors recognise the forms this file uses.** They are + * written to fail closed — an unfamiliar header-bag shape is an error, not a + * skip (invariant 4) — but "fails closed on what it can see" is still not + * "sees everything". * - * WHY AN AST AND NOT A REGEX. Invariants 2 and 3 are claims about SCOPE — "this - * call is inside that `if`", "this function never assigns that property". Line - * proximity is not scope: a `headers.set` moved one line down, out of a guard's - * block, looks identical to grep, and is exactly the regression worth catching. - * `typescript` is already a devDependency (it backs `bun run typecheck`), so - * parsing costs nothing new. + * WHY A TYPE CHECKER AND NOT A REGEX. Invariants 2 and 3 are claims about SCOPE + * — "this call is inside that `if`", "this function never assigns that + * property". Line proximity is not scope: a `headers.set` moved one line down, + * out of a guard's block, looks identical to grep, and is exactly the regression + * worth catching. Invariant 1 is a claim about BINDING — "this token came from a + * parameter" — which name matching cannot answer, because a local + * `const accessToken = process.env.TOKEN` shadows a parameter of the same name + * and reads identically. So this resolves symbols rather than comparing text. + * + * `typescript` is already a devDependency (it backs `bun run typecheck`), and + * `noResolve`/`noLib` keep the program to this one file, so nothing new is + * installed and no dependency graph is walked. `bun run test` runs only in + * unprivileged jobs — `ci-cli`, and `release-cli.yml`'s `prepare`, which holds + * neither `id-token: write` nor the store environment. `workflow-shape.test.ts` + * is what keeps that true. * * INVARIANT 4 IS THE ONE THAT MATTERS MOST. Invariants 1-3 pin gates we already * know about. Invariant 4 — the allowlist of header names — catches the @@ -57,18 +70,22 @@ * ever has to be cut down, cut everything before invariant 4. */ import { describe, expect, it } from "bun:test"; -import { readFileSync } from "node:fs"; import ts from "typescript"; const SOURCE_PATH = "src/api.ts"; -const sourceFile = ts.createSourceFile( - SOURCE_PATH, - readFileSync(SOURCE_PATH, "utf8"), - ts.ScriptTarget.Latest, - /* setParentNodes */ true, - ts.ScriptKind.TS, -); +/** + * One file, no lib, no module resolution. The checker only ever has to answer + * questions about bindings declared inside `src/api.ts` itself, so following + * imports would cost seconds and buy nothing. + */ +const program = ts.createProgram([SOURCE_PATH], { + noResolve: true, + noLib: true, + target: ts.ScriptTarget.Latest, +}); +const checker = program.getTypeChecker(); +const sourceFile = program.getSourceFile(SOURCE_PATH) as ts.SourceFile; /** 1-based line of a node, so a failure names a place rather than a shape. */ const lineOf = (node: ts.Node): number => @@ -78,6 +95,10 @@ const where = (node: ts.Node): string => `${SOURCE_PATH}:${lineOf(node)}`; const normalize = (text: string): string => text.replace(/\s+/g, " ").trim(); +/** A node, with its location, as one string — the shape a failure message wants. */ +const cite = (node: ts.Node): string => + `${where(node)}: ${normalize(node.getText(sourceFile))}`; + /** Every node in the file, depth-first. */ function* walk(node: ts.Node): Generator { yield node; @@ -101,33 +122,105 @@ function staticName(node: ts.Node | undefined): string | undefined { * `Authorization`. */ const headerKey = (name: string): string => name.toLowerCase(); -/** Every `headers.set(...)` call. Narrow on purpose: `res.headers.get(...)` is a - * read of a RESPONSE and is not a credential decision. */ -const headerSetCalls = allNodes.filter( - (node): node is ts.CallExpression => - ts.isCallExpression(node) && - ts.isPropertyAccessExpression(node.expression) && - node.expression.name.text === "set" && - staticName(node.expression.expression) === "headers", +/** The nearest function-like ancestor — the scope whose parameters are in play. */ +function enclosingFunction(node: ts.Node): ts.SignatureDeclaration | undefined { + for (let scope = node.parent; scope !== undefined; scope = scope.parent) { + if ( + ts.isFunctionDeclaration(scope) || + ts.isFunctionExpression(scope) || + ts.isArrowFunction(scope) || + ts.isMethodDeclaration(scope) + ) { + return scope; + } + } + return undefined; +} + +/** Where an identifier is DECLARED, resolved through the checker rather than + * guessed from its text — a local can shadow a parameter of the same name. */ +function declarationOf(node: ts.Node): ts.Declaration | undefined { + const symbol = ts.isShorthandPropertyAssignment(node) + ? checker.getShorthandAssignmentValueSymbol(node) + : checker.getSymbolAtLocation(node); + return symbol?.declarations?.[0]; +} + +/** True when `node` resolves to a parameter of `fn` itself — not of an outer + * function it happens to close over, and not a local that shadows one. */ +function isParameterOf( + node: ts.Node, + fn: ts.SignatureDeclaration | undefined, +): boolean { + const declaration = declarationOf(node); + return ( + declaration !== undefined && + fn !== undefined && + ts.isParameter(declaration) && + declaration.parent === fn + ); +} + +// --------------------------------------------------------------------------- +// Header bags: what counts as one, and the rule that an unfamiliar one fails +// --------------------------------------------------------------------------- + +/** Every identifier bound to a `new Headers(...)`. `src/api.ts` builds exactly + * one, in `fetchOrUnreachable`; invariant 4 asserts that stays true, so a + * second bag cannot appear under a name these collectors do not know. */ +const headerBagNames = new Set( + allNodes + .filter( + (node): node is ts.VariableDeclaration => + ts.isVariableDeclaration(node) && + node.initializer !== undefined && + ts.isNewExpression(node.initializer) && + staticName(node.initializer.expression) === "Headers", + ) + .map((node) => staticName(node.name)) + .filter((name): name is string => name !== undefined), ); -/** Every property of an object literal that is itself the value of a `headers:` - * property — the header bag handed to `fetch` inline, as `authedJsonGet` and - * `authedJsonPost` both do. `{ ...init, headers }` is a shorthand whose value - * is the `Headers` instance already covered above, so it yields nothing. */ -const headerLiteralProps = allNodes - .filter( - (node): node is ts.PropertyAssignment => - ts.isPropertyAssignment(node) && - staticName(node.name) === "headers" && - ts.isObjectLiteralExpression(node.initializer), - ) - .flatMap((node) => - (node.initializer as ts.ObjectLiteralExpression).properties.filter( - (prop): prop is ts.PropertyAssignment => ts.isPropertyAssignment(prop), - ), +/** The trailing name of a `.set(...)` receiver: `headers` for `headers.set`, + * `headers` for `init.headers.set`, `requestHeaders` for `requestHeaders.set`. */ +function receiverName(expr: ts.Expression): string | undefined { + if (ts.isIdentifier(expr)) return expr.text; + if (ts.isPropertyAccessExpression(expr)) return expr.name.text; + return undefined; +} + +/** Every `.set(...)` call. Two ways in, because either + * alone leaves a hole: a bag called `requestHeaders` slips a `new Headers` + * rule if it is assigned rather than declared, and a bag called `h` slips a + * name rule. `res.headers.get(...)` is a READ of a response and is + * deliberately not here. */ +const headerSetCalls = allNodes.filter((node): node is ts.CallExpression => { + if (!ts.isCallExpression(node)) return false; + if (!ts.isPropertyAccessExpression(node.expression)) return false; + if (node.expression.name.text !== "set") return false; + const name = receiverName(node.expression.expression); + if (name === undefined) return false; + return headerBagNames.has(name) || /headers?$/i.test(name); +}); + +/** Every `headers:` property assignment, whatever its value is. Invariant 4 + * rejects the ones whose value is not a plain object literal rather than + * skipping them. */ +const headerProperties = allNodes.filter( + (node): node is ts.PropertyAssignment => + ts.isPropertyAssignment(node) && staticName(node.name) === "headers", +); + +/** Every member of an inline header object — INCLUDING spreads, which invariant + * 4 fails on, because dropping them here is how a collector goes quiet. */ +const inlineHeaderMembers = headerProperties + .filter((node) => ts.isObjectLiteralExpression(node.initializer)) + .flatMap( + (node) => (node.initializer as ts.ObjectLiteralExpression).properties, ); +const inlineHeaderProps = inlineHeaderMembers.filter(ts.isPropertyAssignment); + // --------------------------------------------------------------------------- // Invariant 1 - every Authorization value is `Bearer ${}` // --------------------------------------------------------------------------- @@ -135,7 +228,7 @@ const headerLiteralProps = allNodes /** Both ways a header can be set in this file, reduced to (name, value), then * narrowed to the Authorization ones. */ const authorizationSites: { name: ts.Node; value: ts.Expression }[] = [ - ...headerLiteralProps.map((prop) => ({ + ...inlineHeaderProps.map((prop) => ({ name: prop.name as ts.Node, value: prop.initializer, })), @@ -154,7 +247,7 @@ describe("invariant 1: Authorization carries a Bearer token from a parameter", ( it.each( authorizationSites.map((site) => [where(site.value), site] as const), - )("%s is a template of exactly Bearer + one interpolation", (_label, site) => { + )("%s is a template of Bearer plus exactly one interpolation", (_label, site) => { // A string literal here would be a hardcoded credential. A concatenation, // a second span or a tail would let a prefix or suffix smuggle something // else into the same header. @@ -167,40 +260,28 @@ describe("invariant 1: Authorization carries a Bearer token from a parameter", ( it.each( authorizationSites.map((site) => [where(site.value), site] as const), - )("%s interpolates a bare parameter of the enclosing function", (_label, site) => { + )("%s interpolates a parameter of the function that sends the request", (_label, site) => { const template = site.value as ts.TemplateExpression; const interpolated = template.templateSpans[0]?.expression; // A bare identifier: not `process.env.X`, not `config.token`, not a call. - // The credential must be PASSED IN, so the decision about which token to - // send stays with the command that made it. expect(interpolated !== undefined && ts.isIdentifier(interpolated)).toBe( true, ); - const name = (interpolated as ts.Identifier).text; - - // ...and passed in to THIS function. A module-scope `let accessToken` - // would satisfy the check above while turning the token into ambient state - // that anything in 1,700 lines can write. - const parameterNames = new Set(); - for ( - let scope: ts.Node | undefined = site.value; - scope !== undefined; - scope = scope.parent - ) { - if ( - ts.isFunctionDeclaration(scope) || - ts.isFunctionExpression(scope) || - ts.isArrowFunction(scope) || - ts.isMethodDeclaration(scope) - ) { - for (const param of scope.parameters) { - const paramName = staticName(param.name); - if (paramName !== undefined) parameterNames.add(paramName); - } - } - } - expect([...parameterNames]).toContain(name); + + // ...and one the CHECKER says is a parameter of the NEAREST enclosing + // function. Both halves earn their place: + // - resolved, not name-matched, because `const accessToken = + // process.env.TOKEN` shadows the parameter and reads identically; + // - nearest, not any ancestor, because a nested helper closing over an + // outer function's parameter is a different claim from this one. + // The point is that the credential is PASSED IN, so the decision about + // which token to send stays with the command that made it. + const fn = enclosingFunction(site.value); + expect({ + at: cite(site.value), + fromAParameter: isParameterOf(interpolated as ts.Identifier, fn), + }).toEqual({ at: cite(site.value), fromAParameter: true }); }); }); @@ -255,6 +336,8 @@ const refresh = allNodes.find( node.name?.text === "refreshIdentityAssertion", ); +const refreshNodes = refresh?.body === undefined ? [] : [...walk(refresh.body)]; + describe("invariant 3: refreshIdentityAssertion never assigns `allowed`", () => { it("exists as a function declaration with a body", () => { // If it is renamed or reshaped, the rules below stop applying SILENTLY. @@ -272,7 +355,7 @@ describe("invariant 3: refreshIdentityAssertion never assigns `allowed`", () => // computed key (`{ ["allow" + "ed"]: true }`) has no legitimate use in this // three-line function, and an allowlist rejects it without having to guess // at what it evaluates to. - const offenders = [...walk(refresh?.body as ts.Node)] + const offenders = refreshNodes .filter((node) => { if ( ts.isPropertyAssignment(node) || @@ -289,17 +372,40 @@ describe("invariant 3: refreshIdentityAssertion never assigns `allowed`", () => } return false; }) - .map((node) => `${where(node)}: ${normalize(node.getText(sourceFile))}`); + .map(cite); expect(offenders).toEqual([]); }); + it("assigns the `token` parameter it was handed", () => { + // The allowlist above is satisfied by a function that assigns NOTHING, and + // a refresh that quietly stops refreshing is its own bug — the CLI would go + // on presenting a stale assertion. So the token write is required, and + // required to be THE PARAMETER: `{ ...identityAssertion, token: somethingElse }` + // passes a name check and fails this one. + const assignsParameter = refreshNodes + .filter( + ( + node, + ): node is ts.PropertyAssignment | ts.ShorthandPropertyAssignment => + (ts.isPropertyAssignment(node) || + ts.isShorthandPropertyAssignment(node)) && + staticName(node.name) === "token", + ) + .some((node) => + ts.isShorthandPropertyAssignment(node) + ? isParameterOf(node, refresh) + : isParameterOf(node.initializer, refresh), + ); + expect(assignsParameter).toBe(true); + }); + it("carries the previous state forward by spreading it", () => { // The positive half of the rule above. Not spreading would be fail-CLOSED // today (`allowed` would be absent, and absent is falsy), so this is not a // security assertion standing on its own — it pins the MECHANISM, so the // next person here reads "carry forward" rather than "re-derive". - const spreads = [...walk(refresh?.body as ts.Node)] + const spreads = refreshNodes .filter(ts.isSpreadAssignment) .map((node) => normalize(node.expression.getText(sourceFile))); expect(spreads).toContain("identityAssertion"); @@ -311,9 +417,10 @@ describe("invariant 3: refreshIdentityAssertion never assigns `allowed`", () => // --------------------------------------------------------------------------- /** - * EVERY header name `src/api.ts` may attach via `headers.set`, and what each one - * carries. This list is the point of the whole file: it fails by DEFAULT on - * anything new, so a header nobody anticipated cannot be added quietly. + * EVERY header name `src/api.ts` may attach through a header bag's `.set`, and + * what each one carries. This list is the point of the whole file: it fails by + * DEFAULT on anything new, so a header nobody anticipated cannot be added + * quietly. * * To add a header: add it here, in the same pull request, with a line saying * what it carries. That edit is in `scripts/`, so it needs a release signer — @@ -332,19 +439,27 @@ const ALLOWED_SET_HEADERS: Record = { /** The same closed set, for headers written inline into a `fetch` init object. * Separate list because it is a separate mechanism: a new credential added - * there would never touch a `headers.set` call. */ + * there would never touch a `.set` call. */ const ALLOWED_LITERAL_HEADERS: Record = { authorization: "THE access token - shaped by invariant 1", "content-type": "request body encoding, JSON on the POST path", }; describe("invariant 4: no header leaves this file without being listed here", () => { - it("names every headers.set(...) with a static string", () => { + it("builds exactly one header bag, under a name the collectors know", () => { + // The collectors recognise `.set` on a `new Headers` binding or on a + // headerish name. The NUMBER of bags is worth pinning on its own: one bag + // is why "every header this file sends" is a list somebody can finish + // reading, and a second one is a second place to look. + expect([...headerBagNames]).toEqual(["headers"]); + }); + + it("names every header-bag .set(...) with a static string", () => { // `headers.set(name, value)` with a variable name would make the allowlist // below unenforceable, so the allowlist starts by requiring names it can read. const dynamic = headerSetCalls .filter((call) => staticName(call.arguments[0]) === undefined) - .map((call) => `${where(call)}: ${normalize(call.getText(sourceFile))}`); + .map(cite); expect(dynamic).toEqual([]); }); @@ -357,11 +472,31 @@ describe("invariant 4: no header leaves this file without being listed here", () ); }); + it("builds every inline header bag as a plain object literal", () => { + // `headers: credentialHeaders` or `headers: buildHeaders(token)` moves the + // decision somewhere this file cannot see. Fail rather than skip: a bag the + // collector cannot read is the case an allowlist is worth least in. + const opaque = headerProperties + .filter((node) => !ts.isObjectLiteralExpression(node.initializer)) + .map(cite); + expect(opaque).toEqual([]); + }); + + it("writes every inline header as a named property, never a spread", () => { + // `{ ...credentialHeaders, "Content-Type": "application/json" }` leaves the + // name list below unchanged while adding any header it likes. A spread is + // therefore a failure in its own right, not a member the collector drops. + const unreadable = inlineHeaderMembers + .filter((member) => !ts.isPropertyAssignment(member)) + .map(cite); + expect(unreadable).toEqual([]); + }); + it("writes only allowlisted names into inline header objects", () => { - const names = headerLiteralProps + const names = inlineHeaderProps .map((prop) => staticName(prop.name)) // A computed key inside a header bag is the same hole as a dynamic - // `headers.set` name. Surface it as an unlistable name rather than skip it. + // `.set` name. Surface it as an unlistable name rather than skip it. .map((name) => (name === undefined ? "" : headerKey(name))); expect([...new Set(names)].sort()).toEqual( Object.keys(ALLOWED_LITERAL_HEADERS).sort(), From fe27f93a1f46c6459b9fb9abd3a7c1d0fcc0263e Mon Sep 17 00:00:00 2001 From: sunny-wego Date: Tue, 22 Sep 2026 13:06:52 +0800 Subject: [PATCH 3/4] test(api): guard the request chokepoint, not just the header payload Invariants 1-4 all describe the header bag of a request that already goes through `fetchOrUnreachable`. That left the largest hole in the file open: a request that never goes through it at all. await fetch("https://collector.example/ingest", { headers: { Authorization: `Bearer ${accessToken}` }, }); Every earlier rule passes on that line. The header name is allowlisted, the value is a `Bearer` template, and the token is a parameter of the enclosing function. It also sends the user's access token to a host nobody chose - the most severe thing in REPO-2's threat model, and the one thing the first version of this test did not catch. The allowlists guard the payload at the door; this guards the door. 5a - REACHABILITY. `src/api.ts` has exactly one outbound call site, and it is reached from exactly two places. All three facts are now asserted: the global `fetch` is referenced only as the injected `HttpFetch` default; the chokepoint issues exactly one request, through that injected parameter rather than a module-scope binding; and the set of functions calling `fetchOrUnreachable` is a closed allowlist (`authedJsonGet`, `authedJsonPost`). A new request path - a new endpoint, a retry helper, a "quick" health check - has to add its name in `scripts/`, in front of a release signer, before it can exist. 5b - TOKEN FLOW. An `accessToken` parameter may only be FORWARDED into another parameter also named `accessToken`, or interpolated as the sole span of the `Bearer ` template. Anything else fails: a token concatenated into a URL, packed into a JSON body, or handed to an unrelated function never touches a `headers.set` and never touches an `Authorization` property, so invariants 1-4 are all silent on it. Header rules cannot see a token in a query string. `src/target.ts` and `src/config.ts` - both already code-owned - decide which HOST may receive a credential and refuse cleartext. 5a is what stops `src/api.ts` from going around them. Six new mutations, all watched fail: a raw `fetch` to a hardcoded host carrying the token; an unlisted new caller of the chokepoint; the chokepoint bypassing its injected transport; the token concatenated into a query string; the token packed into the request body; the token handed to `readApiError`. The full harness is now 23 mutations and all 23 go red. `git diff src/api.ts` is clean afterwards, and every invariant holds on current `main`. One resolver bug found by that exercise and fixed here: in `{ ...body, accessToken }` the identifier is the PROPERTY's name, so asking the checker about it returns the property symbol rather than the variable being read. `declarationOf` now resolves through the shorthand node. Without it, mutation 5b2 - a token packed into a request body - resolved to nothing and was silently skipped, which is the worst failure mode a rule like this has: green because it looked away. Context for the scope change: this guard was originally sized against an audit premise I had not tested. `wego/cli` is open source and takes outside contributions, so the premise holds and the chokepoint is worth pinning properly. Refs REPO-2, wego/foundations#221. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017HfAf9uJdm3Q76vvk2Tidz Co-Authored-By: Claude --- scripts/api-credential-shape.test.ts | 202 ++++++++++++++++++++++++++- 1 file changed, 199 insertions(+), 3 deletions(-) diff --git a/scripts/api-credential-shape.test.ts b/scripts/api-credential-shape.test.ts index 7ffbd04..9aba374 100644 --- a/scripts/api-credential-shape.test.ts +++ b/scripts/api-credential-shape.test.ts @@ -140,9 +140,18 @@ function enclosingFunction(node: ts.Node): ts.SignatureDeclaration | undefined { /** Where an identifier is DECLARED, resolved through the checker rather than * guessed from its text — a local can shadow a parameter of the same name. */ function declarationOf(node: ts.Node): ts.Declaration | undefined { - const symbol = ts.isShorthandPropertyAssignment(node) - ? checker.getShorthandAssignmentValueSymbol(node) - : checker.getSymbolAtLocation(node); + // `{ ...rest, accessToken }` is the case worth spelling out. The identifier + // there is the PROPERTY's name, and asking the checker about it returns the + // property symbol — not the variable being read. Resolving through the + // shorthand node is what gets the value, and without this a token packed + // into a request body resolves to nothing and is silently skipped. + const target = + ts.isIdentifier(node) && ts.isShorthandPropertyAssignment(node.parent) + ? node.parent + : node; + const symbol = ts.isShorthandPropertyAssignment(target) + ? checker.getShorthandAssignmentValueSymbol(target) + : checker.getSymbolAtLocation(target); return symbol?.declarations?.[0]; } @@ -503,3 +512,190 @@ describe("invariant 4: no header leaves this file without being listed here", () ); }); }); + +// --------------------------------------------------------------------------- +// Invariant 5 - the request chokepoint, and where a token may travel +// --------------------------------------------------------------------------- + +/** + * WHY THIS ONE EXISTS, AND WHY IT IS NOT LAST IN IMPORTANCE. + * + * Invariants 1-4 all describe the header bag of a request that already goes + * through `fetchOrUnreachable`. That leaves the largest hole in the file wide + * open: a request that never goes through it at all. + * + * await fetch("https://collector.example/ingest", { + * headers: { Authorization: `Bearer ${accessToken}` }, + * }); + * + * That line satisfies every rule above — the header name is allowlisted, the + * value is a `Bearer` template, and the token is a parameter of the enclosing + * function. It also sends the user's access token to a host nobody chose. The + * allowlists guard the payload at the door; this one guards the door. + * + * So the claim here is about REACHABILITY, in two halves: + * + * a. Exactly one thing in this file issues a network request, every caller of + * it is named, and the set of callers is closed. A new request path is a + * `scripts/` edit — that is, a release signer — before it can exist. + * b. An access token may only ever be FORWARDED to another `accessToken` + * parameter, or interpolated into the `Bearer` header. It may not enter a + * URL, a query string, a request body, a log line, or any other call. + * Header rules cannot see a token in a query string; this can. + * + * `src/target.ts` and `src/config.ts` — both already code-owned — decide which + * HOST a token may reach and refuse cleartext. This is what stops `src/api.ts` + * from going around them. + * + * LIMIT, stated because the rest of this file states its limits. Rule (b) + * tracks the `accessToken` parameters by name and by symbol. It does not follow + * a token through a local alias (`const t = accessToken`) — that is caught only + * because the alias would then have to reach the wire through a call this rule + * already refuses. It does not track `identityAssertion.token`, which is module + * state rather than a parameter; invariant 2 pins its one use. + */ + +/** The only function permitted to issue a request, and the closed set of + * callers allowed to reach it. Adding a caller means adding it here. */ +const REQUEST_CHOKEPOINT = "fetchOrUnreachable"; +const ALLOWED_CHOKEPOINT_CALLERS: Record = { + authedJsonGet: "the shared authenticated GET envelope", + authedJsonPost: "the shared authenticated POST envelope", +}; + +const chokepoint = allNodes.find( + (node): node is ts.FunctionDeclaration => + ts.isFunctionDeclaration(node) && node.name?.text === REQUEST_CHOKEPOINT, +); + +/** The name of the nearest enclosing function, for grouping call sites. */ +function enclosingFunctionName(node: ts.Node): string { + const fn = enclosingFunction(node); + if (fn === undefined) return ""; + const name = (fn as ts.FunctionDeclaration).name; + return name === undefined ? "" : name.text; +} + +describe("invariant 5a: exactly one door, and a closed list of who may use it", () => { + it(`declares ${REQUEST_CHOKEPOINT} as a function`, () => { + expect(chokepoint).toBeDefined(); + }); + + it("references the global fetch only as the injected default", () => { + // `http: HttpFetch = fetch` is the ONE permitted mention: it is what makes + // the transport swappable in tests. Any other `fetch` in this file is a + // second door — a request that skips every header rule above, and skips + // `src/target.ts`'s decision about which host may receive a credential. + const strayFetch = allNodes + .filter( + (node): node is ts.Identifier => + ts.isIdentifier(node) && node.text === "fetch", + ) + .filter((node) => !ts.isParameter(node.parent)) + .map(cite); + expect(strayFetch).toEqual([]); + }); + + it("issues exactly one request, through its injected transport", () => { + // Inside the chokepoint, the call that reaches the network must be the + // INJECTED parameter, not a module-scope binding and not a fresh import. + const viaInjectedTransport = ( + chokepoint?.body === undefined ? [] : [...walk(chokepoint.body)] + ) + .filter(ts.isCallExpression) + .filter((call) => isParameterOf(call.expression, chokepoint)); + expect(viaInjectedTransport.map(where)).toHaveLength(1); + }); + + it("is reached only from the allowlisted callers", () => { + // The fails-by-default half. A new request path — a new endpoint, a retry + // helper, a "quick" health check — has to add its name here, in `scripts/`, + // which is to say in front of a release signer. + const callers = allNodes + .filter( + (node): node is ts.CallExpression => + ts.isCallExpression(node) && + staticName(node.expression) === REQUEST_CHOKEPOINT, + ) + .map(enclosingFunctionName); + expect([...new Set(callers)].sort()).toEqual( + Object.keys(ALLOWED_CHOKEPOINT_CALLERS).sort(), + ); + }); +}); + +/** Every parameter in the file named `accessToken` — the credential's only + * legitimate carrier. */ +const accessTokenParams = allNodes.filter( + (node): node is ts.ParameterDeclaration => + ts.isParameter(node) && staticName(node.name) === "accessToken", +); + +/** True when `node` is an argument of a call whose callee declares that same + * position as an `accessToken` parameter — i.e. the token is being FORWARDED, + * not consumed. `logDebug(accessToken)` fails: `logDebug`'s parameter is not + * called `accessToken`, so the token would be leaving its lane. */ +function isForwardedToAnAccessTokenParameter(node: ts.Node): boolean { + const call = node.parent; + if (!ts.isCallExpression(call)) return false; + const index = call.arguments.indexOf(node as ts.Expression); + if (index < 0) return false; + const callee = declarationOf(call.expression); + if (callee === undefined || !ts.isFunctionLike(callee)) return false; + return staticName(callee.parameters[index]?.name) === "accessToken"; +} + +/** True when `node` is the single interpolation of a `Bearer ` template — + * the one place the token is allowed to become text. */ +function isTheBearerInterpolation(node: ts.Node): boolean { + const span = node.parent; + if (span === undefined || !ts.isTemplateSpan(span)) return false; + const template = span.parent; + return ( + ts.isTemplateExpression(template) && + template.head.text === "Bearer " && + template.templateSpans.length === 1 + ); +} + +describe("invariant 5b: an access token is only forwarded, or put in the header", () => { + const references = allNodes + .filter( + (node): node is ts.Identifier => + ts.isIdentifier(node) && node.text === "accessToken", + ) + // The declarations themselves are not uses. + .filter((node) => !ts.isParameter(node.parent)) + // Only the ones that actually resolve to an `accessToken` PARAMETER; a + // same-named local is invariant 1's problem, not this one's. + .filter((node) => { + const declaration = declarationOf(node); + return ( + declaration !== undefined && + accessTokenParams.includes(declaration as ts.ParameterDeclaration) + ); + }); + + it("has references to check, so the rule below is not vacuous", () => { + expect(references.length).toBeGreaterThan(0); + }); + + it("never lets a token reach a URL, a body, or any other call", () => { + // This is the rule the header allowlists cannot express. A token + // concatenated into a URL, packed into a JSON body, handed to a logger, or + // stringified into a cache key never touches a `headers.set` and never + // touches an `Authorization` property — so invariants 1-4 would all pass. + // + // Two permitted shapes, and nothing else: + // - forwarded into another function's `accessToken` parameter; + // - interpolated as the sole span of the `Bearer ` template. + const escaped = references + .filter( + (node) => + !isForwardedToAnAccessTokenParameter(node) && + !isTheBearerInterpolation(node), + ) + .map(cite); + expect(escaped).toEqual([]); + }); +}); From f52b88b4395265f3ff71e4d674e121e7cf859c3c Mon Sep 17 00:00:00 2001 From: sunny-wego Date: Tue, 22 Sep 2026 17:21:12 +0800 Subject: [PATCH 4/4] test(api): keep only the credential rules a behavioural test cannot make `src/api.test.ts` already tested most of what this file asserted, and tested it better. It drives the real functions through the injected `HttpFetch` and reads the `Headers` that come out: - `src/api.test.ts:79` - the Bearer header carries the token it was passed - `src/api.test.ts:160` - a declining user sends no id-token - `src/api.test.ts:173` - "a refresh cannot turn the header on for someone who opted out" Those are the same claims the old invariants 1, 2 and 3 made by parsing source text, and the behavioural versions are strictly better: they survive refactors. The structural ones did not. A probe of behaviour-PRESERVING edits failed four of five - an equivalent refresh written without a spread, a rename of the header bag, a rename of the token parameter, and reordering the two halves of the consent guard. Each of those is correct code that went red. A guard that fails correct work teaches people to edit the guard instead of reading it, and then it is decoration. So this file now keeps only what a behavioural test STRUCTURALLY cannot say - the claims about absence, which you cannot make by calling a function and watching what comes back: - no header leaves this file that is not on a list; - there is exactly one place that issues a request, reached from a closed set of callers; - a credential never reaches a URL, a body, or any other call. Plus one deliberate exception: consent is never re-decided. That IS behavioural and IS tested in `src/api.test.ts`, but that file is unowned, so one pull request could flip the behaviour and delete the test that noticed, and every other rule here would stay green. Silently sending an identity assertion for a user who declined is the quietest bad change available in `src/api.ts`, so it gets a second, owned guard. The reason is written at that block. 23 assertions -> 13. The rules that remain are stated as properties rather than spellings: the guard is a SET of conjuncts (`a && b` and `b && a` both pass); `allowed` may be READ BACK but never given a new value; the header bag is pinned by COUNT, not by name; and the credential parameters are DERIVED from the `Bearer` template by closure instead of being matched on the identifier `accessToken`, so a rename no longer switches the rule off. Verified both directions, because a simplification is only safe if the coverage survives it: - all 22 mutations run against BOTH suites. Every one is still caught by at least one. The five the structural file no longer catches (hardcoded bearer literal, module-scope token, extra interpolation, a shadowing local, a refresh assigning a non-parameter) are all caught by `src/api.test.ts` - which is the evidence they were duplication, not coverage. The nine that `src/api.test.ts` cannot catch (a new header, a dynamic header name, a new inline credential, a second header bag, a new request path, a token in a URL / a body / another call) are exactly what this file is for. - the false-positive probe is now 0 of 7. Adding a genuinely new header is still red, by design. Refs REPO-2, wego/foundations#221. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017HfAf9uJdm3Q76vvk2Tidz Co-Authored-By: Claude --- scripts/api-credential-shape.test.ts | 794 ++++++++++++--------------- 1 file changed, 340 insertions(+), 454 deletions(-) diff --git a/scripts/api-credential-shape.test.ts b/scripts/api-credential-shape.test.ts index 9aba374..ceb0dd5 100644 --- a/scripts/api-credential-shape.test.ts +++ b/scripts/api-credential-shape.test.ts @@ -1,84 +1,76 @@ /** - * CREDENTIAL SHAPE: the rules `src/api.ts` follows when it attaches a credential - * to an outbound request, asserted against its source. + * CREDENTIAL SHAPE: the things about `src/api.ts` that a behavioural test + * cannot say, asserted against its source. * - * WHY THIS FILE IS IN `scripts/` AND NOT BESIDE `src/api.ts`. That is the whole - * point of it, so it goes first. + * WHY THIS FILE IS IN `scripts/`. That is the point of it, so it goes first. * - * `src/api.ts` is ~1,700 lines and is deliberately NOT in `.github/CODEOWNERS`. - * Owning it would put the three release signers on a file that churns - * constantly — the bottleneck the missing `*` line in that file exists to avoid. - * But it is also where every outbound credential is attached: the `Bearer` - * access token, and the `x-wego-id-token` identity assertion. This repository is - * public, so anyone may open a pull request, and a plausible-looking change to - * WHERE or UNDER WHAT CONDITION a credential is sent currently needs one - * ordinary reviewer. + * `src/api.ts` attaches every outbound credential — the `Bearer` access token + * and the `x-wego-id-token` identity assertion — and is deliberately NOT in + * `.github/CODEOWNERS`: owning ~1,700 churning lines would put the three + * release signers on nearly every pull request, the bottleneck the missing `*` + * line exists to avoid. This repository is public and takes outside + * contributions, so a plausible-looking change to where or under what condition + * a credential is sent otherwise needs one ordinary reviewer. * - * `scripts/` IS code-owned. Putting the guard here means the gate and the thing - * that can weaken the gate sit at different review bars: a pull request that - * loosens credential attachment in `src/api.ts` fails this test, and silencing - * this test requires a release signer. That is the protection CODEOWNERS on - * `api.ts` would give, without the bottleneck. + * `scripts/` IS code-owned, so the gate and the thing that can weaken the gate + * sit at different review bars: loosening credential handling in `src/api.ts` + * fails this file, and silencing this file needs a release signer. * - * The same assertions placed in `src/api.test.ts` would buy nothing — both files - * are unowned, so one pull request could weaken the gate AND edit its guard - * under a single ordinary review. + * WHAT BELONGS HERE, AND WHAT DOES NOT. This is the structural half of a pair. * - * WHAT THIS DOES *NOT* PROTECT. A future reader who over-trusts this file is a - * worse outcome than not having it, so, plainly: + * - `src/api.test.ts` owns BEHAVIOUR, and owns it better than source-reading + * ever could: it drives the real functions through the injected `HttpFetch` + * and inspects the `Headers` that come out. "The Bearer header carries the + * token it was passed", "a declining user sends no id-token", "a refresh + * cannot turn the header on for someone who opted out" are all tested + * there, against running code, and they survive any refactor. + * - This file owns ABSENCE — the claims a behavioural test structurally + * cannot make, because you cannot call a function and observe the headers + * it DIDN'T send, or the request path that DOESN'T exist: * - * - **It asserts what it asserts.** These are four specific structural rules. - * A sufficiently novel credential path can be written to satisfy every one - * of them — a request built outside `fetchOrUnreachable`, a token folded - * into a URL or a request body, a credential handed to a helper in another - * module. None of that is caught here. - * - **A reviewer still has to think.** This narrows what can be done QUIETLY. - * It is friction and detection, not prevention. - * - **It does not protect `src/api.ts` from a release signer**, and is not - * meant to. A signer can change both files in one pull request. The threat - * model is an outsider's pull request seen by one ordinary reviewer. - * - **It reads structure, not behaviour.** It cannot tell you the token a - * parameter carries is the right one, only that a parameter is where it came - * from. Behavioural coverage lives in `src/api.test.ts`; this is the - * structural half, and the two are not substitutes. - * - **The header collectors recognise the forms this file uses.** They are - * written to fail closed — an unfamiliar header-bag shape is an error, not a - * skip (invariant 4) — but "fails closed on what it can see" is still not - * "sees everything". + * "no header leaves this file that is not on a list" + * "there is exactly one place that issues a request" + * "a credential never reaches a URL, a body, or any other call" * - * WHY A TYPE CHECKER AND NOT A REGEX. Invariants 2 and 3 are claims about SCOPE - * — "this call is inside that `if`", "this function never assigns that - * property". Line proximity is not scope: a `headers.set` moved one line down, - * out of a guard's block, looks identical to grep, and is exactly the regression - * worth catching. Invariant 1 is a claim about BINDING — "this token came from a - * parameter" — which name matching cannot answer, because a local - * `const accessToken = process.env.TOKEN` shadows a parameter of the same name - * and reads identically. So this resolves symbols rather than comparing text. + * An earlier version of this file also re-asserted the behavioural properties + * structurally. That was duplication in a worse form — it failed four of five + * behaviour-preserving edits while `src/api.test.ts` passed all five — so those + * assertions are gone. Before adding a rule here, check whether it can be + * written as a behavioural test in `src/api.test.ts` instead. If it can, it + * belongs there. * - * `typescript` is already a devDependency (it backs `bun run typecheck`), and - * `noResolve`/`noLib` keep the program to this one file, so nothing new is - * installed and no dependency graph is walked. `bun run test` runs only in - * unprivileged jobs — `ci-cli`, and `release-cli.yml`'s `prepare`, which holds - * neither `id-token: write` nor the store environment. `workflow-shape.test.ts` - * is what keeps that true. + * WHAT THIS DOES NOT PROTECT. A reader who over-trusts it is a worse outcome + * than not having it: * - * INVARIANT 4 IS THE ONE THAT MATTERS MOST. Invariants 1-3 pin gates we already - * know about. Invariant 4 — the allowlist of header names — catches the - * credential path nobody has thought of yet, because it fails BY DEFAULT on - * anything new: adding a header to `src/api.ts` goes red until somebody edits - * `scripts/`, which is to say until a release signer looks at it. If this file - * ever has to be cut down, cut everything before invariant 4. + * - It asserts what it asserts. A sufficiently novel credential path can be + * written to satisfy every rule below — most obviously one that lives in + * another module entirely. + * - A reviewer still has to think. This narrows what can be done QUIETLY; it + * is friction and detection, not prevention. + * - It does not protect `src/api.ts` from a release signer, and is not meant + * to. The threat model is an outsider's pull request seen by one ordinary + * reviewer. + * - The collectors fail closed on shapes they cannot read, but "fails closed + * on what it can see" is still not "sees everything". + * + * The stronger fix is a separate, owned `src/api-credentials.ts`: an owned + * module makes this class of change impossible rather than merely detectable, + * and would reduce this file to its header allowlist. That is tracked + * separately; this is what is cheap today. + * + * WHY A TYPE CHECKER. Two rules are about BINDING, not text — "this call + * reaches the network through the injected parameter", "this value came from + * the credential". Name matching cannot answer either, so symbols are resolved. + * `typescript` already backs `bun run typecheck`, and `noResolve`/`noLib` keep + * the program to this one file. `bun run test` runs only in unprivileged jobs — + * `ci-cli`, and `release-cli.yml`'s `prepare`, which holds neither + * `id-token: write` nor the store environment. */ import { describe, expect, it } from "bun:test"; import ts from "typescript"; const SOURCE_PATH = "src/api.ts"; -/** - * One file, no lib, no module resolution. The checker only ever has to answer - * questions about bindings declared inside `src/api.ts` itself, so following - * imports would cost seconds and buy nothing. - */ const program = ts.createProgram([SOURCE_PATH], { noResolve: true, noLib: true, @@ -87,19 +79,19 @@ const program = ts.createProgram([SOURCE_PATH], { const checker = program.getTypeChecker(); const sourceFile = program.getSourceFile(SOURCE_PATH) as ts.SourceFile; -/** 1-based line of a node, so a failure names a place rather than a shape. */ -const lineOf = (node: ts.Node): number => - sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile)).line + 1; - -const where = (node: ts.Node): string => `${SOURCE_PATH}:${lineOf(node)}`; +const where = (node: ts.Node): string => { + const line = sourceFile.getLineAndCharacterOfPosition( + node.getStart(sourceFile), + ).line; + return `${SOURCE_PATH}:${line + 1}`; +}; const normalize = (text: string): string => text.replace(/\s+/g, " ").trim(); -/** A node, with its location, as one string — the shape a failure message wants. */ +/** A node with its location — the shape a failure message wants. */ const cite = (node: ts.Node): string => `${where(node)}: ${normalize(node.getText(sourceFile))}`; -/** Every node in the file, depth-first. */ function* walk(node: ts.Node): Generator { yield node; for (const child of node.getChildren(sourceFile)) yield* walk(child); @@ -107,9 +99,9 @@ function* walk(node: ts.Node): Generator { const allNodes = [...walk(sourceFile)]; -/** The static text of a property name or a string-ish literal, or `undefined` - * when it is computed. A computed name defeats every allowlist below, so - * `undefined` is always treated as a failure rather than quietly skipped. */ +/** The static text of a name or string-ish literal, or `undefined` when it is + * computed. Every allowlist below turns `undefined` into an unlistable name + * rather than skipping it, so a computed key fails instead of slipping past. */ function staticName(node: ts.Node | undefined): string | undefined { if (node === undefined) return undefined; if (ts.isIdentifier(node)) return node.text; @@ -117,34 +109,14 @@ function staticName(node: ts.Node | undefined): string | undefined { return undefined; } -/** HTTP header names are case-insensitive on the wire, so every comparison here - * is too: `authorization` must not be a way around a rule about - * `Authorization`. */ +/** Header names are case-insensitive on the wire, so comparisons here are too. */ const headerKey = (name: string): string => name.toLowerCase(); -/** The nearest function-like ancestor — the scope whose parameters are in play. */ -function enclosingFunction(node: ts.Node): ts.SignatureDeclaration | undefined { - for (let scope = node.parent; scope !== undefined; scope = scope.parent) { - if ( - ts.isFunctionDeclaration(scope) || - ts.isFunctionExpression(scope) || - ts.isArrowFunction(scope) || - ts.isMethodDeclaration(scope) - ) { - return scope; - } - } - return undefined; -} - -/** Where an identifier is DECLARED, resolved through the checker rather than - * guessed from its text — a local can shadow a parameter of the same name. */ +/** Where an identifier is DECLARED, via the checker rather than by its text. + * `{ ...rest, token }` needs the shorthand detour: the identifier there is the + * PROPERTY's name, so asking about it directly returns the property symbol and + * the value being read is silently missed. */ function declarationOf(node: ts.Node): ts.Declaration | undefined { - // `{ ...rest, accessToken }` is the case worth spelling out. The identifier - // there is the PROPERTY's name, and asking the checker about it returns the - // property symbol — not the variable being read. Resolving through the - // shorthand node is what gets the value, and without this a token packed - // into a request body resolves to nothing and is silently skipped. const target = ts.isIdentifier(node) && ts.isShorthandPropertyAssignment(node.parent) ? node.parent @@ -155,8 +127,7 @@ function declarationOf(node: ts.Node): ts.Declaration | undefined { return symbol?.declarations?.[0]; } -/** True when `node` resolves to a parameter of `fn` itself — not of an outer - * function it happens to close over, and not a local that shadows one. */ +/** True when `node` resolves to a parameter of `fn` itself. */ function isParameterOf( node: ts.Node, fn: ts.SignatureDeclaration | undefined, @@ -170,13 +141,24 @@ function isParameterOf( ); } +/** The name of the nearest enclosing function, for grouping call sites. */ +function enclosingFunctionName(node: ts.Node): string { + for (let scope = node.parent; scope !== undefined; scope = scope.parent) { + if (ts.isFunctionDeclaration(scope)) { + return scope.name?.text ?? ""; + } + if (ts.isFunctionExpression(scope) || ts.isArrowFunction(scope)) { + return ""; + } + } + return ""; +} + // --------------------------------------------------------------------------- -// Header bags: what counts as one, and the rule that an unfamiliar one fails +// Header bags — what counts as one, and the rule that an unfamiliar one fails // --------------------------------------------------------------------------- -/** Every identifier bound to a `new Headers(...)`. `src/api.ts` builds exactly - * one, in `fetchOrUnreachable`; invariant 4 asserts that stays true, so a - * second bag cannot appear under a name these collectors do not know. */ +/** Every identifier bound to a `new Headers(...)`. */ const headerBagNames = new Set( allNodes .filter( @@ -191,249 +173,55 @@ const headerBagNames = new Set( ); /** The trailing name of a `.set(...)` receiver: `headers` for `headers.set`, - * `headers` for `init.headers.set`, `requestHeaders` for `requestHeaders.set`. */ + * `headers` for `init.headers.set`, `requestHeaders` for + * `requestHeaders.set`. */ function receiverName(expr: ts.Expression): string | undefined { if (ts.isIdentifier(expr)) return expr.text; if (ts.isPropertyAccessExpression(expr)) return expr.name.text; return undefined; } -/** Every `.set(...)` call. Two ways in, because either - * alone leaves a hole: a bag called `requestHeaders` slips a `new Headers` - * rule if it is assigned rather than declared, and a bag called `h` slips a - * name rule. `res.headers.get(...)` is a READ of a response and is - * deliberately not here. */ +/** Every write to a header bag. Matched two ways because either alone leaves a + * hole: a bag called `h` slips a name rule, and one that is assigned rather + * than declared slips a `new Headers` rule. `res.headers.get(...)` is a READ + * of a response and is deliberately not here. */ const headerSetCalls = allNodes.filter((node): node is ts.CallExpression => { if (!ts.isCallExpression(node)) return false; if (!ts.isPropertyAccessExpression(node.expression)) return false; if (node.expression.name.text !== "set") return false; const name = receiverName(node.expression.expression); - if (name === undefined) return false; - return headerBagNames.has(name) || /headers?$/i.test(name); + return ( + name !== undefined && (headerBagNames.has(name) || /headers?$/i.test(name)) + ); }); -/** Every `headers:` property assignment, whatever its value is. Invariant 4 - * rejects the ones whose value is not a plain object literal rather than - * skipping them. */ +/** Every `headers:` property, whatever its value. The rules below reject the + * ones whose value is not a plain object literal rather than skipping them. */ const headerProperties = allNodes.filter( (node): node is ts.PropertyAssignment => ts.isPropertyAssignment(node) && staticName(node.name) === "headers", ); -/** Every member of an inline header object — INCLUDING spreads, which invariant - * 4 fails on, because dropping them here is how a collector goes quiet. */ +/** Every member of an inline header object, INCLUDING spreads — dropping them + * here is how a collector goes quiet. */ const inlineHeaderMembers = headerProperties .filter((node) => ts.isObjectLiteralExpression(node.initializer)) .flatMap( (node) => (node.initializer as ts.ObjectLiteralExpression).properties, ); -const inlineHeaderProps = inlineHeaderMembers.filter(ts.isPropertyAssignment); - -// --------------------------------------------------------------------------- -// Invariant 1 - every Authorization value is `Bearer ${}` -// --------------------------------------------------------------------------- - -/** Both ways a header can be set in this file, reduced to (name, value), then - * narrowed to the Authorization ones. */ -const authorizationSites: { name: ts.Node; value: ts.Expression }[] = [ - ...inlineHeaderProps.map((prop) => ({ - name: prop.name as ts.Node, - value: prop.initializer, - })), - ...headerSetCalls.map((call) => ({ - name: call.arguments[0] as ts.Node, - value: call.arguments[1] as ts.Expression, - })), -].filter((site) => headerKey(staticName(site.name) ?? "") === "authorization"); - -describe("invariant 1: Authorization carries a Bearer token from a parameter", () => { - it("sets Authorization somewhere, so the rules below are exercised", () => { - // Without this, removing every call site would make the cases below - // vacuously green: `it.each([])` asserts nothing at all. - expect(authorizationSites.length).toBeGreaterThan(0); - }); - - it.each( - authorizationSites.map((site) => [where(site.value), site] as const), - )("%s is a template of Bearer plus exactly one interpolation", (_label, site) => { - // A string literal here would be a hardcoded credential. A concatenation, - // a second span or a tail would let a prefix or suffix smuggle something - // else into the same header. - expect(ts.isTemplateExpression(site.value)).toBe(true); - const template = site.value as ts.TemplateExpression; - expect(template.head.text).toBe("Bearer "); - expect(template.templateSpans).toHaveLength(1); - expect(template.templateSpans[0]?.literal.text).toBe(""); - }); - - it.each( - authorizationSites.map((site) => [where(site.value), site] as const), - )("%s interpolates a parameter of the function that sends the request", (_label, site) => { - const template = site.value as ts.TemplateExpression; - const interpolated = template.templateSpans[0]?.expression; - - // A bare identifier: not `process.env.X`, not `config.token`, not a call. - expect(interpolated !== undefined && ts.isIdentifier(interpolated)).toBe( - true, - ); - - // ...and one the CHECKER says is a parameter of the NEAREST enclosing - // function. Both halves earn their place: - // - resolved, not name-matched, because `const accessToken = - // process.env.TOKEN` shadows the parameter and reads identically; - // - nearest, not any ancestor, because a nested helper closing over an - // outer function's parameter is a different claim from this one. - // The point is that the credential is PASSED IN, so the decision about - // which token to send stays with the command that made it. - const fn = enclosingFunction(site.value); - expect({ - at: cite(site.value), - fromAParameter: isParameterOf(interpolated as ts.Identifier, fn), - }).toEqual({ at: cite(site.value), fromAParameter: true }); - }); -}); - -// --------------------------------------------------------------------------- -// Invariant 2 - x-wego-id-token only inside the consent guard -// --------------------------------------------------------------------------- - -/** The guard the identity assertion must sit behind, normalized for whitespace - * so reformatting is free and reordering is not. `allowed` is the user's - * consent; `token` is the assertion itself. Both, or nothing is sent. */ -const CONSENT_GUARD = "identityAssertion.allowed && identityAssertion.token"; - -const idTokenSets = headerSetCalls.filter( - (call) => - headerKey(staticName(call.arguments[0]) ?? "") === "x-wego-id-token", -); - -describe("invariant 2: x-wego-id-token is sent only under the consent guard", () => { - it("is set exactly once", () => { - // One call site is what lets "inside the guard" be a complete statement - // about the file. A second would mean the rule below has to hold in two - // places and a reviewer has to notice both — the situation this prevents. - expect(idTokenSets.map(where)).toHaveLength(1); - }); - - it.each( - idTokenSets.map((call) => [where(call), call] as const), - )(`%s sits in the THEN branch of \`if (${CONSENT_GUARD})\``, (_label, call) => { - // Scope, not proximity. An `if` whose ELSE branch holds the call, or an - // `if` the call merely follows, must not count — so this walks the parent - // chain and requires the child to be on the `thenStatement` side. - const guards: string[] = []; - let node: ts.Node = call; - while (node.parent !== undefined) { - const parent: ts.Node = node.parent; - if (ts.isIfStatement(parent) && parent.thenStatement === node) { - guards.push(normalize(parent.expression.getText(sourceFile))); - } - node = parent; - } - expect(guards).toContain(CONSENT_GUARD); - }); -}); - -// --------------------------------------------------------------------------- -// Invariant 3 - a refresh never re-decides consent -// --------------------------------------------------------------------------- - -const refresh = allNodes.find( - (node): node is ts.FunctionDeclaration => - ts.isFunctionDeclaration(node) && - node.name?.text === "refreshIdentityAssertion", -); - -const refreshNodes = refresh?.body === undefined ? [] : [...walk(refresh.body)]; - -describe("invariant 3: refreshIdentityAssertion never assigns `allowed`", () => { - it("exists as a function declaration with a body", () => { - // If it is renamed or reshaped, the rules below stop applying SILENTLY. - // Failing here sends whoever did that to this file to say why. - expect(refresh?.body).toBeDefined(); - }); - - it("assigns no property other than `token`", () => { - // The consent decision is made once, by `setIdentityAssertion`, out of the - // login flow. A refresh only ever learns a NEW TOKEN. A refresh that could - // also flip `allowed` to `true` would send an identity assertion for a user - // who declined — a consent bypass that reads like a one-word tidy-up. - // - // Stated as an allowlist rather than as "no property named `allowed`": a - // computed key (`{ ["allow" + "ed"]: true }`) has no legitimate use in this - // three-line function, and an allowlist rejects it without having to guess - // at what it evaluates to. - const offenders = refreshNodes - .filter((node) => { - if ( - ts.isPropertyAssignment(node) || - ts.isShorthandPropertyAssignment(node) - ) { - return staticName(node.name) !== "token"; - } - if ( - ts.isBinaryExpression(node) && - node.operatorToken.kind === ts.SyntaxKind.EqualsToken && - ts.isPropertyAccessExpression(node.left) - ) { - return node.left.name.text === "allowed"; - } - return false; - }) - .map(cite); - - expect(offenders).toEqual([]); - }); - - it("assigns the `token` parameter it was handed", () => { - // The allowlist above is satisfied by a function that assigns NOTHING, and - // a refresh that quietly stops refreshing is its own bug — the CLI would go - // on presenting a stale assertion. So the token write is required, and - // required to be THE PARAMETER: `{ ...identityAssertion, token: somethingElse }` - // passes a name check and fails this one. - const assignsParameter = refreshNodes - .filter( - ( - node, - ): node is ts.PropertyAssignment | ts.ShorthandPropertyAssignment => - (ts.isPropertyAssignment(node) || - ts.isShorthandPropertyAssignment(node)) && - staticName(node.name) === "token", - ) - .some((node) => - ts.isShorthandPropertyAssignment(node) - ? isParameterOf(node, refresh) - : isParameterOf(node.initializer, refresh), - ); - expect(assignsParameter).toBe(true); - }); - - it("carries the previous state forward by spreading it", () => { - // The positive half of the rule above. Not spreading would be fail-CLOSED - // today (`allowed` would be absent, and absent is falsy), so this is not a - // security assertion standing on its own — it pins the MECHANISM, so the - // next person here reads "carry forward" rather than "re-derive". - const spreads = refreshNodes - .filter(ts.isSpreadAssignment) - .map((node) => normalize(node.expression.getText(sourceFile))); - expect(spreads).toContain("identityAssertion"); - }); -}); - // --------------------------------------------------------------------------- -// Invariant 4 - the closed set of header names +// The closed set of headers // --------------------------------------------------------------------------- /** - * EVERY header name `src/api.ts` may attach through a header bag's `.set`, and - * what each one carries. This list is the point of the whole file: it fails by - * DEFAULT on anything new, so a header nobody anticipated cannot be added - * quietly. + * EVERY header name `src/api.ts` may attach through a header bag, and what each + * carries. This list is why the file exists: it fails by DEFAULT on anything + * new, so a header nobody anticipated cannot be added quietly. * - * To add a header: add it here, in the same pull request, with a line saying - * what it carries. That edit is in `scripts/`, so it needs a release signer — - * which is the review a new outbound header deserves. + * To add one: add it here, in the same pull request, with a line saying what it + * carries. That edit is in `scripts/`, so it needs a release signer — which is + * the review a new outbound header deserves. */ const ALLOWED_SET_HEADERS: Record = { "user-agent": "build identification; carries no user data", @@ -443,38 +231,35 @@ const ALLOWED_SET_HEADERS: Record = { "x-wego-os-type": "machine fact, not stored telemetry", "x-wego-os-version": "machine fact, not stored telemetry", "x-wego-timezone": "utc offset, computed per request", - "x-wego-id-token": "THE identity assertion - gated by invariant 2", + "x-wego-id-token": "THE identity assertion - gated below", }; -/** The same closed set, for headers written inline into a `fetch` init object. - * Separate list because it is a separate mechanism: a new credential added - * there would never touch a `.set` call. */ +/** The same closed set for headers written inline into a `fetch` init object. + * A separate mechanism, so a separate list: a credential added there would + * never touch a `.set` call. */ const ALLOWED_LITERAL_HEADERS: Record = { - authorization: "THE access token - shaped by invariant 1", + authorization: "THE access token", "content-type": "request body encoding, JSON on the POST path", }; -describe("invariant 4: no header leaves this file without being listed here", () => { - it("builds exactly one header bag, under a name the collectors know", () => { - // The collectors recognise `.set` on a `new Headers` binding or on a - // headerish name. The NUMBER of bags is worth pinning on its own: one bag - // is why "every header this file sends" is a list somebody can finish - // reading, and a second one is a second place to look. - expect([...headerBagNames]).toEqual(["headers"]); - }); - - it("names every header-bag .set(...) with a static string", () => { - // `headers.set(name, value)` with a variable name would make the allowlist - // below unenforceable, so the allowlist starts by requiring names it can read. - const dynamic = headerSetCalls - .filter((call) => staticName(call.arguments[0]) === undefined) - .map(cite); - expect(dynamic).toEqual([]); +/** A header name as the allowlist sees it. A computed key becomes an unlistable + * string rather than a skipped entry, so it fails like any unknown name. */ +const listedAs = (name: string | undefined, node: ts.Node): string => + name === undefined ? `` : headerKey(name); + +describe("no header leaves this file without being listed here", () => { + it("builds exactly one header bag", () => { + // The NUMBER of bags is the property; the NAME is not. One bag is why + // "every header this file sends" is a list somebody can finish reading. + expect({ bags: [...headerBagNames], count: headerBagNames.size }).toEqual({ + bags: [...headerBagNames], + count: 1, + }); }); - it("sets only allowlisted header names", () => { + it("writes only allowlisted names to the header bag", () => { const names = headerSetCalls.map((call) => - headerKey(staticName(call.arguments[0]) as string), + listedAs(staticName(call.arguments[0]), call), ); expect([...new Set(names)].sort()).toEqual( Object.keys(ALLOWED_SET_HEADERS).sort(), @@ -482,9 +267,9 @@ describe("invariant 4: no header leaves this file without being listed here", () }); it("builds every inline header bag as a plain object literal", () => { - // `headers: credentialHeaders` or `headers: buildHeaders(token)` moves the - // decision somewhere this file cannot see. Fail rather than skip: a bag the - // collector cannot read is the case an allowlist is worth least in. + // `headers: buildHeaders(token)` moves the decision somewhere this file + // cannot see. Fail rather than skip: a bag the collector cannot read is the + // case an allowlist is worth least in. const opaque = headerProperties .filter((node) => !ts.isObjectLiteralExpression(node.initializer)) .map(cite); @@ -493,8 +278,7 @@ describe("invariant 4: no header leaves this file without being listed here", () it("writes every inline header as a named property, never a spread", () => { // `{ ...credentialHeaders, "Content-Type": "application/json" }` leaves the - // name list below unchanged while adding any header it likes. A spread is - // therefore a failure in its own right, not a member the collector drops. + // name list unchanged while adding any header it likes. const unreadable = inlineHeaderMembers .filter((member) => !ts.isPropertyAssignment(member)) .map(cite); @@ -502,11 +286,9 @@ describe("invariant 4: no header leaves this file without being listed here", () }); it("writes only allowlisted names into inline header objects", () => { - const names = inlineHeaderProps - .map((prop) => staticName(prop.name)) - // A computed key inside a header bag is the same hole as a dynamic - // `.set` name. Surface it as an unlistable name rather than skip it. - .map((name) => (name === undefined ? "" : headerKey(name))); + const names = inlineHeaderMembers + .filter(ts.isPropertyAssignment) + .map((prop) => listedAs(staticName(prop.name), prop)); expect([...new Set(names)].sort()).toEqual( Object.keys(ALLOWED_LITERAL_HEADERS).sort(), ); @@ -514,49 +296,22 @@ describe("invariant 4: no header leaves this file without being listed here", () }); // --------------------------------------------------------------------------- -// Invariant 5 - the request chokepoint, and where a token may travel +// One door, and a closed list of who may use it // --------------------------------------------------------------------------- /** - * WHY THIS ONE EXISTS, AND WHY IT IS NOT LAST IN IMPORTANCE. - * - * Invariants 1-4 all describe the header bag of a request that already goes - * through `fetchOrUnreachable`. That leaves the largest hole in the file wide - * open: a request that never goes through it at all. + * `src/api.test.ts` can prove the headers a request DOES carry. It cannot prove + * that no other request exists — and this line satisfies every header rule + * above while shipping the user's token to a host nobody chose: * * await fetch("https://collector.example/ingest", { * headers: { Authorization: `Bearer ${accessToken}` }, * }); * - * That line satisfies every rule above — the header name is allowlisted, the - * value is a `Bearer` template, and the token is a parameter of the enclosing - * function. It also sends the user's access token to a host nobody chose. The - * allowlists guard the payload at the door; this one guards the door. - * - * So the claim here is about REACHABILITY, in two halves: - * - * a. Exactly one thing in this file issues a network request, every caller of - * it is named, and the set of callers is closed. A new request path is a - * `scripts/` edit — that is, a release signer — before it can exist. - * b. An access token may only ever be FORWARDED to another `accessToken` - * parameter, or interpolated into the `Bearer` header. It may not enter a - * URL, a query string, a request body, a log line, or any other call. - * Header rules cannot see a token in a query string; this can. - * - * `src/target.ts` and `src/config.ts` — both already code-owned — decide which - * HOST a token may reach and refuse cleartext. This is what stops `src/api.ts` - * from going around them. - * - * LIMIT, stated because the rest of this file states its limits. Rule (b) - * tracks the `accessToken` parameters by name and by symbol. It does not follow - * a token through a local alias (`const t = accessToken`) — that is caught only - * because the alias would then have to reach the wire through a call this rule - * already refuses. It does not track `identityAssertion.token`, which is module - * state rather than a parameter; invariant 2 pins its one use. + * `src/target.ts` and `src/config.ts`, both already code-owned, decide which + * host may receive a credential and refuse cleartext. This is what stops + * `src/api.ts` going around them. */ - -/** The only function permitted to issue a request, and the closed set of - * callers allowed to reach it. Adding a caller means adding it here. */ const REQUEST_CHOKEPOINT = "fetchOrUnreachable"; const ALLOWED_CHOKEPOINT_CALLERS: Record = { authedJsonGet: "the shared authenticated GET envelope", @@ -568,49 +323,34 @@ const chokepoint = allNodes.find( ts.isFunctionDeclaration(node) && node.name?.text === REQUEST_CHOKEPOINT, ); -/** The name of the nearest enclosing function, for grouping call sites. */ -function enclosingFunctionName(node: ts.Node): string { - const fn = enclosingFunction(node); - if (fn === undefined) return ""; - const name = (fn as ts.FunctionDeclaration).name; - return name === undefined ? "" : name.text; -} - -describe("invariant 5a: exactly one door, and a closed list of who may use it", () => { - it(`declares ${REQUEST_CHOKEPOINT} as a function`, () => { - expect(chokepoint).toBeDefined(); - }); - +describe("exactly one place issues a request", () => { it("references the global fetch only as the injected default", () => { - // `http: HttpFetch = fetch` is the ONE permitted mention: it is what makes - // the transport swappable in tests. Any other `fetch` in this file is a - // second door — a request that skips every header rule above, and skips - // `src/target.ts`'s decision about which host may receive a credential. - const strayFetch = allNodes + // `http: HttpFetch = fetch` is the ONE permitted mention — it is what makes + // the transport swappable, and what `src/api.test.ts` drives. Any other + // `fetch` here is a second door. + const stray = allNodes .filter( (node): node is ts.Identifier => ts.isIdentifier(node) && node.text === "fetch", ) .filter((node) => !ts.isParameter(node.parent)) .map(cite); - expect(strayFetch).toEqual([]); + expect(stray).toEqual([]); }); - it("issues exactly one request, through its injected transport", () => { - // Inside the chokepoint, the call that reaches the network must be the - // INJECTED parameter, not a module-scope binding and not a fresh import. - const viaInjectedTransport = ( - chokepoint?.body === undefined ? [] : [...walk(chokepoint.body)] - ) + it("reaches the network only through the injected transport", () => { + // Also pins that the chokepoint still exists: renamed away, the caller rule + // below would otherwise stop applying silently. + expect(chokepoint?.body).toBeDefined(); + const viaInjected = [...walk(chokepoint?.body as ts.Node)] .filter(ts.isCallExpression) .filter((call) => isParameterOf(call.expression, chokepoint)); - expect(viaInjectedTransport.map(where)).toHaveLength(1); + expect(viaInjected.map(where)).toHaveLength(1); }); it("is reached only from the allowlisted callers", () => { // The fails-by-default half. A new request path — a new endpoint, a retry - // helper, a "quick" health check — has to add its name here, in `scripts/`, - // which is to say in front of a release signer. + // helper, a "quick" health check — has to add its name here first. const callers = allNodes .filter( (node): node is ts.CallExpression => @@ -624,29 +364,26 @@ describe("invariant 5a: exactly one door, and a closed list of who may use it", }); }); -/** Every parameter in the file named `accessToken` — the credential's only - * legitimate carrier. */ -const accessTokenParams = allNodes.filter( - (node): node is ts.ParameterDeclaration => - ts.isParameter(node) && staticName(node.name) === "accessToken", -); +// --------------------------------------------------------------------------- +// Where a credential may travel +// --------------------------------------------------------------------------- -/** True when `node` is an argument of a call whose callee declares that same - * position as an `accessToken` parameter — i.e. the token is being FORWARDED, - * not consumed. `logDebug(accessToken)` fails: `logDebug`'s parameter is not - * called `accessToken`, so the token would be leaving its lane. */ -function isForwardedToAnAccessTokenParameter(node: ts.Node): boolean { +/** The parameter position an argument is passed into, or `undefined` when the + * node is not a call argument. */ +function parameterReceiving( + node: ts.Node, +): ts.ParameterDeclaration | undefined { const call = node.parent; - if (!ts.isCallExpression(call)) return false; + if (call === undefined || !ts.isCallExpression(call)) return undefined; const index = call.arguments.indexOf(node as ts.Expression); - if (index < 0) return false; + if (index < 0) return undefined; const callee = declarationOf(call.expression); - if (callee === undefined || !ts.isFunctionLike(callee)) return false; - return staticName(callee.parameters[index]?.name) === "accessToken"; + if (callee === undefined || !ts.isFunctionLike(callee)) return undefined; + return callee.parameters[index]; } -/** True when `node` is the single interpolation of a `Bearer ` template — - * the one place the token is allowed to become text. */ +/** True when `node` is the single interpolation of a `Bearer ` template — the + * one place a credential may become text. */ function isTheBearerInterpolation(node: ts.Node): boolean { const span = node.parent; if (span === undefined || !ts.isTemplateSpan(span)) return false; @@ -658,44 +395,193 @@ function isTheBearerInterpolation(node: ts.Node): boolean { ); } -describe("invariant 5b: an access token is only forwarded, or put in the header", () => { +/** + * THE CREDENTIAL PARAMETERS, derived rather than named — a guard that depends + * on an identifier is a guard an ordinary rename switches off. + * + * SEED every parameter interpolated into a `Bearer ` template. + * CLOSURE any parameter passed into a credential parameter's position is + * itself carrying the credential. Iterated to a fixed point, since + * the CLI threads the token down several layers. + */ +const credentialParams = new Set(); + +for (const node of allNodes) { + if (!ts.isTemplateExpression(node)) continue; + if (node.head.text !== "Bearer " || node.templateSpans.length !== 1) continue; + const seed = declarationOf(node.templateSpans[0]?.expression as ts.Node); + if (seed !== undefined && ts.isParameter(seed)) credentialParams.add(seed); +} + +for (let growing = true; growing; ) { + growing = false; + for (const node of allNodes) { + if (!ts.isIdentifier(node)) continue; + const target = parameterReceiving(node); + if (target === undefined || !credentialParams.has(target)) continue; + const source = declarationOf(node); + if ( + source !== undefined && + ts.isParameter(source) && + !credentialParams.has(source) + ) { + credentialParams.add(source); + growing = true; + } + } +} + +describe("a credential is only forwarded, or put in the Bearer header", () => { const references = allNodes - .filter( - (node): node is ts.Identifier => - ts.isIdentifier(node) && node.text === "accessToken", - ) - // The declarations themselves are not uses. + .filter(ts.isIdentifier) .filter((node) => !ts.isParameter(node.parent)) - // Only the ones that actually resolve to an `accessToken` PARAMETER; a - // same-named local is invariant 1's problem, not this one's. .filter((node) => { const declaration = declarationOf(node); return ( declaration !== undefined && - accessTokenParams.includes(declaration as ts.ParameterDeclaration) + ts.isParameter(declaration) && + credentialParams.has(declaration) ); }); - it("has references to check, so the rule below is not vacuous", () => { + it("found the credential parameters to check", () => { + // Without this the rule below is vacuous: if the seed stops matching, the + // set is empty, every filter yields nothing, and this goes green having + // checked nothing at all. + expect(credentialParams.size).toBeGreaterThan(0); expect(references.length).toBeGreaterThan(0); }); - it("never lets a token reach a URL, a body, or any other call", () => { - // This is the rule the header allowlists cannot express. A token - // concatenated into a URL, packed into a JSON body, handed to a logger, or - // stringified into a cache key never touches a `headers.set` and never - // touches an `Authorization` property — so invariants 1-4 would all pass. - // - // Two permitted shapes, and nothing else: - // - forwarded into another function's `accessToken` parameter; - // - interpolated as the sole span of the `Bearer ` template. + it("never lets a credential reach a URL, a body, or any other call", () => { + // The rule the header allowlists cannot express. A token concatenated into + // a URL, packed into a JSON body, or handed to a logger never touches a + // header at all — so every rule above stays silent on it. const escaped = references + .filter((node) => { + const target = parameterReceiving(node); + const forwarded = target !== undefined && credentialParams.has(target); + return !forwarded && !isTheBearerInterpolation(node); + }) + .map(cite); + expect(escaped).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// Consent is never re-decided +// --------------------------------------------------------------------------- + +/** + * The one behavioural property that is ALSO kept here, deliberately. + * + * `src/api.test.ts` tests it properly — "a refresh cannot turn the header on + * for someone who opted out" — but that file is unowned, so one pull request + * could flip the behaviour and delete the test that noticed. Every other rule + * in this file would stay green: the header name is allowlisted, no new request + * appears, no credential moves. Silently sending an identity assertion for a + * user who declined is the quietest bad change available in `src/api.ts`, which + * is why it gets a second, owned guard. + */ +const CONSENT_CONJUNCTS = [ + "identityAssertion.allowed", + "identityAssertion.token", +]; + +/** Flatten an `&&` chain into its operands, so `a && b` and `b && a` are the + * same guard — a rule that accepts one spelling only teaches people to edit + * the rule rather than read it. */ +function conjunctsOf(expr: ts.Expression): string[] { + if ( + ts.isBinaryExpression(expr) && + expr.operatorToken.kind === ts.SyntaxKind.AmpersandAmpersandToken + ) { + return [...conjunctsOf(expr.left), ...conjunctsOf(expr.right)]; + } + return [normalize(expr.getText(sourceFile))]; +} + +const refresh = allNodes.find( + (node): node is ts.FunctionDeclaration => + ts.isFunctionDeclaration(node) && + node.name?.text === "refreshIdentityAssertion", +); + +const refreshNodes = refresh?.body === undefined ? [] : [...walk(refresh.body)]; + +describe("consent is never re-decided", () => { + it("sends the identity assertion only under both conjuncts", () => { + const idTokenSets = headerSetCalls.filter( + (call) => + headerKey(staticName(call.arguments[0]) ?? "") === "x-wego-id-token", + ); + expect(idTokenSets.map(where)).toHaveLength(1); + + // Scope, not proximity: an `if` whose ELSE branch holds the call, or one + // the call merely follows, must not count. + const guards: string[][] = []; + let node: ts.Node = idTokenSets[0] as ts.Node; + while (node.parent !== undefined) { + const parent: ts.Node = node.parent; + if (ts.isIfStatement(parent) && parent.thenStatement === node) { + guards.push(conjunctsOf(parent.expression).sort()); + } + node = parent; + } + expect(guards).toContainEqual([...CONSENT_CONJUNCTS].sort()); + }); + + it("never gives `allowed` a value other than the one already stored", () => { + // Reading the stored consent back is fine; deciding a new one is not. The + // rule is about the VALUE, not the mention — `{ ...identityAssertion, + // token }` and `{ token, allowed: identityAssertion.allowed }` are the same + // program, and both must pass. + expect(refresh?.body).toBeDefined(); + const CARRIED_FORWARD = "identityAssertion.allowed"; + + const offenders = refreshNodes + .filter((node) => { + if ( + ts.isPropertyAssignment(node) && + staticName(node.name) === "allowed" + ) { + return ( + normalize(node.initializer.getText(sourceFile)) !== CARRIED_FORWARD + ); + } + // `{ allowed }` shorthand takes its value from a local, never from the + // stored state, so it is always a new decision. + if ( + ts.isShorthandPropertyAssignment(node) && + staticName(node.name) === "allowed" + ) { + return true; + } + if ( + ts.isBinaryExpression(node) && + node.operatorToken.kind === ts.SyntaxKind.EqualsToken && + ts.isPropertyAccessExpression(node.left) && + node.left.name.text === "allowed" + ) { + return normalize(node.right.getText(sourceFile)) !== CARRIED_FORWARD; + } + return false; + }) + .map(cite); + + expect(offenders).toEqual([]); + }); + + it("uses no computed property name in the refresh", () => { + // `{ ["allow" + "ed"]: true }` would sail past the rule above, and has no + // legitimate use in a three-line function. + const computed = refreshNodes .filter( (node) => - !isForwardedToAnAccessTokenParameter(node) && - !isTheBearerInterpolation(node), + (ts.isPropertyAssignment(node) || + ts.isShorthandPropertyAssignment(node)) && + staticName(node.name) === undefined, ) .map(cite); - expect(escaped).toEqual([]); + expect(computed).toEqual([]); }); });