Skip to content

Commit b771120

Browse files
serge-the-devclaude
andcommitted
fix(github): the hourly token refresh keeps the account name the binding wrote, so the ETag cache stays on (#651)
`cacheInstallationToken` took the account as an OPTIONAL parameter and bound `account?.login ?? ""` into an `ON CONFLICT … DO UPDATE SET account_login = excluded.account_login`. Two of its three callers supply an account — `authorizeInstallation` and `bindMemberOrgInstallations`, both of which have just verified who owns the installation. The third, `installationTokenResult`, is the token refresh and supplies nothing. A GitHub installation token lives one hour, so every binding row fell through to that refresh at least hourly and blanked its own `account_login` — and on EVERY call when `KEY_ENCRYPTION_KEY` is unset, because the freshness branch additionally requires the stored ciphertext. `githubAuthContext` (github-cache.ts:121-127) names a read's authority by matching that column against the repo owner. `''` matches no owner, so it returned `null` — the module's documented fail-closed value — and both `readGithubCache` and its write sibling short-circuit on a falsy context. So from the first refresh onward, permanently, no authenticated GitHub read was conditionally cached: deploy-watch.ts:243, hosted-repo.ts:155/:205, github-issues.ts:154/:180, github-prs.ts:280/:340. deploy-watch's cron polls per repo per tick, which is direct exposure to GitHub's primary rate limit. `GET /v1/github/installations` also returned blank org names. Fixed at the writer, not the reader: the UPDATE clause includes the account columns only when the caller passed an account, so the refresh has no opinion about a column it cannot know. The INSERT still writes '' into the NOT NULL columns and is unreachable from the refresh path, which returns `no-binding` before minting when the row is absent. Verified NOT affected, as the issue claims: `resolveGithubAccess` matches the owner against the LIVE App installation list, and the cross-tenant IDOR guard keys on the binding row's existence, not on `account_login`. Repo resolution and private-repo isolation were never at risk. Already-blanked rows are NOT self-healing on ordinary sign-in — the issue's guess was wrong. `bindMemberOrgInstallations` is called from exactly one place, routes/auth.ts:153, inside the `state.linkUid` branch, which is the "Connect GitHub" LINK flow and not the sign-in path. Rather than add a backfill migration, `resolveGithubAccess` now hands the refresh the account it already holds from the live App list — free, since that list was fetched anyway — so a blanked row is corrected by the next authenticated read. Minting still requires the verified binding row; nothing about authority changed. Pinned with three tests over `realSchemaD1` (the migrations applied to real in-memory SQLite, foreign keys ON): the refresh preserves the login, a binding writer still corrects it, and `resolveGithubAccess` repairs a row already blanked. The first fails against the old unconditional SET. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 2f86fab commit b771120

2 files changed

Lines changed: 120 additions & 9 deletions

File tree

‎workers/api/src/lib/github-app.test.ts‎

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { afterEach, describe, expect, it, vi } from "vitest";
2-
import { appJwt, githubAccessDenial, githubAppConfigured, resolveGithubAccess } from "./github-app.js";
2+
import { realSchemaD1 } from "./d1-sqlite.js";
3+
import { appJwt, cacheInstallationToken, githubAccessDenial, githubAppConfigured, listUserInstallations, resolveGithubAccess } from "./github-app.js";
34
import type { Env } from "../types.js";
45

56
/** Export a generated RSA private key as PKCS#8 PEM (what GitHub gives you). */
@@ -247,3 +248,79 @@ describe("resolveGithubAccess — which condition it decides it is", () => {
247248
if (r.ok) expect(r.token).toBe("ghs_x");
248249
});
249250
});
251+
252+
describe("cacheInstallationToken — the account columns belong to the binding writers (#651)", () => {
253+
const hourFromNow = () => new Date(Date.now() + 3600e3).toISOString();
254+
const readRow = (d1: ReturnType<typeof realSchemaD1>) =>
255+
d1.sqlite
256+
.prepare("SELECT account_login, account_type, token_expires_at FROM github_installations WHERE user_id = 'u1' AND installation_id = 7")
257+
.get() as { account_login: string; account_type: string; token_expires_at: string };
258+
259+
const boundUser = () => {
260+
const d1 = realSchemaD1();
261+
d1.exec("INSERT INTO users (id, github_login) VALUES ('u1', 'octo')");
262+
return d1;
263+
};
264+
265+
it("a refresh that knows no account leaves the one the binding wrote", async () => {
266+
// THE reported failure. An installation token lives one hour, so `installationTokenResult`
267+
// re-caches at least hourly with no account; the old unconditional SET wrote `''` over the
268+
// verified login every time, and nothing ever put it back.
269+
const d1 = boundUser();
270+
const env = { DB: d1.DB } as unknown as Env;
271+
try {
272+
await cacheInstallationToken(env, "u1", 7, "ghs_first", hourFromNow(), { login: "acme", type: "Organization" });
273+
await cacheInstallationToken(env, "u1", 7, "ghs_refreshed", hourFromNow());
274+
275+
const row = readRow(d1);
276+
expect(row.account_login).toBe("acme");
277+
expect(row.account_type).toBe("Organization");
278+
// The refresh still did its actual job.
279+
expect(await listUserInstallations(env, "u1")).toEqual([{ id: 7, account: "acme", type: "Organization" }]);
280+
} finally {
281+
d1.close();
282+
}
283+
});
284+
285+
it("a binding writer still corrects the account — that is the repair path", async () => {
286+
const d1 = boundUser();
287+
const env = { DB: d1.DB } as unknown as Env;
288+
try {
289+
await cacheInstallationToken(env, "u1", 7, "ghs_a", hourFromNow(), { login: "old-name", type: "User" });
290+
await cacheInstallationToken(env, "u1", 7, "ghs_b", hourFromNow(), { login: "renamed", type: "Organization" });
291+
const row = readRow(d1);
292+
expect(row.account_login).toBe("renamed");
293+
expect(row.account_type).toBe("Organization");
294+
} finally {
295+
d1.close();
296+
}
297+
});
298+
299+
it("resolveGithubAccess repairs a row already blanked, from the live App list — no backfill needed", async () => {
300+
const d1 = boundUser();
301+
d1.exec(
302+
"INSERT INTO github_installations (id, user_id, installation_id, account_login, account_type) VALUES ('ghinst_u1_7', 'u1', 7, '', '')",
303+
);
304+
const { pem } = await makePem();
305+
const env = { GITHUB_APP_ID: "1", GITHUB_APP_PRIVATE_KEY: pem, DB: d1.DB } as unknown as Env;
306+
vi.stubGlobal(
307+
"fetch",
308+
vi.fn(async (url: string) => {
309+
if (String(url).endsWith("/app/installations")) {
310+
return { ok: true, status: 200, json: async () => [{ id: 7, account: { login: "acme", type: "Organization" } }] } as Response;
311+
}
312+
return { ok: true, status: 200, json: async () => ({ token: "ghs_x", expires_at: hourFromNow() }) } as Response;
313+
}),
314+
);
315+
try {
316+
const r = await resolveGithubAccess(env, "u1", "acme");
317+
expect(r.ok).toBe(true);
318+
// This is the value `githubAuthContext` matches against the repo owner; blank fails
319+
// closed and silently disables the ETag cache for every authenticated read.
320+
expect(readRow(d1).account_login).toBe("acme");
321+
} finally {
322+
vi.unstubAllGlobals();
323+
d1.close();
324+
}
325+
});
326+
});

