* fix(log-audit): kill 404 noise from `/api/github/installation-token` at source (#693) Closes #693. Issue diagnosed a surface symptom (`log.error` on expected 404s) but missed the actual root causes. Investigation revealed two distinct populations producing identical 3-call 404 bursts: 1. **Fork-CI on `pullfrog/pullfrog`**: `test-token.yml` and `trigger-sync.yml` ship with `on: push: main`, so every fork inherits them and 404s our token endpoint on first push. Self-inflicted noise that scales with fork count. 2. **Real users hitting the full action without installing the App**: `/api/repo/.../run-context` uses the caller's `GITHUB_TOKEN` to read the repo from GitHub and then unconditionally lazy-provisions Account+Repo rows via `fetchOrCreateRepo`, even when the App isn't installed. Generates phantom DB rows and false `new account created` team@ alerts. (Confirmed via Prisma: `ezcorp-org` has an Account row with `installerLogin: null`, never installed our App.) Both populations then trip the client retry loop in `acquireTokenViaOIDC`, which matched `"Token exchange failed"` and retried 3× on terminal 4xx — tripling log volume and wasting CI time. ## Changes - `action/.github/workflows/{test-token,trigger-sync}.yml`: gate jobs with `if: github.repository == 'pullfrog/pullfrog'`. Forks inherit the files but the jobs no-op. - `app/api/repo/[owner]/[repo]/run-context/route.ts`: call `getRepoInstallation` first; return 404 with install URL if the App isn't installed, before any DB writes or GitHub repo fetch. - `action/utils/github.ts`: introduce `TokenExchangeError` for non-2xx server responses; `acquireNewToken` no longer retries it. Retry now fires only on genuine network/timeout failures. 404 surfaces a user-actionable error pointing at the install URL. - `app/api/github/installation-token/route.ts`: move `log.error` inside the 500 branch only. 404 branch is silent (expected user-state) and returns the same install URL message for consistency. ## Effect - Better Stack `level=error` lines from this path: 6/day → 0. - Failed user-trial CI time: 3 wasted token requests → 1. - User-facing error: opaque `Token exchange failed: 404` → actionable install URL. - No more phantom Account rows from never-installed callers. Skipped per design discussion: phantom-account cleanup (conservative — stop the bleed, leave history), `AGENTS.md` rule (overgeneralized). * review: address oracle leak + per-env install URL + retryable 5xx Addresses pullfrog[bot] (IMPORTANT) and Copilot review findings on #708: - **Install-status oracle in `run-context`** [pullfrog, Copilot]: `getRepoInstallation` runs with our App's JWT, *before* the caller's bearer token is validated against the repo. Pre-PR the route was uniformly bad-token-shaped; the new install-specific 404 turned it into an unauthenticated oracle distinguishing "Pullfrog installed here" from "not installed". Collapsed the 404 message to match the outer catch's ambiguous "repository not found or token lacks access". Legit runners still get the actionable install URL from `/api/github/installation-token`, which IS gated by OIDC. - **Hardcoded `github.com/apps/pullfrog`** [Copilot]: server-side `installation-token` now uses `GITHUB_APP_INSTALL_URL` from `app/globals.ts`, so dev/staging deployments with a different `GITHUB_APP_SLUG` direct users to the correct app. Action-side echoes the server's `error` body when present (single source of truth) and falls back to a generic message only if the body isn't JSON. - **Transient 5xx/429 made terminal** [Copilot]: `shouldRetry` now returns `true` for `TokenExchangeError` with `status >= 500` or `status === 429`. 4xx remains terminal (the actual #693 fix). Real outages no longer fail the workflow immediately. - **Stale comment** [pullfrog, Copilot]: reworded the comment at `installation-token/route.ts:141` to reflect the new retry policy ("the action surfaces this once (no retry)" instead of "the action retries on this"). * review: restore caller-token-first auth in run-context Pre-PR, `getEnrichedRepo({owner, repo, token})` used the caller's token as the auth boundary — `getRepo({token})` succeeding was the proof-of-access check. My initial install-gate inverted the order and ran the App-credentialed `getRepoInstallation` first, which is how it became: - an install-status oracle (pullfrog bot, addressed previously by matching the outer-catch wording), and - an outbound amplifier against our App JWT for arbitrary `owner/repo` (pullfrog bot, this commit). Reordered so `getRepo({token})` runs first. Garbage / unauthorized bearers get rejected by github (mapped to 403 by the outer catch) before any App-credentialed call fires. `getRepo` is cached 5min, so `getEnrichedRepo` below remains a free re-hit.
This commit is contained in:
Colin McDonnell
committed by
pullfrog[bot]
pullfrog[bot]
parent
4260984257
commit
60cc8772a6
@@ -11,6 +11,10 @@ permissions:
|
||||
|
||||
jobs:
|
||||
test-token:
|
||||
# only run in the upstream publish target. forks inherit this file but
|
||||
# haven't installed the pullfrog github app — running it there 404s our
|
||||
# token endpoint and pollutes our error logs (see #693).
|
||||
if: github.repository == 'pullfrog/pullfrog'
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- name: Get installation token
|
||||
|
||||
@@ -10,8 +10,10 @@ permissions:
|
||||
|
||||
jobs:
|
||||
trigger:
|
||||
# skip if pushed by our bot (breaks the loop)
|
||||
if: github.actor != 'pullfrog[bot]'
|
||||
# only run in the upstream publish target (forks inherit this file but
|
||||
# can't dispatch into pullfrog/app), and skip if pushed by our bot (breaks
|
||||
# the loop).
|
||||
if: github.repository == 'pullfrog/pullfrog' && github.actor != 'pullfrog[bot]'
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: actions/checkout@v6
|
||||
|
||||
+46
-8
@@ -99,6 +99,22 @@ type AcquireTokenOptions = {
|
||||
permissions?: GitHubAppPermissions;
|
||||
};
|
||||
|
||||
/**
|
||||
* Thrown when our token-exchange endpoint returns a non-2xx response.
|
||||
* The retry policy in `acquireNewToken` looks for this concrete type to
|
||||
* skip retries — 4xx is terminal user state (not-installed, not-authorized)
|
||||
* and 5xx is rare enough that re-running the workflow is the right escape
|
||||
* hatch. Genuine network failures throw plain `Error` and stay retryable.
|
||||
*/
|
||||
class TokenExchangeError extends Error {
|
||||
readonly status: number;
|
||||
constructor(status: number, message: string) {
|
||||
super(message);
|
||||
this.name = "TokenExchangeError";
|
||||
this.status = status;
|
||||
}
|
||||
}
|
||||
|
||||
async function acquireTokenViaOIDC(opts?: AcquireTokenOptions): Promise<string> {
|
||||
const oidcToken = await core.getIDToken("pullfrog-api");
|
||||
|
||||
@@ -128,7 +144,22 @@ async function acquireTokenViaOIDC(opts?: AcquireTokenOptions): Promise<string>
|
||||
clearTimeout(timeoutId);
|
||||
|
||||
if (!tokenResponse.ok) {
|
||||
throw new Error(`Token exchange failed: ${tokenResponse.status} ${tokenResponse.statusText}`);
|
||||
// prefer the server-side `error` field — it's the single source of
|
||||
// truth for the install URL (uses GITHUB_APP_INSTALL_URL, which
|
||||
// varies per env / GITHUB_APP_SLUG). fall back to a generic message
|
||||
// if the body isn't JSON or doesn't carry an `error` field.
|
||||
let serverMessage: string | undefined;
|
||||
try {
|
||||
const body = (await tokenResponse.json()) as { error?: unknown };
|
||||
if (typeof body.error === "string") serverMessage = body.error;
|
||||
} catch {
|
||||
// body wasn't JSON — fall through to the generic message
|
||||
}
|
||||
throw new TokenExchangeError(
|
||||
tokenResponse.status,
|
||||
serverMessage ??
|
||||
`Token exchange failed: ${tokenResponse.status} ${tokenResponse.statusText}`
|
||||
);
|
||||
}
|
||||
|
||||
const tokenData = (await tokenResponse.json()) as InstallationToken;
|
||||
@@ -333,13 +364,20 @@ export async function acquireNewToken(opts?: AcquireTokenOptions): Promise<strin
|
||||
if (isOIDCAvailable()) {
|
||||
return await retry(() => acquireTokenViaOIDC(opts), {
|
||||
label: "token exchange",
|
||||
shouldRetry: (error) =>
|
||||
error instanceof Error &&
|
||||
(error.name === "AbortError" ||
|
||||
error.message.includes("fetch failed") ||
|
||||
error.message.includes("ECONNRESET") ||
|
||||
error.message.includes("ETIMEDOUT") ||
|
||||
error.message.includes("Token exchange failed")),
|
||||
shouldRetry: (error) => {
|
||||
// 4xx is terminal user state (app not installed, permissions wrong) —
|
||||
// retrying just triples our log noise and the user's CI bill (see
|
||||
// #693). 5xx/429 are transient (vercel cold start, github outage,
|
||||
// rate limit) and should ride the existing backoff.
|
||||
if (error instanceof TokenExchangeError) return error.status >= 500 || error.status === 429;
|
||||
return (
|
||||
error instanceof Error &&
|
||||
(error.message.includes("timed out") ||
|
||||
error.message.includes("fetch failed") ||
|
||||
error.message.includes("ECONNRESET") ||
|
||||
error.message.includes("ETIMEDOUT"))
|
||||
);
|
||||
},
|
||||
});
|
||||
} else {
|
||||
// local development via GitHub App
|
||||
|
||||
Reference in New Issue
Block a user