From 5267599254008a6fb146c7520470220dce5149f5 Mon Sep 17 00:00:00 2001 From: Codex Date: Tue, 10 Nov 2026 11:36:55 +0000 Subject: [PATCH] perf: reduce GitHub quota spent validating credentials --- src/core/GitHubUtils.ts | 18 ++++- src/core/credential-validation.ts | 44 ++++++++++++ test/credential-validation.test.js | 111 +++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 src/core/credential-validation.ts create mode 100644 test/credential-validation.test.js diff --git a/src/core/GitHubUtils.ts b/src/core/GitHubUtils.ts index f644bd4..c2a699f 100644 --- a/src/core/GitHubUtils.ts +++ b/src/core/GitHubUtils.ts @@ -1,4 +1,5 @@ import { isGitHubRateLimitError, OctokitRequestErrorLike } from "./github-rate-limit"; +import { CredentialValidation } from "./credential-validation"; import { refreshLegacyToken } from "./legacy-token-refresh"; import { notifyOwnerAccessProblem } from "./owner-notifications"; import AnonymizedRepositoryModel from "./model/anonymizedRepositories/anonymizedRepositories.model"; @@ -277,13 +278,26 @@ export function octokit(token: string) { export { waitForTokenGate }; +const credentialValidation = new CredentialValidation(); + export async function checkToken(token: string) { const oct = octokit(token); try { const context = githubTokenContext(token); if (context?.publicRepository) { await context.renew(); return true; } - if (token.startsWith("ghs_")) await oct.request("GET /installation/repositories"); - else await oct.users.getAuthenticated(); + // Managed credentials can renew or revalidate repository access. Never + // share their checks or cache validators under a potentially rotating handle. + if (context) { + if (token.startsWith("ghs_")) await oct.request("GET /installation/repositories"); + else await oct.users.getAuthenticated(); + } else { + await credentialValidation.check(token, async etag => { + const headers = etag ? { "if-none-match": etag } : {}; + return token.startsWith("ghs_") + ? oct.request("GET /installation/repositories", { headers }) + : oct.users.getAuthenticated({ headers }); + }); + } return true; } catch (err) { // Only a confirmed invalid credential permits fallback. Network failures, diff --git a/src/core/credential-validation.ts b/src/core/credential-validation.ts new file mode 100644 index 0000000..8f57717 --- /dev/null +++ b/src/core/credential-validation.ts @@ -0,0 +1,44 @@ +import { createHash } from "crypto"; + +type ValidationRequest = (etag?: string) => Promise<{ headers: { etag?: string } }>; + +/** Store validators, never cached authorization decisions. Every completed + * check requires a fresh authenticated response from GitHub. */ +export class CredentialValidation { + private readonly validators = new Map(); + private readonly pending = new Map>(); + + constructor(private readonly capacity = 2000) {} + + check(token: string, request: ValidationRequest): Promise { + // Quota keys can be shared by multiple credentials. Validation cannot. + const key = createHash("sha256").update(token).digest("hex"); + const pending = this.pending.get(key); + if (pending) return pending; + const check = this.validate(key, request); + if (this.pending.size >= this.capacity) return check; + const tracked = check.finally(() => { this.pending.delete(key); }); + this.pending.set(key, tracked); + return tracked; + } + + private async validate(key: string, request: ValidationRequest): Promise { + const etag = this.validators.get(key); + try { + const response = await request(etag); + this.validators.delete(key); + if (response.headers.etag && this.capacity > 0) { + if (this.validators.size >= this.capacity) { + this.validators.delete(this.validators.keys().next().value!); + } + this.validators.set(key, response.headers.etag); + } + } catch (error) { + // Octokit throws for 304. Accept it only when this exact credential + // previously succeeded and this request sent its validator. + if (etag && (error as { status?: number })?.status === 304) return; + this.validators.delete(key); + throw error; + } + } +} diff --git a/test/credential-validation.test.js b/test/credential-validation.test.js new file mode 100644 index 0000000..7ed5177 --- /dev/null +++ b/test/credential-validation.test.js @@ -0,0 +1,111 @@ +const { expect } = require('chai'); +require('ts-node/register/transpile-only'); +const { CredentialValidation } = require('../src/core/credential-validation'); +const { checkToken } = require('../src/core/GitHubUtils'); +const { registerGitHubToken } = require('../src/core/github-token-context'); +const failure = status => Object.assign(new Error('upstream'), { status }); +const success = etag => ({ headers: etag ? { etag } : {} }); + +describe('conditional credential validation', () => { + it('shares only overlapping checks, and revalidates subsequent calls', async () => { + const cache = new CredentialValidation(); + let release, calls = 0; + const request = () => { calls++; return new Promise(resolve => { release = resolve; }); }; + const checks = Array.from({ length: 20 }, () => cache.check('one', request)); + expect(calls).to.equal(1); + release(success('"one"')); + await Promise.all(checks); + await cache.check('one', async etag => { expect(etag).to.equal('"one"'); calls++; throw failure(304); }); + expect(calls).to.equal(2); + }); + + it('does not reuse validation across credentials or retain failures', async () => { + const cache = new CredentialValidation(); + await cache.check('old', async () => success('"old"')); + await cache.check('rotated', async etag => { expect(etag).to.equal(undefined); return success('"new"'); }); + for (const status of [401, 403, 429, 500, 503, undefined]) { + const error = failure(status); + try { await cache.check('old', async () => { throw error; }); throw new Error('accepted'); } + catch (caught) { expect(caught).to.equal(error); } + await cache.check('old', async etag => { expect(etag).to.equal(undefined); return success('"old"'); }); + } + }); + + it('rejects unsolicited 304s and removes validators when absent in a new success', async () => { + const cache = new CredentialValidation(); + const error = failure(304); + try { await cache.check('new', async () => { throw error; }); throw new Error('accepted'); } + catch (caught) { expect(caught).to.equal(error); } + await cache.check('new', async () => success('"etag"')); + await cache.check('new', async () => success()); + await cache.check('new', async etag => { expect(etag).to.equal(undefined); return success(); }); + }); + + it('bounds validators and tracked in-flight requests', async () => { + const cache = new CredentialValidation(1); + let release; + const first = cache.check('first', () => new Promise(resolve => { release = resolve; })); + await cache.check('second', async () => success('"second"')); + expect(cache.pending.size).to.equal(1); + release(success('"first"')); + await first; + expect(cache.validators.size).to.equal(1); + await cache.check('second', async etag => { expect(etag).to.equal(undefined); return success(); }); + expect(cache.pending.size).to.equal(0); + }); + + it('uses authenticated conditional requests through Octokit and detects revocation', async () => { + const originalFetch = global.fetch; + const requests = []; + let status = 200; + global.fetch = async (url, options) => { + requests.push({ url, headers: options.headers }); + return new global.Response(status === 304 ? null : JSON.stringify(status === 200 ? { id: 1 } : { message: 'Bad credentials' }), { + status, headers: { 'content-type': 'application/json', etag: '"user-v1"' } + }); + }; + try { + const token = 'test-conditional-credential'; + expect(await checkToken(token)).to.equal(true); + status = 304; + expect(await checkToken(token)).to.equal(true); + expect(requests[1].headers['if-none-match']).to.equal('"user-v1"'); + expect(requests[1].headers.authorization).to.equal(`token ${token}`); + status = 401; + expect(await checkToken(token)).to.equal(false); + status = 200; + expect(await checkToken(token)).to.equal(true); + expect(requests[3].headers['if-none-match']).to.equal(undefined); + expect(requests.every(request => request.url === 'https://api.github.com/user')).to.equal(true); + } finally { global.fetch = originalFetch; } + }); + + it('validates installation credentials against their own endpoint', async () => { + const originalFetch = global.fetch; + let calls = 0; + global.fetch = async (url, options) => { + expect(url).to.equal('https://api.github.com/installation/repositories'); + expect(options.headers.authorization).to.equal('token ghs_conditional-test'); + calls++; + if (calls === 2) expect(options.headers['if-none-match']).to.equal('"installation"'); + return new global.Response(calls === 2 ? null : '{"repositories":[]}', { + status: calls === 2 ? 304 : 200, + headers: { 'content-type': 'application/json', etag: '"installation"' } + }); + }; + try { + expect(await checkToken('ghs_conditional-test')).to.equal(true); + expect(await checkToken('ghs_conditional-test')).to.equal(true); + } finally { global.fetch = originalFetch; } + }); + + it('keeps repository-specific managed renewal independent for every call', async () => { + let calls = 0; + const token = 'public-read:conditional-validation-test'; + registerGitHubToken(token, { quotaKey: 'shared', publicRepository: 'owner/repo', renew: async () => { calls++; return 'renewed'; } }); + await Promise.all([checkToken(token), checkToken(token)]); + expect(calls).to.equal(2); + registerGitHubToken(token, { quotaKey: 'shared', publicRepository: 'owner/repo', renew: async () => { throw failure(401); } }); + expect(await checkToken(token)).to.equal(false); + }); +});