‎workers/api/src/lib/github-app.ts‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,12 @@ async function installationTokenResult(
151151
env: Env,
152152
userId: string,
153153
installationId: number,
154+
/**
155+
* The account this installation belongs to, when the CALLER already knows it from the live App
156+
* list. Never fetched for this: omitted means "this path has no opinion about the account
157+
* columns", which is exactly what a token refresh should say (#651).
158+
*/
159+
account?: { login: string; type?: string },
154160
): Promise<{ ok: true; token: string } | { ok: false; reason: "no-binding" | "mint-failed" }> {
155161
const row = await env.DB.prepare(
156162
"SELECT id, token_ciphertext, token_dek, token_iv, token_expires_at FROM github_installations WHERE user_id = ?1 AND installation_id = ?2",
@@ -176,11 +182,33 @@ async function installationTokenResult(
176182

177183
const minted = await mintInstallationToken(env, installationId);
178184
if (!minted) return { ok: false, reason: "mint-failed" };
179-
await cacheInstallationToken(env, userId, installationId, minted.token, minted.expiresAt);
185+
await cacheInstallationToken(env, userId, installationId, minted.token, minted.expiresAt, account);
180186
return { ok: true, token: minted.token };
181187
}
182188

183-
/** Persist (encrypt) the installation token + metadata for reuse. */
189+
/**
190+
* Persist (encrypt) the installation token — and the account columns ONLY when the caller supplied
191+
* them (#651).
192+
*
193+
* The account columns belong to the two BINDING writers, `authorizeInstallation` and
194+
* `bindMemberOrgInstallations`, which have just proved who owns the installation. The token
195+
* refresh in `installationTokenResult` knows the token and nothing else, and it runs at least
196+
* hourly (an installation token lives one hour) — and on EVERY call when `KEY_ENCRYPTION_KEY` is
197+
* unset, because the freshness branch also requires the stored ciphertext.
198+
*
199+
* That refresh used to bind `account?.login ?? ""` into an `ON CONFLICT … DO UPDATE SET
200+
* account_login = excluded.account_login`, so every row blanked its own account within the hour.
201+
* `githubAuthContext` (github-cache.ts) names a read's authority by matching that column against
202+
* the repo owner; `''` matches nothing, it fail-closes to `null`, and both the read and write
203+
* halves of the GitHub ETag cache short-circuit on a falsy context. The result was that every
204+
* authenticated GitHub read stopped being conditionally cached, permanently, from the first
205+
* refresh onward — deploy-watch's per-repo cron poll included, which is direct exposure to
206+
* GitHub's primary rate limit.
207+
*
208+
* So the UPDATE touches those columns only when there is a verified account to write. The INSERT
209+
* still needs a value for the NOT NULL columns; it is unreachable from the refresh path, which
210+
* returns `no-binding` before minting when the row is absent.
211+
*/
184212
export async function cacheInstallationToken(
185213
env: Env,
186214
userId: string,
@@ -192,13 +220,14 @@ export async function cacheInstallationToken(
192220
let cipher: { ciphertext: Uint8Array; dekWrapped: Uint8Array; iv: Uint8Array } | null = null;
193221
if (env.KEY_ENCRYPTION_KEY) cipher = await encryptKey(token, env.KEY_ENCRYPTION_KEY);
194222
const id = `ghinst_${userId}_${installationId}`;
223+
const accountSet = account
224+
? "account_login = excluded.account_login,\n\t\t account_type = excluded.account_type,\n\t\t "
225+
: "";
195226
await env.DB.prepare(
196227
`INSERT INTO github_installations (id, user_id, installation_id, account_login, account_type, token_ciphertext, token_dek, token_iv, token_expires_at, updated_at)
197228
VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, datetime('now'))
198229
ON CONFLICT(user_id, installation_id) DO UPDATE SET
199-
account_login = excluded.account_login,
200-
account_type = excluded.account_type,
201-
token_ciphertext = excluded.token_ciphertext,
230+
${accountSet}token_ciphertext = excluded.token_ciphertext,
202231
token_dek = excluded.token_dek,
203232
token_iv = excluded.token_iv,
204233
token_expires_at = excluded.token_expires_at,
@@ -383,9 +412,14 @@ export async function resolveGithubAccess(
383412
return deny("not-installed", { installUrl: await installUrlFor(env) });
384413
}
385414

386-
const token = await installationTokenResult(env, userId, match.id).catch(
387-
() => ({ ok: false, reason: "mint-failed" }) as const,
388-
);
415+
// The account is already in hand from the live App list, so handing it to the refresh costs
416+
// nothing and REPAIRS a row blanked by the pre-#651 upsert on the next authenticated read —
417+
// which is why no backfill migration is needed. This is not a new authority claim: minting
418+
// still requires the verified binding row, and `match` comes from GitHub, not from our column.
419+
const token = await installationTokenResult(env, userId, match.id, {
420+
login: match.account.login,
421+
type: match.account.type,
422+
}).catch(() => ({ ok: false, reason: "mint-failed" }) as const);
389423
if (token.ok) return { ok: true, token: token.token };
390424
return token.reason === "no-binding" ? deny("not-authorized") : deny("transient", { detail: "the installation token could not be minted" });
391425
}

0 commit comments

Comments
 (0)