diff --git a/docs/operate/troubleshooting.md b/docs/operate/troubleshooting.md index 03a7f1bb..ee7e4695 100644 --- a/docs/operate/troubleshooting.md +++ b/docs/operate/troubleshooting.md @@ -90,7 +90,7 @@ Every API call the action makes is traced as a debug line: method, path, request What the trace never shows: - **The token.** The authorization header is not part of the trace line, and a token stored as a repository secret is masked by the runner wherever it appears in output. -- **A redacted private target's detail.** For redacted private targets in multi-repo mode, the traced path collapses to `` and the payload is dropped entirely. +- **A redacted private target's detail.** For a redacted private target, whether a single-repository, multi-repo, or snapshot run reaches it, the traced path collapses to `` and the payload is dropped entirely. Failures do not need debug mode; every error already carries the API's message and the fix, so reach for the trace when you need to see the requests that succeeded. diff --git a/docs/reference/library.md b/docs/reference/library.md index 8e5aa226..6b867e65 100644 --- a/docs/reference/library.md +++ b/docs/reference/library.md @@ -135,8 +135,8 @@ console.log(parsed.success, SECTION_KEYS.length, schema.$schema); | `DEFAULT_API_VERSION` | const | The `X-GitHub-Api-Version` the action pins | | `ApiError` | type | A failed request as the port returns it: `status`, `message`, `body` | | `GraphqlOp` | type | A GraphQL operation as the port takes it | -| `RequestMark` | type | The per-request options; `carriesSecret` marks a payload holding a resolved secret | -| `ListOptions` | type | What `tryList` takes beside the path: `perPage` (the page size, `PAGE_SIZE` when omitted) and `until` (ends the walk after the first page it accepts) | +| `RequestMark` | type | The per-request options; `carriesSecret` marks a payload holding a resolved secret, `redactTrace` a request serving a redacted target, whose route and payload stay out of the trace | +| `ListOptions` | type | What `tryList` takes beside the path: `perPage` (the page size, `PAGE_SIZE` when omitted), `until` (ends the walk after the first page it accepts), and `redactTrace` (as on `RequestMark`) | | `PAGE_SIZE` | const | The page size a list walk asks for unless the endpoint caps lower: 100, GitHub's maximum | | `classifyApiError` | function | Which `ApiErrorKind` an `ApiError` is, the one judgment the two predicates below read | | `ApiErrorKind` | type | `"rate-limit"`, `"permission"` (any other 403, and every 404: a fine-grained token answers 404 for a resource it denies, so an absent one lands here too), or `"other"` | diff --git a/src/flows/multi.ts b/src/flows/multi.ts index 42db6dfb..209dacab 100644 --- a/src/flows/multi.ts +++ b/src/flows/multi.ts @@ -98,7 +98,8 @@ async function processTarget(ctx: { injectMarker: boolean; channel: TargetChannel; }): Promise { - const { api, target, defaults, cfg, injectMarker, channel } = ctx; + const { target, defaults, cfg, injectMarker, channel } = ctx; + const api = channel.client(ctx.api); const fail = (richMessage: string): TargetResult => targetFailure(channel.io, richMessage); // Injection needs the typed labels, so it follows validation; the injected document is validated again inside. diff --git a/src/flows/redact.ts b/src/flows/redact.ts index d2c50c88..b2bd9aa4 100644 --- a/src/flows/redact.ts +++ b/src/flows/redact.ts @@ -8,6 +8,7 @@ import type { Target } from "../discovery/targets.js"; import type { RunOutcome } from "../engine/outcome.js"; +import { type GitHubClient, redactedClient } from "../github/api.js"; import type { RepoVisibility } from "../github/repo-visibility.js"; import { type SlugKey, slugKey } from "../github/slug.js"; import { type CollectedLine, type Io, prefixedIo } from "../io.js"; @@ -243,6 +244,9 @@ export interface TargetChannel { /** Sink for lines that already name their source (validation warnings): unprefixed, or the same capture. */ unprefixed: Io; + /** The client the target's requests go through: `api` itself in the clear, its redactedClient() view when redacted. */ + client(api: GitHubClient): GitHubClient; + /** Seals or opens the target's end state: everything in its detail but the slug, which the channel holds. */ close(end: Omit): TargetOutcome["detail"]; } @@ -252,6 +256,7 @@ export function publicChannel(io: Io, slug: string, attributed: boolean): Target display: slug, io: prefixedIo(io, attributed ? `${slug}: ` : ""), unprefixed: io, + client: (api) => api, close: ({ outcomes, note, file }) => ({ slug, outcomes, note, file }), }; } @@ -262,6 +267,7 @@ export function redactedChannel(io: Io, slug: string, display: string): TargetCh display, io: capture.io, unprefixed: capture.io, + client: redactedClient, close: ({ outcomes, note, file }) => markPrivate({ slug, outcomes, note, file, transcript: capture.drain() }), }; diff --git a/src/flows/single.ts b/src/flows/single.ts index 815403e2..c25b38b8 100644 --- a/src/flows/single.ts +++ b/src/flows/single.ts @@ -101,7 +101,7 @@ async function runTarget( async () => engineOutcome( await runForRepo( - api, + channel.client(api), { repo: cfg.repo, settings: injected.settings, diff --git a/src/flows/snapshot.ts b/src/flows/snapshot.ts index 82e0cce1..13f35536 100644 --- a/src/flows/snapshot.ts +++ b/src/flows/snapshot.ts @@ -280,7 +280,7 @@ async function snapshotFile( channel, () => snapshotTarget({ - api, + api: channel.client(api), repo: cfg.repo, cfg, path: cfg.snapshotFile, @@ -333,7 +333,7 @@ async function snapshotDir( channel, () => snapshotTarget({ - api, + api: channel.client(api), repo: target.repo, cfg, path: located.path, diff --git a/src/github/api.ts b/src/github/api.ts index dc8f9750..6105e9dc 100644 --- a/src/github/api.ts +++ b/src/github/api.ts @@ -1,8 +1,8 @@ /** - * The GitHubClient port and its one implementation, GitHubApi, on @octokit/core with the request-log, retry, - * throttling, and paginate-rest plugins; what a failed request becomes is api-error.ts's, and what a trace line may - * show is trace-redaction.ts's. A payload's JSON body is its own serialization (redactSecretPayloadSafe), never an - * endpoint typing that could drop an unknown field. + * The GitHubClient port, its view for a redacted target (redactedClient), and its one implementation, GitHubApi, on + * @octokit/core with the request-log, retry, throttling, and paginate-rest plugins; what a failed request becomes is + * api-error.ts's, and what a trace line may show is trace-redaction.ts's. A payload's JSON body is its own + * serialization (redactSecretPayloadSafe), never an endpoint typing that could drop an unknown field. */ import { Octokit } from "@octokit/core"; @@ -35,12 +35,7 @@ import { throttleGroups, } from "./scheduler.js"; import { redactSecretPayloadSafe } from "./secret-scan.js"; -import { - redactingOctokitLog, - repoSlugOf, - type TraceIo, - TraceRedaction, -} from "./trace-redaction.js"; +import { redactingOctokitLog, type TraceIo, TraceRedaction } from "./trace-redaction.js"; /** The single source for the header default here, the action.yml `api-version` default, and the inputs fallback; the action-yml contract test asserts the three stay equal. */ export const DEFAULT_API_VERSION = "2022-11-28"; @@ -55,14 +50,18 @@ export interface GraphqlOp { readonly query: string; } -/** - * `carriesSecret` marks a request whose payload or variables hold a resolved secret. The engine sets it from the act of - * resolving (engine/execute.ts) and withholds the request's error on its own side of this port whatever the client - * answers (sections/contract/requests.ts), so a caller-supplied client cannot leak an echoed value into an outcome or - * a report; GitHubApi honors the mark too, beside its field-name scan, for its direct callers. - */ +/** What a caller knows about a request that the client cannot read off it. */ export interface RequestMark { + /** + * The payload or variables hold a resolved secret. The engine sets it from the act of resolving (engine/execute.ts) + * and withholds the request's error on its own side of this port whatever the client answers + * (sections/contract/requests.ts), so a caller-supplied client cannot leak an echoed value into an outcome or a + * report; GitHubApi honors the mark too, beside its field-name scan, for its direct callers. + */ carriesSecret?: boolean; + + /** The request serves a redacted target, so its route and payload stay out of the trace; redactedClient() sets it. */ + redactTrace?: boolean; } /** @@ -76,7 +75,7 @@ export type ClientAnswer = { data: D } | { error: ApiError } | { failed: stri /** The page size every list walk asks GitHub for, GitHub's documented maximum, unless the endpoint caps lower. */ export const PAGE_SIZE = 100; -export interface ListOptions { +export interface ListOptions extends Pick { /** Items per page; PAGE_SIZE when omitted. An endpoint with a documented cap below it passes the cap. */ perPage?: number | undefined; @@ -85,15 +84,11 @@ export interface ListOptions { } export interface GitHubClient { - /** - * `redactTrace` holds the request's `/repos//` slug redacted for the request's duration, for the - * visibility probe, which must not leak the slug before it knows whether the repository is private. - */ tryRequest( method: string, path: string, payload?: unknown, - options?: RequestMark & { accept?: string; raw?: boolean; redactTrace?: boolean }, + options?: RequestMark & { accept?: string; raw?: boolean }, ): Promise>; /** @@ -116,7 +111,34 @@ export interface GitHubClient { ): Promise>>; } -/** The primary and secondary limits are handled identically but for the log `label`, so one factory keeps them from drifting. */ +/** One view per client, so what a section memoizes per client (the owner gate's probe) spans every redacted target of a run. */ +const redactedViews = new WeakMap(); + +/** + * The view of `api` a redacted target's requests go through: each carries `redactTrace`, so none prints its route or + * payload, whatever section sends it and whether or not the route names the repository. + */ +export function redactedClient(api: GitHubClient): GitHubClient { + const known = redactedViews.get(api); + if (known !== undefined) { + return known; + } + + const view: GitHubClient = { + tryRequest: (method, path, payload, options) => + api.tryRequest(method, path, payload, { ...options, redactTrace: true }), + tryList: (path, options) => api.tryList(path, { ...options, redactTrace: true }), + tryGraphql: (op, variables, slug, options) => + api.tryGraphql(op, variables, slug, { ...options, redactTrace: true }), + }; + redactedViews.set(api, view); + return view; +} + +/** + * The primary and secondary limits are handled identically but for the log `label`, so one factory keeps them from + * drifting. The line is written where the answer is decided, so it never announces a retry the answer refuses. + */ function throttleCallback( label: string, trace: TraceRedaction, @@ -127,10 +149,21 @@ function throttleCallback( retryCount: number, ) => boolean { return (retryAfter, options, _octokit, retryCount) => { - trace.debug( - `${label} on ${options.method} ${trace.path(options.url).path}; retry ${retryCount + 1}/${MAX_RETRIES} after ${retryAfter}s`, - ); - return retryAfter <= MAX_RETRY_WAIT_S && retryCount < MAX_RETRIES; + const limited = `${label} on ${options.method} ${trace.path(options.url).path}`; + if (retryCount >= MAX_RETRIES) { + trace.debug(`${limited}; giving up: the ${MAX_RETRIES} retries are spent`); + return false; + } + + if (retryAfter > MAX_RETRY_WAIT_S) { + trace.debug( + `${limited}; giving up: the wait of ${retryAfter}s is over the ${MAX_RETRY_WAIT_S}s cap`, + ); + return false; + } + + trace.debug(`${limited}; retry ${retryCount + 1}/${MAX_RETRIES} after ${retryAfter}s`); + return true; }; } @@ -276,25 +309,31 @@ export class GitHubApi implements GitHubClient { }); } - async tryRequest( + /** The port's three methods share one rule: a request marked `redactTrace` seals the trace until it settles. */ + tryRequest( method: string, path: string, payload?: unknown, - options?: RequestMark & { accept?: string; raw?: boolean; redactTrace?: boolean }, + options?: RequestMark & { accept?: string; raw?: boolean }, ): Promise> { - if (!options?.redactTrace) { - return this.request(method, path, payload, options); - } - const slug = repoSlugOf(path); - if (slug === undefined) { - throw new Error(`BUG: redactTrace needs a /repos// path, got ${path}`); - } - const release = this.trace.hold(slug); - try { - return await this.request(method, path, payload, options); - } finally { - release(); - } + return this.trace.during(options?.redactTrace === true, () => + this.request(method, path, payload, options), + ); + } + + tryList(path: string, options?: ListOptions): Promise> { + return this.trace.during(options?.redactTrace === true, () => this.list(path, options)); + } + + tryGraphql( + op: GraphqlOp, + variables: Readonly>, + slug: string, + options?: RequestMark, + ): Promise>> { + return this.trace.during(options?.redactTrace === true, () => + this.graphql(op, variables, slug, options), + ); } /** @@ -305,7 +344,7 @@ export class GitHubApi implements GitHubClient { * is noted before the plugin reads it as an empty list (GitHub's answer for an empty repository) and surfaces as the * error it is. */ - async tryList(path: string, options?: ListOptions): Promise> { + private async list(path: string, options?: ListOptions): Promise> { const pages: unknown[] = []; let conflict: OctokitHttpError | undefined; let current = path; @@ -495,7 +534,7 @@ export class GitHubApi implements GitHubClient { * any errors[] entry, even beside partial data -> { error }, so a section never acts on a half-answered query * `extensions.warnings` (legacy node-ID notices) -> the debug trace only */ - async tryGraphql( + private async graphql( op: GraphqlOp, variables: Readonly>, slug: string, diff --git a/src/github/repo-file.ts b/src/github/repo-file.ts index 71ddd4e0..53db562d 100644 --- a/src/github/repo-file.ts +++ b/src/github/repo-file.ts @@ -6,12 +6,15 @@ * -> that branch's git ref is read, which needs Contents: read and succeeds whether or not the file exists * -> the token could have read the file, so it is missing * - * A failed ref read leaves the proof inconclusive: a denied grant, or an empty repository whose branch has no commit. + * A refused ref read leaves the proof inconclusive: a denied grant (403, 404), or an empty repository (409, no commit yet). */ import type { GitHubClient } from "./api.js"; import { type ApiError, classifyApiError } from "./api-error.js"; +/** The status of a ref read on a repository with no commit. */ +const EMPTY_REPOSITORY = 409; + export async function getRepoFile( api: GitHubClient, slug: string, @@ -64,7 +67,9 @@ export async function getRepoFile( if (!("error" in refProbe)) { return { missing: true }; } - if (classifyApiError(refProbe.error) !== "permission") { + const inconclusive = + refProbe.error.status === EMPTY_REPOSITORY || classifyApiError(refProbe.error) === "permission"; + if (!inconclusive) { return { error: refProbe.error }; } return { diff --git a/src/github/repo-visibility.ts b/src/github/repo-visibility.ts index c0eb9dd1..b0d88b0a 100644 --- a/src/github/repo-visibility.ts +++ b/src/github/repo-visibility.ts @@ -26,7 +26,7 @@ export function createVisibilityResolver( async function probe(api: GitHubClient, slug: string): Promise { // The probe decides redaction, so its own trace (and any throttle-callback trace) must fail closed before the answer - // is known: redactTrace holds the slug redacted for the request's duration. + // is known: the redactTrace mark seals the trace until the request settles. const result = await api.tryRequest("GET", `/repos/${slug}`, undefined, { redactTrace: true }); if ("failed" in result || "error" in result) { return "unknown"; diff --git a/src/github/trace-redaction.ts b/src/github/trace-redaction.ts index 2fa05d5c..da45fe72 100644 --- a/src/github/trace-redaction.ts +++ b/src/github/trace-redaction.ts @@ -1,28 +1,28 @@ /** - * Slug redaction of the client's trace lines: a /repos// slug collapses its line while the Io port masks - * it or an in-flight request holds it (GitHubApi's redactTrace, for the visibility probe), and octokit's plugin log - * runs through the same scan (redactingOctokitLog). TraceIo is the slice of Io the client (api.ts) takes for this. + * What the client's trace lines may show of a redacted target; TraceIo is the slice of Io the client (api.ts) takes + * for it. Two rules, either one enough: + * + * a request marked as serving a redacted target -> the trace is sealed until it settles (during) + * a slug masked through the Io port -> every line naming it collapses, whoever sent the request + * + * Octokit's plugin log runs through the same rules (redactingOctokitLog). */ import type { Io } from "../io.js"; -import { SLUG_SEGMENT, type SlugKey, slugKey } from "./slug.js"; +import { SLUG_SEGMENT, slugKey } from "./slug.js"; export type TraceIo = Pick; // The segment charset stops at the segment boundary so an octokit line's trailing " - 204 with id ..." is never folded into the name, and the `i` flag keeps a mixed-case path from slipping the redaction. const REPO_SLUG = new RegExp(`/repos/(${SLUG_SEGMENT}/${SLUG_SEGMENT})`, "i"); -export function repoSlugOf(path: string): string | undefined { - return path.match(REPO_SLUG)?.[1]; -} +// A collapsed path and a collapsed plugin line read the same on every route, so the constant carries nothing of either. +const REDACTED = ""; -/** - * A slug is redacted while it is masked through the Io port or held by an in-flight request (the visibility probe). A - * traced payload is private content no mask covers, so it is dropped. - */ +/** A traced payload is private content no mask covers, so a redacted path drops it. */ export class TraceRedaction { - // One token per hold, so concurrent holds on the same slug release independently and a double release is inert. - private readonly holds = new Set<{ readonly slug: SlugKey }>(); + /** How many requests in flight serve a redacted target. */ + private sealed = 0; constructor(private readonly io: TraceIo) {} @@ -30,12 +30,22 @@ export class TraceRedaction { this.io.debug(line); } - hold(slug: string): () => void { - const token = { slug: slugKey(slug) }; - this.holds.add(token); - return () => { - this.holds.delete(token); - }; + /** + * Runs `request`; when it serves a redacted target, no line written until it settles shows a path or a payload. + * The seal is the whole trace's, not the request's: a plugin logs through the client, so its line for this + * request cannot be told from its line for another one in flight beside it. + */ + async during(servesRedactedTarget: boolean, request: () => Promise): Promise { + if (!servesRedactedTarget) { + return request(); + } + + this.sealed += 1; + try { + return await request(); + } finally { + this.sealed -= 1; + } } isRedacted(slug: string): boolean { @@ -53,9 +63,9 @@ export class TraceRedaction { * anything but a constant leaks what redaction hides. */ path(path: string): { path: string; redacted: boolean } { - const slug = repoSlugOf(path); - if (slug && this.isRedacted(slug)) { - return { path: "", redacted: true }; + const slug = path.match(REPO_SLUG)?.[1]; + if (this.sealed > 0 || (slug !== undefined && this.isRedacted(slug))) { + return { path: REDACTED, redacted: true }; } return { path, redacted: false }; } @@ -65,19 +75,20 @@ export class TraceRedaction { * needle as a case-insensitive substring collapses the whole line. */ message(message: string): string { + if (this.sealed > 0) { + return REDACTED; + } + const lower = message.toLowerCase(); for (const needle of this.needles()) { if (lower.includes(needle)) { - return ""; + return REDACTED; } } return message; } private *needles(): Iterable { - for (const token of this.holds) { - yield token.slug; - } for (const value of this.io.masked()) { // An empty mask would match every line. if (value !== "") { diff --git a/test/github/api.test.ts b/test/github/api.test.ts index 6bd3055f..36a9b40d 100644 --- a/test/github/api.test.ts +++ b/test/github/api.test.ts @@ -3,8 +3,11 @@ import { parse as parseYaml } from "yaml"; import { DEFAULT_API_VERSION, GitHubApi, + type GitHubClient, + type GraphqlOp, MAX_RETRIES, MAX_RETRY_WAIT_S, + redactedClient, } from "../../src/github/api.js"; import { type ApiError, @@ -18,7 +21,7 @@ import { TIMERS_SCHEDULER, } from "../../src/github/scheduler.js"; import { redactingOctokitLog, TraceRedaction } from "../../src/github/trace-redaction.js"; -import { api, restoreFetch, stubFetch, traceIo } from "./stub.js"; +import { api, json, restoreFetch, stubFetch, timeless, traceIo } from "./stub.js"; afterEach(restoreFetch); @@ -31,6 +34,12 @@ const rateLimited = () => headers: { "retry-after": "0", "x-ratelimit-remaining": "0" }, }); +const READ_OP: GraphqlOp = { + name: "RepoId", + kind: "read", + query: 'query RepoId($owner: String!) { repository(owner: $owner, name: "r") { id } }', +}; + describe("retry and throttling", () => { test("429 rate limits are retried until they succeed", async () => { const state = stubFetch([rateLimited, rateLimited, okJson]); @@ -104,6 +113,56 @@ describe("retry and throttling", () => { expect("error" in result && result.error.message).toBe('{"message":"rate limited"}'); expect("error" in result && result.error.documentationUrl).toBeUndefined(); }); + + // The throttle line is the trace's only record of what the client did about a limit: it once announced a retry the + // callback then refused, so a run that failed at once read as one that had waited. + const primaryLimit = (resetInSeconds: number) => () => + new Response('{"message":"API rate limit exceeded for user ID 1."}', { + status: 403, + headers: { + "content-type": "application/json", + "x-ratelimit-remaining": "0", + "x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + resetInSeconds), + }, + }); + test.each< + [ + name: string, + send: (client: GitHubApi) => Promise, + response: () => Response, + requests: number, + lines: string[], + ] + >([ + [ + "a primary limit whose reset is beyond the cap gives up with one request sent", + (client) => client.tryRequest("GET", "/repos/o/r"), + primaryLimit(3000), + 1, + [ + `rate limit on GET /repos/o/r; giving up: the wait of Ns is over the ${MAX_RETRY_WAIT_S}s cap`, + ], + ], + [ + "a GraphQL RATE_LIMITED is retried, then given up once the retries are spent", + (client) => client.tryGraphql(READ_OP, { owner: "o" }, "o/r"), + json({ data: null, errors: [{ type: "RATE_LIMITED", message: "slow down" }] }), + 1 + MAX_RETRIES, + [ + `rate limit on POST /graphql; retry 1/${MAX_RETRIES} after 0s`, + `rate limit on POST /graphql; retry 2/${MAX_RETRIES} after 0s`, + `rate limit on POST /graphql; giving up: the ${MAX_RETRIES} retries are spent`, + ], + ], + ])("the throttle trace says what follows: %s", async (_name, send, response, requests, lines) => { + const state = stubFetch([response]); + const trace = traceIo(); + await send(api(trace.io)); + expect(state.calls).toBe(requests); + // The wait is read off the clock (3000 or 3001 here), so its number is folded. + const limited = trace.lines.filter((line) => line.startsWith("rate limit on")); + expect(limited.map((line) => line.replace(/wait of \d+s/, "wait of Ns"))).toEqual(lines); + }); }); describe("GSAC_RETRY_BASE_MS: millisecond units and the immediate scheduler, same plugin topology", () => { @@ -482,27 +541,78 @@ describe("debug-trace hardening for redacted slugs", () => { expect(dbg.lines).toEqual(["", ""]); }); - test("redactTrace holds the slug for the request only; the same client traces it legibly afterwards", async () => { - // The visibility probe must not leak its target before the answer is known; once the flow decided it is public, the same slug is legible again. - stubFetch([() => new Response(null, { status: 204 })]); - const dbg = traceIo(); - const client = api(dbg.io); - await client.tryRequest("GET", "/repos/owner/probed", undefined, { redactTrace: true }); - // Octokit's own request lines ride the same channel, so the whole window must stay free of the slug. - const held = dbg.lines.join("\n"); - expect(held).toContain("GET -> 204"); - expect(held).not.toContain("owner/probed"); - dbg.lines.length = 0; - await client.tryRequest("GET", "/repos/owner/probed"); - expect(dbg.lines.join("\n")).toContain("GET /repos/owner/probed -> 204"); - // A hold needs a slug to hold: a slug-free path cannot be traced redacted. - await expect( - client.tryRequest("GET", "/user", undefined, { redactTrace: true }), - ).rejects.toThrow(/redactTrace needs a/); - }); + // No slug is masked in this table: the mark alone decides, so a route that names no repository hides too. Each row + // is sent marked, then unmarked through the same client, which proves the seal lifts and shows what it hid. + test.each< + [ + name: string, + send: (client: GitHubApi, mark: { redactTrace?: boolean }) => Promise, + response: () => Response, + own: string, + hidden: string, + ] + >([ + [ + "the visibility probe", + (client, mark) => client.tryRequest("GET", "/repos/owner/probed", undefined, mark), + json({ private: true }), + "GET -> 200 (Nms)", + "GET /repos/owner/probed -> 200", + ], + [ + "the owner probe", + (client, mark) => client.tryRequest("GET", "/orgs/secret-org", undefined, mark), + json({ login: "secret-org" }), + "GET -> 200 (Nms)", + "GET /orgs/secret-org -> 200", + ], + [ + "the app lookup", + (client, mark) => client.tryRequest("GET", "/apps/internal-deploy-bot", undefined, mark), + json({ slug: "internal-deploy-bot" }), + "GET -> 200 (Nms)", + "GET /apps/internal-deploy-bot -> 200", + ], + [ + "a write and its payload", + (client, mark) => + client.tryRequest("PATCH", "/orgs/secret-org", { description: "CANARY" }, mark), + json({}), + "PATCH -> 200 (Nms)", + 'payload: {"description":"CANARY"}', + ], + [ + "a list walk", + (client, mark) => client.tryList("/orgs/secret-org/teams", mark), + json([]), + "GET -> 200 (Nms)", + "GET /orgs/secret-org/teams?per_page=100&page=1 -> 200", + ], + [ + "a GraphQL operation and its variables", + (client, mark) => + client.tryGraphql(READ_OP, { owner: "secret-org" }, "secret-org/private-repo", mark), + json({ data: { repository: { id: "R_1" } } }), + "", + 'variables: {"owner":"secret-org"}', + ], + ])( + "a request marked as serving a redacted target leaves no route or payload in the trace: %s", + async (_name, send, response, own, hidden) => { + stubFetch([response]); + const dbg = traceIo(); + const client = api(dbg.io); + await send(client, { redactTrace: true }); + // The request-log plugin's two lines for the attempt, then the client's own. + expect(dbg.lines.map(timeless)).toEqual(["", "", own]); + dbg.lines.length = 0; + await send(client, {}); + expect(dbg.lines.join("\n")).toContain(hidden); + }, + ); test("a rate-limited visibility probe leaks no raw slug in any trace", async () => { - // The throttle-callback trace fires on the 429 retry, before the probe result exists, so the probe's request-window hold is what redacts it. + // The throttle-callback trace fires on the 429 retry, before the probe result exists, so the seal the probe's mark sets is what redacts it. const { createVisibilityResolver } = await import("../../src/github/repo-visibility.js"); stubFetch([ rateLimited, @@ -1231,21 +1341,66 @@ describe("DELETE request bodies reach the wire", () => { }); }); -describe("TraceRedaction holds", () => { - test("overlapping holds on one slug are independent: releasing one, even twice, keeps the other's redaction", () => { +describe("TraceRedaction.during", () => { + test("overlapping requests for redacted targets keep the trace sealed until the last one settles, a rejected one included", async () => { + // A seal the first settled request lifted would print the plugin lines of the one still in flight. const trace = new TraceRedaction(traceIo().io); - const releaseFirst = trace.hold("o/probed"); - const releaseSecond = trace.hold("O/Probed"); - releaseFirst(); - releaseFirst(); // a repeated release must not consume the other hold - expect(trace.path("/repos/o/probed/labels")).toEqual({ path: "", redacted: true }); - expect(trace.message("retrying o/probed after 429")).toBe(""); - releaseSecond(); - expect(trace.path("/repos/o/probed/labels")).toEqual({ - path: "/repos/o/probed/labels", - redacted: false, - }); - expect(trace.message("retrying o/probed after 429")).toBe("retrying o/probed after 429"); - expect(trace.isRedacted("o/probed")).toBe(false); + const first = Promise.withResolvers(); + const second = Promise.withResolvers(); + const firstRequest = trace.during(true, () => first.promise); + const secondRequest = trace.during(true, () => second.promise); + first.resolve(); + await firstRequest; + expect(trace.path("/orgs/o")).toEqual({ path: "", redacted: true }); + expect(trace.message("GET /orgs/o - 200 with id A1 in 1ms")).toBe(""); + second.reject(new Error("socket hang up")); + await expect(secondRequest).rejects.toThrow("socket hang up"); + expect(trace.path("/orgs/o")).toEqual({ path: "/orgs/o", redacted: false }); + expect(trace.message("GET /orgs/o - 200 with id A1 in 1ms")).toBe( + "GET /orgs/o - 200 with id A1 in 1ms", + ); + }); +}); + +describe("redactedClient", () => { + test("every request through the view reaches the client marked, the caller's own options kept", async () => { + // A method of the view that forgot the mark would print its route; one that dropped the caller's options would + // lose carriesSecret, and with it the withholding of a secret's echo. + const seen: unknown[][] = []; + const client: GitHubClient = { + tryRequest: async (...args) => { + seen.push(args); + return { data: null }; + }, + tryList: async (...args) => { + seen.push(args); + return { data: [] }; + }, + tryGraphql: async (...args) => { + seen.push(args); + return { data: {} }; + }, + }; + const view = redactedClient(client); + await view.tryRequest( + "PUT", + "/orgs/o/teams/t", + { permission: "push" }, + { carriesSecret: true }, + ); + await view.tryList("/orgs/o/teams", { perPage: 30 }); + await view.tryGraphql(READ_OP, { owner: "o" }, "o/r"); + expect(seen).toEqual([ + [ + "PUT", + "/orgs/o/teams/t", + { permission: "push" }, + { carriesSecret: true, redactTrace: true }, + ], + ["/orgs/o/teams", { perPage: 30, redactTrace: true }], + [READ_OP, { owner: "o" }, "o/r", { redactTrace: true }], + ]); + // One view per client: the owner gate memoizes its probe per client, so a view per target would probe once per redacted target. + expect(redactedClient(client)).toBe(view); }); }); diff --git a/test/github/redacted-target-trace.test.ts b/test/github/redacted-target-trace.test.ts new file mode 100644 index 00000000..b16aeb57 --- /dev/null +++ b/test/github/redacted-target-trace.test.ts @@ -0,0 +1,167 @@ +/** + * The debug trace of a whole run over a redacted target: the flow, the engine, and the real sections over GitHubApi, + * against the e2e mock server. The owner probe (GET /orgs/) is the request under watch: it serves the target + * and carries no repository slug, so only the run's knowledge that the target is redacted keeps it out of the trace. + */ + +import { describe, expect, test } from "bun:test"; +import { writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { stringify as stringifyYaml } from "yaml"; +import { DEFAULT_DISCOVERY_FILTERS } from "../../src/discovery/discover.js"; +import { parseRepoSlug } from "../../src/discovery/targets.js"; +import { SectionSelection } from "../../src/engine/section-selection.js"; +import { runMulti } from "../../src/flows/multi.js"; +import { runSingle } from "../../src/flows/single.js"; +import { runSnapshot } from "../../src/flows/snapshot.js"; +import { GitHubApi, type GitHubClient } from "../../src/github/api.js"; +import { type Io, silentIo } from "../../src/io.js"; +import { E2E_TOKEN } from "../e2e/constants.js"; +import type { MockHandle } from "../e2e/mock/server.js"; +import { mockServerLifecycle, scenario } from "../e2e/mock/server-test-support.js"; +import { withTempDir } from "../temp-dir.js"; +import { timeless } from "./stub.js"; + +const start = mockServerLifecycle(); +const PUBLIC = "acme/app"; +const PRIVATE = "secret-org/private-repo"; +const TEAMS = { teams: [{ name: "reviewers", permission: "push" }] }; +const REDACT = { privateRepos: "redact" as const, selfSlug: "acme/admin" }; +const CHECK = { + ...REDACT, + mode: "check" as const, + onMissingPermission: "fail" as const, + sections: SectionSelection.ALL, + privateReport: "none" as const, + reportPublicKey: "", + runUrl: "", +}; +const SNAPSHOT = { + ...REDACT, + onMissingPermission: "fail" as const, + sections: SectionSelection.of({ only: ["teams"] })._unsafeUnwrap(), +}; +const TARGETS = { + reposDir: "", + adminOwner: "acme", + discoveryFilters: DEFAULT_DISCOVERY_FILTERS, + discoveryFiltersSet: [], +}; + +/** What one run shares: the mock GitHub, a client pointed at it, and the Io whose debug trace `lines` keeps. */ +interface Run { + mock: MockHandle; + client: GitHubClient; + io: Io; + lines: string[]; +} + +/** A run over a mock GitHub holding the public and the private repository, each declaring the same team. */ +async function startRun(): Promise { + const mock = await start( + scenario({ + repos: { + [PUBLIC]: { settings: TEAMS }, + [PRIVATE]: { + settings: TEAMS, + live_state: { repo: { private: true, visibility: "private" } }, + }, + }, + }), + ); + const lines: string[] = []; + const io: Io = { ...silentIo(), debug: (line) => lines.push(line) }; + const client = new GitHubApi({ token: E2E_TOKEN, io, baseUrl: mock.url, retryBaseMs: 1 }); + return { mock, client, io, lines }; +} + +/** Whether a request path, or a trace line, is the owner probe: /orgs/ and nothing under it. */ +function isOwnerProbe(text: string): boolean { + return /\/orgs\/[\w-]+( |$)/.test(text); +} + +/** The owner probes the mock received, in order. */ +function ownerProbes(mock: MockHandle): string[] { + return mock.requests.map((request) => request.pathname).filter(isOwnerProbe); +} + +describe("the debug trace of a run over a redacted target", () => { + // One row per place a flow hands a target's client to the engine: a flow that handed over the run's own client + // printed the probe, since nothing in its path says whom it serves. + test.each<[flow: string, run: (client: GitHubClient, io: Io, dir: string) => Promise]>([ + [ + "a single-repo check", + async (client, io, dir) => { + const settingsFile = join(dir, "settings.yml"); + writeFileSync(settingsFile, stringifyYaml(TEAMS)); + const repo = parseRepoSlug(PRIVATE)._unsafeUnwrap(); + return (await runSingle(client, { ...CHECK, repo, settingsFile }, io)).isOk(); + }, + ], + [ + "a multi-repo check", + async (client, io) => { + const fleet = { ...CHECK, ...TARGETS, reposInput: PRIVATE, defaultsFile: "" }; + return (await runMulti(client, fleet, io)).isOk(); + }, + ], + [ + "a snapshot to a file", + async (client, io, dir) => { + const repo = parseRepoSlug(PRIVATE)._unsafeUnwrap(); + const snapshotFile = join(dir, "snapshot.yml"); + const file = { ...SNAPSHOT, form: "file" as const, repo, snapshotFile }; + return (await runSnapshot(client, file, io)).isOk(); + }, + ], + [ + "a snapshot to a directory", + async (client, io, dir) => { + const snapshotDir = join(dir, "out"); + const fleet = { + ...SNAPSHOT, + ...TARGETS, + form: "dir" as const, + reposInput: PRIVATE, + snapshotDir, + }; + return (await runSnapshot(client, fleet, io)).isOk(); + }, + ], + ])("%s of a private target leaves only redacted lines", (_flow, run) => + withTempDir("sac-redacted-trace-", async (dir) => { + const { mock, client, io, lines } = await startRun(); + expect(await run(client, io, dir)).toBe(true); + // The controls: the probe reached the mock, which answered every request by its contract, so the probe's + // absence from the trace is redaction, not a request never made. + expect(ownerProbes(mock)).toEqual(["/orgs/secret-org"]); + expect(mock.violations).toEqual([]); + // Every request of the run serves the one redacted target, so the whole trace is the two redacted forms: a + // plugin's line, and the client's own, which keeps the method and the status. + const forms = lines.map((line) => line.replace(/-> \d+ \(\d+ms\)/, "-> NNN (Nms)")); + expect([...new Set(forms)].sort()).toEqual(["", "GET -> NNN (Nms)"]); + }), + ); + + test("a multi-repo run prints the public target's owner probe beside the private target's hidden one", async () => { + // Both targets send the same slug-free probe, so this is the pair a seal left on, or never set, cannot pass. + const { mock, client, io, lines } = await startRun(); + const fleet = { ...CHECK, ...TARGETS, reposInput: `${PUBLIC},${PRIVATE}`, defaultsFile: "" }; + const run = await runMulti(client, fleet, io); + expect(run.map((targets) => targets.map((target) => [target.display, target.result]))).toEqual( + expect.objectContaining({ + value: [ + [PUBLIC, "drift"], + ["private repository #1", "drift"], + ], + }), + ); + expect(ownerProbes(mock)).toEqual(["/orgs/acme", "/orgs/secret-org"]); + expect(mock.violations).toEqual([]); + expect(lines.filter(isOwnerProbe).map(timeless)).toEqual([ + "GET /orgs/acme - 200 with id undefined in Nms", + "GET /orgs/acme -> 200 (Nms)", + ]); + expect(lines.filter((line) => /secret-org|private-repo/.test(line))).toEqual([]); + }); +}); diff --git a/test/github/repo-file.test.ts b/test/github/repo-file.test.ts index ec923bfa..f8cb3fb3 100644 --- a/test/github/repo-file.test.ts +++ b/test/github/repo-file.test.ts @@ -63,14 +63,14 @@ describe("getRepoFile", () => { test.each([ [ "404", - "is an inconclusive proof naming the grant and the empty-branch case, never a missing file", + "is an inconclusive proof naming the grant, never a missing file", notFound, 1, unproven(404), ], [ "403", - "is an inconclusive proof naming the grant and the empty-branch case, never a missing file", + "is an inconclusive proof naming the grant, never a missing file", forbidden, 1, unproven(403), @@ -102,6 +102,19 @@ describe("getRepoFile", () => { expect(result).toEqual(expected); }); + test("an empty repository is an inconclusive proof naming the uninitialized branch, never a failure to re-run", async () => { + // What GitHub answers for a repository with no commit: the contents read 404s with its own message and the ref + // read 409s. Read as a failure, the 409 told the operator to re-run a request no re-run can change. + const state = stubFetch([ + () => json(404, { message: "This repository is empty." }), + repo(), + () => json(409, { message: "Git Repository is empty." }), + ]); + const result = await getRepoFile(api(), "o/r", FILE); + expect(state.paths).toEqual([CONTENTS, REPO, REF]); + expect(result).toEqual(unproven(409)); + }); + test("a contents 404 on an invisible repo surfaces the repo-level error before any ref read", async () => { const state = stubFetch([notFound, notFound]); const result = await getRepoFile(api(), "o/r", FILE); diff --git a/test/github/stub.ts b/test/github/stub.ts index 7a877a9d..250aeff6 100644 --- a/test/github/stub.ts +++ b/test/github/stub.ts @@ -45,6 +45,13 @@ export function stubFetch(responses: Array<() => Response>): { return state; } +/** A stubbed JSON answer as a factory, the shape stubFetch and a route table take, so every request reads a fresh body. */ +export const json = (body: unknown) => (): Response => + new Response(JSON.stringify(body), { headers: { "content-type": "application/json" } }); + +/** A trace line with its timing folded, so a whole line compares. */ +export const timeless = (line: string): string => line.replace(/\d+ms/, "Nms"); + /** A fresh trace facet per test, with an isolated mask registry so one test's masks never redact another's traces. */ export function traceIo(): { io: TraceIo & Pick; lines: string[] } { const lines: string[] = [];