From 85b9a50d4ded72daf78e59e446a40668cba1a554 Mon Sep 17 00:00:00 2001 From: Thomas Durieux <5577568+tdurieux@users.noreply.github.com> Date: Fri, 9 Oct 2026 05:42:19 +0000 Subject: [PATCH] fix: address owner notification review findings --- README.md | 3 +- src/core/Gist.ts | 9 +++ src/core/PullRequest.ts | 9 +++ src/core/model/users/users.schema.ts | 1 + src/core/model/users/users.types.ts | 1 + src/core/owner-notifications.ts | 22 +++++--- src/server/routes/github-app.ts | 12 +++- src/server/routes/user.ts | 12 ++-- test/owner-notifications.test.js | 82 +++++++++++++++++++++++++++- 9 files changed, 130 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 9631a61..03c1e75 100644 --- a/README.md +++ b/README.md @@ -112,7 +112,8 @@ Emails contain no repository names, anonymous URLs, or credentials. An atomic database claim limits alerts to one per owner every 24 hours across workers and repositories. Sending is best effort: delivery failures do not block -repository requests, and a later access error can retry after ten minutes. +repository requests. A definite provider rejection allows a later access error +to retry after ten minutes; ambiguous timeouts retain the daily limit. A process interrupted after claiming an alert may skip that day's email. Leaving either Resend setting empty disables delivery and the email prompt. diff --git a/src/core/Gist.ts b/src/core/Gist.ts index a85d5d2..10e1e6b 100644 --- a/src/core/Gist.ts +++ b/src/core/Gist.ts @@ -1,3 +1,4 @@ +import { notifyOwnerAccessProblem } from "./owner-notifications"; import { registerGitHubToken } from "./github-token-context"; import { isConnected } from "../server/database"; import { expireEmbeddedContent } from "./content-expiration"; @@ -67,6 +68,14 @@ export default class Gist { } async download() { + try { await this.downloadWithAccess(); } + catch (error) { + void notifyOwnerAccessProblem(this.owner.id, error); + throw error; + } + } + + private async downloadWithAccess() { const access = await this.getAccess(); try { await this.downloadContent(access.token); diff --git a/src/core/PullRequest.ts b/src/core/PullRequest.ts index 94e8e02..3b63401 100644 --- a/src/core/PullRequest.ts +++ b/src/core/PullRequest.ts @@ -1,3 +1,4 @@ +import { notifyOwnerAccessProblem } from "./owner-notifications"; import { isConnected } from "../server/database"; import { expireEmbeddedContent } from "./content-expiration"; import { boundAppToken } from "./github-app"; @@ -33,6 +34,14 @@ export default class PullRequest { } async download() { + try { await this.downloadContent(); } + catch (error) { + void notifyOwnerAccessProblem(this.owner.id, error); + throw error; + } + } + + private async downloadContent() { logger.info("downloading pull request", { pullRequestId: this._model.source.pullRequestId, }); diff --git a/src/core/model/users/users.schema.ts b/src/core/model/users/users.schema.ts index 2541642..63c77c3 100644 --- a/src/core/model/users/users.schema.ts +++ b/src/core/model/users/users.schema.ts @@ -15,6 +15,7 @@ const UserSchema = new Schema({ type: String, index: { unique: true }, }, + notificationEmail: { type: String }, emailPromptNever: { type: Boolean, default: false }, accessAlertAfter: { type: Date, select: false }, emails: [ diff --git a/src/core/model/users/users.types.ts b/src/core/model/users/users.types.ts index 62b4f0d..fc0ada6 100644 --- a/src/core/model/users/users.types.ts +++ b/src/core/model/users/users.types.ts @@ -20,6 +20,7 @@ export interface IUser { createdAt?: Date; lastUsedAt?: Date; }[]; + notificationEmail?: string; emailPromptNever?: boolean; emails: { email: string; diff --git a/src/core/owner-notifications.ts b/src/core/owner-notifications.ts index cf987eb..ca91791 100644 --- a/src/core/owner-notifications.ts +++ b/src/core/owner-notifications.ts @@ -7,7 +7,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", - "repo_not_accessible", "repo_access_limited", "repo_saml_enforcement", + "pull_request_not_found", "gist_not_found", "repo_not_accessible", "repo_access_limited", "repo_saml_enforcement", ]); export function normalizeEmail(value: unknown): string | null { @@ -20,14 +20,15 @@ export function normalizeEmail(value: unknown): string | null { return email; } -export function notificationEmail(emails?: { email: string; default: boolean }[]): string { - return normalizeEmail(emails?.find(e => e.default)?.email) || +export function notificationEmail(emails?: { email: string; default: boolean }[], preferred?: string): string { + return normalizeEmail(preferred) || normalizeEmail(emails?.find(e => e.default)?.email) || emails?.map(e => normalizeEmail(e.email)).find(Boolean) || ""; } export function isAccessFailure(error: unknown): boolean { const code = typeof error === "string" ? error : error instanceof Error ? error.message : ""; - if (!accessErrors.has(code)) return false; + const rawStatus = error instanceof Error ? (error as Error & { status?: number }).status : undefined; + if (!accessErrors.has(code) && ![401, 404].includes(rawStatus || 0)) return false; if (error instanceof Error) { const upstream = (error as Error & { cause?: { status?: number; httpStatus?: number } }).cause; // Some legacy paths wrap timeouts or 5xx responses as repo_not_found. @@ -48,14 +49,15 @@ export async function notifyOwnerAccessProblem(ownerId: string, error: unknown): // per day. Claim before sending so concurrent requests cannot flood an inbox. const owner = await UserModel.findOneAndUpdate({ _id: ownerId, status: { $nin: ["removed", "banned"] }, - "emails.0": { $exists: true }, + $and: [{ $or: [{ "emails.0": { $exists: true } }, { notificationEmail: { $exists: true, $ne: "" } }] }], $or: [{ accessAlertAfter: { $exists: false } }, { accessAlertAfter: { $lte: now } }], }, { $set: { accessAlertAfter: new Date(now.getTime() + DAY) } }, { new: true }).lean(); if (!owner) return; - const to = notificationEmail(owner.emails); + const to = notificationEmail(owner.emails, owner.notificationEmail); if (!to) return; const base = /^https?:\/\//.test(config.APP_HOSTNAME) ? config.APP_HOSTNAME : `https://${config.APP_HOSTNAME}`; const url = new URL("/connections", base).href; + let rejected = false; try { const response = await fetch("https://api.resend.com/emails", { method: "POST", @@ -67,10 +69,12 @@ export async function notifyOwnerAccessProblem(ownerId: string, error: unknown): text: `Anonymous GitHub could not access GitHub using your connection, or could not read a repository.\n\nOpen your connections page to reconnect your account or restore repository access:\n${url}\n\nYou are receiving this email because you own content on Anonymous GitHub. You can change your alert address on the connections page.\n`, }), }); - if (!response.ok) throw new Error("email_delivery_failed"); + rejected = !response.ok; + if (rejected) throw new Error("email_delivery_failed"); } catch { - // Permit retry on a later access failure, with a short backoff for outages. - await UserModel.updateOne({ _id: ownerId, accessAlertAfter: new Date(now.getTime() + DAY) }, + // Only a definite rejection may retry early. A timeout can occur after + // Resend accepted the message, so retain the daily claim in that case. + if (rejected) await UserModel.updateOne({ _id: ownerId, accessAlertAfter: new Date(now.getTime() + DAY) }, { $set: { accessAlertAfter: new Date(now.getTime() + 10 * 60000) } }); logger.warn("owner access email delivery failed"); } diff --git a/src/server/routes/github-app.ts b/src/server/routes/github-app.ts index 9e5d89b..efb3900 100644 --- a/src/server/routes/github-app.ts +++ b/src/server/routes/github-app.ts @@ -29,6 +29,12 @@ export function consumeFlow(flow: Flow | undefined, state: unknown): Flow { function newFlow(ownerId: string | undefined, returnTo: unknown): Flow { return { state: randomBytes(32).toString("hex"), expires: Date.now() + 10 * 60000, ownerId, returnTo: safeReturnTo(returnTo) }; } +// Parallel user/connections reads must initialize the same token even when +// both loaded the session before either request saved it. +export function connectionCSRF(req: express.Request): string { + return req.session.githubConnectionCSRF ||= createHmac("sha256", config.SESSION_SECRET) + .update(`github-connections:${req.sessionID}`).digest("hex"); +} function saveSession(req: express.Request) { return new Promise((resolve, reject) => req.session.save(err => err ? reject(err) : resolve())); } function enabled(req: express.Request, res: express.Response, next: express.NextFunction) { if (!config.GITHUB_APP_ENABLED || !config.GITHUB_APP_NEW_CONNECTIONS) return res.status(503).json({ error: "github_app_disabled" }); @@ -151,7 +157,7 @@ router.get("/app/setup", enabled, async (req, res) => { router.get("/connections", async (req, res) => { try { const user = await getUser(req); - req.session.githubConnectionCSRF ||= randomBytes(32).toString("hex"); + connectionCSRF(req); await saveSession(req); const credentials = await CredentialModel.find({ ownerId: user.id }).select("provider revoked").lean(); const appConnected = credentials.some(c => c.provider === APP_PROVIDER && !c.revoked); @@ -168,7 +174,7 @@ router.get("/connections", async (req, res) => { const repos = await RepositoryModel.find({ owner: user.id, status: { $ne: "removed" } }).select("repoId source.repositoryName githubAccess status statusDate").lean(); const prs = await PullRequestModel.find({ owner: user.id, status: { $ne: "removed" } }).select("pullRequestId source.repositoryFullName githubAccess status statusDate").lean(); const gistCount = await GistModel.countDocuments({ owner: user.id, status: { $ne: "removed" } }); - res.json({ notificationEmail: notificationEmail(user.model.emails), emailNotificationsEnabled: !!(config.RESEND_API_KEY && config.EMAIL_FROM), + res.json({ notificationEmail: notificationEmail(user.model.emails, user.model.notificationEmail), emailNotificationsEnabled: !!(config.RESEND_API_KEY && config.EMAIL_FROM), csrf: req.session.githubConnectionCSRF, appEnabled: config.GITHUB_APP_ENABLED && config.GITHUB_APP_NEW_CONNECTIONS, gistConnection: config.GITHUB_APP_ENABLED && credentials.some(c => c.provider === APP_PROVIDER) ? "github-app" : "oauth", oauthEnabled: config.GITHUB_OAUTH_ENABLED, oauthConnected: !!(await getCredentialToken(user.id)), appConnected, appError: appErrorCode, @@ -199,7 +205,7 @@ router.post("/connections/email", async (req, res) => { } const email = normalizeEmail(req.body?.email); if (!email) return res.status(400).json({ error: "Enter a valid email address." }); - await UserModel.updateOne({ _id: user.id }, { $set: { emails: [{ email, default: true }] } }); + await UserModel.updateOne({ _id: user.id }, { $set: { notificationEmail: email } }); res.json({ notificationEmail: email }); } catch (error) { handleError(error, res, req); } }); diff --git a/src/server/routes/user.ts b/src/server/routes/user.ts index 259ffeb..ca1bc0b 100644 --- a/src/server/routes/user.ts +++ b/src/server/routes/user.ts @@ -1,8 +1,7 @@ -import { randomBytes } from "crypto"; import { notificationEmail } from "../../core/owner-notifications"; import DashboardName from "../../core/model/dashboard-name"; import { dashboardSummary } from "./dashboard-summary"; -import { revokeGrant } from "./github-app"; +import { connectionCSRF, revokeGrant } from "./github-app"; import CredentialModel from "../../core/model/credentials/credentials.model"; import * as express from "express"; import config from "../../config"; @@ -43,16 +42,16 @@ router.get("/", async (req, res) => { try { const user = await getUser(req); const showEmailPrompt = !!(config.RESEND_API_KEY && config.EMAIL_FROM) && - !notificationEmail(user.model.emails) && !user.model.emailPromptNever && !req.session.emailPromptDismissed; + !notificationEmail(user.model.emails, user.model.notificationEmail) && !user.model.emailPromptNever && !req.session.emailPromptDismissed; if (showEmailPrompt) { - req.session.githubConnectionCSRF ||= randomBytes(32).toString("hex"); + connectionCSRF(req); await new Promise((resolve, reject) => req.session.save(error => error ? reject(error) : resolve())); } res.set("Cache-Control", "no-store"); res.json({ showEmailPrompt, emailPromptCSRF: showEmailPrompt ? req.session.githubConnectionCSRF : undefined, - notificationEmail: notificationEmail(user.model.emails), + notificationEmail: notificationEmail(user.model.emails, user.model.notificationEmail), emailNotificationsEnabled: !!(config.RESEND_API_KEY && config.EMAIL_FROM), username: user.username, photo: user.photo, @@ -239,6 +238,9 @@ router.delete("/", async (req, res) => { repositories: [], }, $unset: { + notificationEmail: "", + emailPromptNever: "", + accessAlertAfter: "", accessTokens: "", accessTokenDates: "", externalIDs: "", diff --git a/test/owner-notifications.test.js b/test/owner-notifications.test.js index 7c52660..c180fba 100644 --- a/test/owner-notifications.test.js +++ b/test/owner-notifications.test.js @@ -17,7 +17,7 @@ describe("owner access notifications", () => { stub(config, "APP_HOSTNAME", "anonymous.example.com"); stub(User, "findOneAndUpdate", (filter, update) => ({ lean: async () => { claims.push(filter); - if (!owner || ["removed", "banned"].includes(owner.status) || !owner.emails.length || after > new Date()) return null; + if (!owner || ["removed", "banned"].includes(owner.status) || (!owner.emails.length && !owner.notificationEmail) || after > new Date()) return null; after = update.$set.accessAlertAfter; return owner; } })); @@ -72,6 +72,32 @@ describe("owner access notifications", () => { expect(retryWrites).to.have.length(1); }); + it("retains the daily claim when delivery may already have happened", async () => { + let attempts = 0; + stub(global, "fetch", async () => { attempts++; throw new Error("response timed out after acceptance"); }); + await notifyOwnerAccessProblem("owner-id", "token_expired"); + expect(retryWrites).to.have.length(0); + expect(after.getTime() - Date.now()).to.be.within(86390000, 86400000); + await notifyOwnerAccessProblem("owner-id", "token_expired"); + expect(attempts).to.equal(1); + }); + + it("uses a dedicated alert address without requiring or replacing profile emails", async () => { + owner.notificationEmail = "alerts@example.com"; + const original = JSON.stringify(owner.emails); + await notifyOwnerAccessProblem("owner-id", "token_expired"); + expect(sent[0].body.to).to.deep.equal(["alerts@example.com"]); + expect(JSON.stringify(owner.emails)).to.equal(original); + after = null; owner.emails = []; + await notifyOwnerAccessProblem("owner-id", "token_expired"); + expect(sent).to.have.length(2); + }); + + 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 [429, 500, 503]) expect(notifications.isAccessFailure(Object.assign(new Error("upstream error"), { status }))).to.equal(false); + }); + it("does not let database failures escape", async () => { stub(User, "findOneAndUpdate", () => { throw new Error("database down"); }); await notifyOwnerAccessProblem("owner-id", "token_expired"); @@ -97,7 +123,7 @@ describe("owner email settings", () => { it("updates only the signed-in owner's address", async () => { const res = response(); await endpoint({ body: { email: " owner@example.com ", ownerId: "someone-else" } }, res); - expect(writes).to.deep.equal([[{ _id: "signed-in-owner" }, { $set: { emails: [{ email: "owner@example.com", default: true }] } }]]); + expect(writes).to.deep.equal([[{ _id: "signed-in-owner" }, { $set: { notificationEmail: "owner@example.com" } }]]); expect(res.body.notificationEmail).to.equal("owner@example.com"); }); it("rejects malformed input without a database write", async () => { @@ -120,6 +146,20 @@ describe("owner email settings", () => { expect(res.statusCode).to.equal(400); expect(writes).to.have.length(0); }); + it("initializes the same CSRF token for parallel session snapshots", () => { + const { connectionCSRF } = require("../src/server/routes/github-app"); + const userRequest = { sessionID: "same-login-session", session: {} }; + const connectionsRequest = { sessionID: "same-login-session", session: {} }; + const popupToken = connectionCSRF(userRequest); + const pageToken = connectionCSRF(connectionsRequest); + expect(popupToken).to.equal(pageToken); + expect(connectionCSRF({ sessionID: "another-login", session: {} })).not.to.equal(pageToken); + for (const token of [popupToken, pageToken]) { + let accepted = false; + csrf({ method: "POST", session: connectionsRequest.session, headers: { "x-csrf-token": token } }, response(), () => { accepted = true; }); + expect(accepted).to.equal(true); + } + }); it("requires the session's CSRF token before changing email", () => { const res = response(); let next = false; csrf({ method: "POST", session: { githubConnectionCSRF: "secret" }, headers: {} }, res, () => { next = true; }); @@ -139,10 +179,11 @@ describe("email popup eligibility", () => { config.RESEND_API_KEY = "test"; config.EMAIL_FROM = "alerts@example.com"; }); afterEach(() => { utils.getUser = oldUser; config.RESEND_API_KEY = oldKey; config.EMAIL_FROM = oldFrom; }); - for (const reason of ["eligible", "email", "never", "later", "no-key", "no-sender"]) { + for (const reason of ["eligible", "email", "alert-email", "never", "later", "no-key", "no-sender"]) { it(`checks ${reason} before showing the form`, async () => { utils.getUser = async () => ({ username: "owner", model: { emails: reason === "email" ? [{ email: "owner@example.com", default: true }] : [], + notificationEmail: reason === "alert-email" ? "alerts@example.com" : undefined, emailPromptNever: reason === "never", } }); if (reason === "no-key") config.RESEND_API_KEY = ""; @@ -199,3 +240,38 @@ describe("repository notification hooks", () => { expect(sent).to.deep.equal([[repo.owner.id, error]]); }); }); + +describe("gist and pull-request notification hooks", () => { + const Gist = require("../src/core/Gist").default; + const GistModel = require("../src/core/model/anonymizedGists/anonymizedGists.model").default; + const PullRequest = require("../src/core/PullRequest").default; + const PullRequestModel = require("../src/core/model/anonymizedPullRequests/anonymizedPullRequests.model").default; + const github = require("../src/core/GitHubUtils"); + const restores = []; + const stub = (object, key, value) => { const old = object[key]; restores.push(() => object[key] = old); object[key] = value; }; + let sent; + beforeEach(() => { sent = []; stub(notifications, "notifyOwnerAccessProblem", async (...args) => { sent.push(args); }); }); + afterEach(() => { while (restores.length) restores.pop()(); }); + for (const type of ["gist", "pull-request"]) for (const failure of ["oauth", "app-access"]) { + it(`alerts the owner on ${type} ${failure} failure and preserves the error`, async () => { + const data = { owner: "507f1f77bcf86cd799439011", source: { gistId: "gist", repositoryFullName: "private/source", pullRequestId: 1 }, options: { expirationMode: "never" } }; + const resource = type === "gist" ? new Gist(new GistModel(data)) : new PullRequest(new PullRequestModel(data)); + const error = failure === "oauth" ? Object.assign(new Error("Bad credentials"), { status: 401 }) : new Error("github_app_access_required"); + if (type === "gist") { + resource.getAccess = async () => { + if (failure === "app-access") throw error; + return { token: "token", connection: "oauth" }; + }; + resource.downloadContent = async () => { throw error; }; + } else { + resource.getToken = async () => { if (failure === "app-access") throw error; return "token"; }; + stub(github, "octokit", () => ({ rest: { pulls: { get: async () => { throw error; } } } })); + } + let caught; + try { await resource.download(); } catch (err) { caught = err; } + expect(caught).to.be.instanceOf(Error); + expect(caught.message).to.equal(type === "gist" && failure === "oauth" ? "github_oauth_required" : error.message); + expect(sent).to.deep.equal([[resource.owner.id, caught]]); + }); + } +});