fix: address owner notification review findings

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.
 

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);

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,
     });

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: [

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;

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");
     }

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<void>((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); }
 });

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<void>((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: "",

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]]);
+    });
+  }
+});