diff --git a/src/core/AnonymizedFile.ts b/src/core/AnonymizedFile.ts index 09c684d..da1d92f 100644 --- a/src/core/AnonymizedFile.ts +++ b/src/core/AnonymizedFile.ts @@ -1,3 +1,4 @@ +import { notifyOwnerAccessProblem } from "./owner-notifications"; import { streamResponse } from "./response-stream"; import { join, basename, dirname } from "path"; import { Response } from "express"; @@ -355,7 +356,11 @@ export default class AnonymizedFile { httpStatus: 413, }); } - const content = await this.repository.source?.getFileContent(this); + const content = await this.repository.source?.getFileContent(this).catch(error => { + void notifyOwnerAccessProblem(this.repository.owner.id, error); + throw error; + }); + content.on("error", error => { void notifyOwnerAccessProblem(this.repository.owner.id, error); }); const cacheWasReset = this.repository.model.isReseted; if (cacheWasReset) { await this.repository.markCachePresent(); @@ -378,7 +383,7 @@ export default class AnonymizedFile { } // use the streamer service - return got.stream(join(config.STREAMER_ENTRYPOINT, "api"), { + const content = got.stream(join(config.STREAMER_ENTRYPOINT, "api"), { method: "POST", headers: requestHeaders(), json: { @@ -395,6 +400,11 @@ export default class AnonymizedFile { anonymizerOptions: anonymizer.opt, }, }); + content.on("error", err => { + const { error } = streamerErrorToAnonymous(err, { repoId: this.repository.repoId, filePath: this.anonymizedPath }); + void notifyOwnerAccessProblem(this.repository.owner.id, error); + }); + return content; } get filePath() { @@ -472,6 +482,7 @@ export default class AnonymizedFile { filePath: this.anonymizedPath, } ); + void notifyOwnerAccessProblem(this.repository.owner.id, error); error.value = this; handleError(error, res); if (res.headersSent && !res.writableEnded) res.destroy(); diff --git a/src/core/GitHubUtils.ts b/src/core/GitHubUtils.ts index 0a8c79f..040b3cd 100644 --- a/src/core/GitHubUtils.ts +++ b/src/core/GitHubUtils.ts @@ -1,3 +1,4 @@ +import { isGitHubRateLimitError, OctokitRequestErrorLike } from "./github-rate-limit"; import { refreshLegacyToken } from "./legacy-token-refresh"; import { notifyOwnerAccessProblem } from "./owner-notifications"; import AnonymizedRepositoryModel from "./model/anonymizedRepositories/anonymizedRepositories.model"; @@ -17,35 +18,7 @@ import { measureStage } from "./request-monitoring"; const logger = createLogger("github"); -// Octokit RequestError shape (subset we care about for rate-limit detection). -interface OctokitRequestErrorLike { - status?: number; - message?: string; - response?: { - headers?: Record; - }; -} - -/** - * Detect GitHub rate-limit / abuse responses (primary 5k/h or undocumented - * "secondary" limits) and rewrap them as a translatable AnonymousError so - * the UI can show a friendly message instead of a raw HttpError stack. The - * GitHub `x-github-request-id` header is preserved in `detail` so users can - * cite it if they reach out to GitHub Support. - */ -export function isGitHubRateLimitError(err: unknown): boolean { - if (!err || typeof err !== "object") return false; - const e = err as OctokitRequestErrorLike; - const msg = (e.message ?? "").toLowerCase(); - // Primary limits return 403 with "x-ratelimit-remaining: 0"; secondary - // limits return 403 (sometimes 429) with "secondary rate limit" in the - // body. Match on either signal so we catch both. - const status = e.status ?? 0; - if (status !== 403 && status !== 429) return false; - if (msg.includes("rate limit") || msg.includes("abuse")) return true; - const remaining = e.response?.headers?.["x-ratelimit-remaining"]; - return remaining === "0"; -} +export { isGitHubRateLimitError } from "./github-rate-limit"; function rateLimitDetail(err: OctokitRequestErrorLike): string { const headers = err.response?.headers ?? {}; @@ -299,15 +272,10 @@ export async function checkToken(token: string, ownerId?: string) { else await oct.users.getAuthenticated(); return true; } catch (err) { - if ( - err instanceof AnonymousError && - err.message === "github_rate_limit_exceeded" - ) { - throw err; - } - if (ownerId && (err as { status?: number }).status === 401) { - void notifyOwnerAccessProblem(ownerId, "token_expired"); - } + // Only a confirmed invalid credential permits fallback. Network failures, + // permission failures and rate limits must retain their original meaning. + if ((err as { status?: number })?.status !== 401) throw err; + if (ownerId) void notifyOwnerAccessProblem(ownerId, "token_expired"); return false; } } diff --git a/src/core/github-rate-limit.ts b/src/core/github-rate-limit.ts new file mode 100644 index 0000000..e076a38 --- /dev/null +++ b/src/core/github-rate-limit.ts @@ -0,0 +1,26 @@ +// Octokit RequestError shape (subset we care about for rate-limit detection). +export interface OctokitRequestErrorLike { + status?: number; + httpStatus?: number; + message?: string; + response?: { + statusCode?: number; + headers?: Record; + }; +} + +/** Detect GitHub primary and secondary rate limits in API and stream errors. */ +export function isGitHubRateLimitError(err: unknown): boolean { + if (!err || typeof err !== "object") return false; + const e = err as OctokitRequestErrorLike; + const msg = (e.message ?? "").toLowerCase(); + // Primary limits return 403 with "x-ratelimit-remaining: 0"; secondary + // limits return 403 (sometimes 429) with "secondary rate limit" in the + // body. Match on either signal so we catch both. + const status = e.status ?? e.response?.statusCode ?? e.httpStatus ?? 0; + if (status !== 403 && status !== 429) return false; + if (msg.includes("rate limit") || msg.includes("abuse")) return true; + const remaining = e.response?.headers?.["x-ratelimit-remaining"]; + return remaining === "0"; +} + diff --git a/src/core/owner-notifications.ts b/src/core/owner-notifications.ts index ca91791..5f1b315 100644 --- a/src/core/owner-notifications.ts +++ b/src/core/owner-notifications.ts @@ -1,3 +1,4 @@ +import { isGitHubRateLimitError } from "./github-rate-limit"; import config from "../config"; import UserModel from "./model/users/users.model"; import { createLogger } from "./logger"; @@ -7,7 +8,7 @@ const DAY = 24 * 60 * 60 * 1000; const accessErrors = new Set([ "token_expired", "github_oauth_required", "github_app_reconnect_required", "github_app_access_required", "repo_not_found", "repository_not_found", - "pull_request_not_found", "gist_not_found", "repo_not_accessible", "repo_access_limited", "repo_saml_enforcement", + "file_not_accessible", "file_not_found", "pull_request_not_found", "gist_not_found", "repo_not_accessible", "repo_access_limited", "repo_saml_enforcement", ]); export function normalizeEmail(value: unknown): string | null { @@ -26,14 +27,16 @@ export function notificationEmail(emails?: { email: string; default: boolean }[] } export function isAccessFailure(error: unknown): boolean { + if (isGitHubRateLimitError(error)) return false; const code = typeof error === "string" ? error : error instanceof Error ? error.message : ""; const rawStatus = error instanceof Error ? (error as Error & { status?: number }).status : undefined; - if (!accessErrors.has(code) && ![401, 404].includes(rawStatus || 0)) return false; + if (!accessErrors.has(code) && ![401, 403, 404].includes(rawStatus || 0)) return false; if (error instanceof Error) { - const upstream = (error as Error & { cause?: { status?: number; httpStatus?: number } }).cause; + const upstream = (error as Error & { cause?: { status?: number; httpStatus?: number; response?: { statusCode?: number } } }).cause; // Some legacy paths wrap timeouts or 5xx responses as repo_not_found. // Only a confirmed access failure should prompt the owner to reconnect. - if (upstream && ![401, 403, 404].includes(upstream.status || upstream.httpStatus || 0)) return false; + if (isGitHubRateLimitError(upstream)) return false; + if (upstream && ![401, 403, 404].includes(upstream.status || upstream.response?.statusCode || upstream.httpStatus || 0)) return false; const status = (error as Error & { httpStatus?: number }).httpStatus; if (status && (status === 429 || status >= 500)) return false; } diff --git a/test/owner-notifications.test.js b/test/owner-notifications.test.js index c180fba..ca18a7b 100644 --- a/test/owner-notifications.test.js +++ b/test/owner-notifications.test.js @@ -94,10 +94,25 @@ describe("owner access notifications", () => { }); it("recognizes raw authentication and missing-resource errors without alerting for outages", () => { - for (const status of [401, 404]) expect(notifications.isAccessFailure(Object.assign(new Error("GitHub rejected access"), { status }))).to.equal(true); + for (const status of [401, 403, 404]) expect(notifications.isAccessFailure(Object.assign(new Error("GitHub rejected access"), { status }))).to.equal(true); for (const status of [429, 500, 503]) expect(notifications.isAccessFailure(Object.assign(new Error("upstream error"), { status }))).to.equal(false); }); + it("suppresses primary and secondary rate limits including wrapped streaming errors", () => { + const AnonymousError = require("../src/core/AnonymousError").default; + for (const error of [ + Object.assign(new Error("API rate limit exceeded"), { status: 403 }), + Object.assign(new Error("abuse detection"), { status: 403 }), + Object.assign(new Error("Forbidden"), { status: 403, response: { headers: { "x-ratelimit-remaining": "0" } } }), + Object.assign(new Error("secondary rate limit"), { response: { statusCode: 403 } }), + ]) { + expect(notifications.isAccessFailure(error)).to.equal(false); + expect(notifications.isAccessFailure(new AnonymousError("file_not_accessible", { httpStatus: 403, cause: error }))).to.equal(false); + } + const forbidden = Object.assign(new Error("SAML enforcement"), { response: { statusCode: 403 } }); + expect(notifications.isAccessFailure(new AnonymousError("file_not_accessible", { httpStatus: 403, cause: forbidden }))).to.equal(true); + }); + it("does not let database failures escape", async () => { stub(User, "findOneAndUpdate", () => { throw new Error("database down"); }); await notifyOwnerAccessProblem("owner-id", "token_expired"); @@ -275,3 +290,63 @@ describe("gist and pull-request notification hooks", () => { }); } }); + +describe("streamed file access alerts", () => { + const File = require("../src/core/AnonymizedFile").default; + const AnonymousError = require("../src/core/AnonymousError").default; + const { PassThrough } = require("stream"); + const got = require("got"); + const restores = []; + const stub = (object, key, value) => { const old = object[key]; restores.push(() => object[key] = old); object[key] = value; }; + let sent, repository, file; + beforeEach(() => { + sent = []; + stub(notifications, "notifyOwnerAccessProblem", async (owner, error) => { if (notifications.isAccessFailure(error)) sent.push({ owner, error }); }); + repository = { owner: { id: "saved-owner" }, options: { terms: [] }, model: { source: {} }, repoId: "anon-id", status: "ready", source: {}, + getToken: async () => "token", generateAnonymizeTransformer: () => new PassThrough() }; + file = new File({ repository, anonymizedPath: "README.md" }); + }); + afterEach(() => { while (restores.length) restores.pop()(); }); + for (const status of [403, 404, 503]) it(`handles local file download failure ${status}`, async () => { + const cause = Object.assign(new Error("upstream"), { response: { statusCode: status } }); + const error = new AnonymousError(status === 403 ? "file_not_accessible" : status === 404 ? "file_not_found" : "upstream_error", { httpStatus: status, cause }); + repository.source.getFileContent = async () => { throw error; }; + let caught; + try { await file.content(); } catch (err) { caught = err; } + expect(caught).to.equal(error); + expect(sent).to.have.length(status === 503 ? 0 : 1); + if (sent.length) expect(sent[0].owner).to.equal("saved-owner"); + }); + for (const status of [403, 404, 503]) it(`handles remote streamer error ${status}`, async () => { + stub(config, "STREAMER_ENTRYPOINT", "http://streamer/"); + const stream = new PassThrough(); + stub(got, "stream", () => stream); + file.originalPath = async () => "README.md"; + file.sha = async () => "sha"; + file.size = async () => 10; + const returned = await file.anonymizedContent(); + expect(returned).to.equal(stream); + const error = Object.assign(new Error("streamer failure"), { response: { statusCode: status, body: JSON.stringify({ error: status === 503 ? "upstream_error" : "file_not_accessible" }) } }); + stream.emit("error", error); + expect(sent).to.have.length(status === 503 ? 0 : 1); + stream.destroy(); + }); +}); + +describe("token validation preserves upstream failures", () => { + const tokens = require("../src/core/GitHubUtils"); + const { registerGitHubToken } = require("../src/core/github-token-context"); + for (const status of [undefined, 403, 429, 500, 503]) it(`propagates token-check failure ${status}`, async () => { + const error = Object.assign(new Error("upstream check failed"), { status }); + const token = `test-token-check-${status}`; + registerGitHubToken(token, { quotaKey: token, publicRepository: "owner/repo", renew: async () => { throw error; } }); + let caught; + try { await tokens.checkToken(token); } catch (err) { caught = err; } + expect(caught).to.equal(error); + }); + it("returns false only for a confirmed invalid token", async () => { + const token = "test-token-check-invalid"; + registerGitHubToken(token, { quotaKey: token, publicRepository: "owner/repo", renew: async () => { throw Object.assign(new Error("Bad credentials"), { status: 401 }); } }); + expect(await tokens.checkToken(token)).to.equal(false); + }); +});