mirror of
https://github.com/tdurieux/anonymous_github.git
synced 2026-10-19 22:27:20 +02:00
fix: cover streamed access errors without alerting on outages
This commit is contained in:
1 parent
85b9a50d4d
commit
2b20abe0a5
5 files changed
+128
-45
No files matched your search
@@ -1,3 +1,4 @@
|
|||||||
|
import { notifyOwnerAccessProblem } from "./owner-notifications";
|
||||||
import { streamResponse } from "./response-stream";
|
import { streamResponse } from "./response-stream";
|
||||||
import { join, basename, dirname } from "path";
|
import { join, basename, dirname } from "path";
|
||||||
import { Response } from "express";
|
import { Response } from "express";
|
||||||
@@ -355,7 +356,11 @@ export default class AnonymizedFile {
|
|||||||
httpStatus: 413,
|
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;
|
const cacheWasReset = this.repository.model.isReseted;
|
||||||
if (cacheWasReset) {
|
if (cacheWasReset) {
|
||||||
await this.repository.markCachePresent();
|
await this.repository.markCachePresent();
|
||||||
@@ -378,7 +383,7 @@ export default class AnonymizedFile {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// use the streamer service
|
// use the streamer service
|
||||||
return got.stream(join(config.STREAMER_ENTRYPOINT, "api"), {
|
const content = got.stream(join(config.STREAMER_ENTRYPOINT, "api"), {
|
||||||
method: "POST",
|
method: "POST",
|
||||||
headers: requestHeaders(),
|
headers: requestHeaders(),
|
||||||
json: {
|
json: {
|
||||||
@@ -395,6 +400,11 @@ export default class AnonymizedFile {
|
|||||||
anonymizerOptions: anonymizer.opt,
|
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() {
|
get filePath() {
|
||||||
@@ -472,6 +482,7 @@ export default class AnonymizedFile {
|
|||||||
filePath: this.anonymizedPath,
|
filePath: this.anonymizedPath,
|
||||||
}
|
}
|
||||||
);
|
);
|
||||||
|
void notifyOwnerAccessProblem(this.repository.owner.id, error);
|
||||||
error.value = this;
|
error.value = this;
|
||||||
handleError(error, res);
|
handleError(error, res);
|
||||||
if (res.headersSent && !res.writableEnded) res.destroy();
|
if (res.headersSent && !res.writableEnded) res.destroy();
|
||||||
|
|||||||
+6
-38
@@ -1,3 +1,4 @@
|
|||||||
|
import { isGitHubRateLimitError, OctokitRequestErrorLike } from "./github-rate-limit";
|
||||||
import { refreshLegacyToken } from "./legacy-token-refresh";
|
import { refreshLegacyToken } from "./legacy-token-refresh";
|
||||||
import { notifyOwnerAccessProblem } from "./owner-notifications";
|
import { notifyOwnerAccessProblem } from "./owner-notifications";
|
||||||
import AnonymizedRepositoryModel from "./model/anonymizedRepositories/anonymizedRepositories.model";
|
import AnonymizedRepositoryModel from "./model/anonymizedRepositories/anonymizedRepositories.model";
|
||||||
@@ -17,35 +18,7 @@ import { measureStage } from "./request-monitoring";
|
|||||||
|
|
||||||
const logger = createLogger("github");
|
const logger = createLogger("github");
|
||||||
|
|
||||||
// Octokit RequestError shape (subset we care about for rate-limit detection).
|
export { isGitHubRateLimitError } from "./github-rate-limit";
|
||||||
interface OctokitRequestErrorLike {
|
|
||||||
status?: number;
|
|
||||||
message?: string;
|
|
||||||
response?: {
|
|
||||||
headers?: Record<string, string | undefined>;
|
|
||||||
};
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* 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";
|
|
||||||
}
|
|
||||||
|
|
||||||
function rateLimitDetail(err: OctokitRequestErrorLike): string {
|
function rateLimitDetail(err: OctokitRequestErrorLike): string {
|
||||||
const headers = err.response?.headers ?? {};
|
const headers = err.response?.headers ?? {};
|
||||||
@@ -299,15 +272,10 @@ export async function checkToken(token: string, ownerId?: string) {
|
|||||||
else await oct.users.getAuthenticated();
|
else await oct.users.getAuthenticated();
|
||||||
return true;
|
return true;
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
if (
|
// Only a confirmed invalid credential permits fallback. Network failures,
|
||||||
err instanceof AnonymousError &&
|
// permission failures and rate limits must retain their original meaning.
|
||||||
err.message === "github_rate_limit_exceeded"
|
if ((err as { status?: number })?.status !== 401) throw err;
|
||||||
) {
|
if (ownerId) void notifyOwnerAccessProblem(ownerId, "token_expired");
|
||||||
throw err;
|
|
||||||
}
|
|
||||||
if (ownerId && (err as { status?: number }).status === 401) {
|
|
||||||
void notifyOwnerAccessProblem(ownerId, "token_expired");
|
|
||||||
}
|
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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<string, string | undefined>;
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
/** 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";
|
||||||
|
}
|
||||||
|
|
||||||
@@ -1,3 +1,4 @@
|
|||||||
|
import { isGitHubRateLimitError } from "./github-rate-limit";
|
||||||
import config from "../config";
|
import config from "../config";
|
||||||
import UserModel from "./model/users/users.model";
|
import UserModel from "./model/users/users.model";
|
||||||
import { createLogger } from "./logger";
|
import { createLogger } from "./logger";
|
||||||
@@ -7,7 +8,7 @@ const DAY = 24 * 60 * 60 * 1000;
|
|||||||
const accessErrors = new Set([
|
const accessErrors = new Set([
|
||||||
"token_expired", "github_oauth_required", "github_app_reconnect_required",
|
"token_expired", "github_oauth_required", "github_app_reconnect_required",
|
||||||
"github_app_access_required", "repo_not_found", "repository_not_found",
|
"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 {
|
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 {
|
export function isAccessFailure(error: unknown): boolean {
|
||||||
|
if (isGitHubRateLimitError(error)) return false;
|
||||||
const code = typeof error === "string" ? error : error instanceof Error ? error.message : "";
|
const code = typeof error === "string" ? error : error instanceof Error ? error.message : "";
|
||||||
const rawStatus = error instanceof Error ? (error as Error & { status?: number }).status : undefined;
|
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) {
|
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.
|
// Some legacy paths wrap timeouts or 5xx responses as repo_not_found.
|
||||||
// Only a confirmed access failure should prompt the owner to reconnect.
|
// 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;
|
const status = (error as Error & { httpStatus?: number }).httpStatus;
|
||||||
if (status && (status === 429 || status >= 500)) return false;
|
if (status && (status === 429 || status >= 500)) return false;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -94,10 +94,25 @@ describe("owner access notifications", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it("recognizes raw authentication and missing-resource errors without alerting for outages", () => {
|
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);
|
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 () => {
|
it("does not let database failures escape", async () => {
|
||||||
stub(User, "findOneAndUpdate", () => { throw new Error("database down"); });
|
stub(User, "findOneAndUpdate", () => { throw new Error("database down"); });
|
||||||
await notifyOwnerAccessProblem("owner-id", "token_expired");
|
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);
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in new issue
Block a user