From 633e5ddcae09d8fb4252c0fd887895742b2ae2e1 Mon Sep 17 00:00:00 2001 From: Thomas Durieux <5577568+tdurieux@users.noreply.github.com> Date: Fri, 9 Oct 2026 07:47:51 +0000 Subject: [PATCH] fix: rearm owner alerts across recovery actions safely --- README.md | 4 +- .../anonymizedGists/anonymizedGists.schema.ts | 1 + .../anonymizedPullRequests.schema.ts | 1 + .../anonymizedRepositories.schema.ts | 1 + src/core/owner-notifications.ts | 13 +++- src/server/routes/github-app.ts | 1 + src/server/routes/repository-private.ts | 4 +- test/owner-alert-actions.test.js | 65 +++++++++++++++++++ test/owner-notifications.test.js | 37 +++++++++-- 9 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 test/owner-alert-actions.test.js diff --git a/README.md b/README.md index 1132049..4484c70 100644 --- a/README.md +++ b/README.md @@ -112,7 +112,9 @@ Emails contain no repository names, anonymous URLs, or credentials. An atomic database claim allows one email attempt per repository, pull request, or gist until its owner explicitly refreshes it, saves its settings or connection, -or reconnects GitHub. Reconnecting GitHub resets alerts for that owner's resources. +or reconnects GitHub. Claiming ownership also clears the previous owner's alert. +Reconnecting GitHub or completing a verified App installation update resets alerts +for that owner's resources. Resets invalidate pending claims from older failures. Viewing content, scheduled updates, and automatic token renewal never reset alerts. There is no daily reminder. Claims also remain after delivery failures or an interrupted process, avoiding duplicate messages until the owner acts. diff --git a/src/core/model/anonymizedGists/anonymizedGists.schema.ts b/src/core/model/anonymizedGists/anonymizedGists.schema.ts index ca2fa66..65f06f1 100644 --- a/src/core/model/anonymizedGists/anonymizedGists.schema.ts +++ b/src/core/model/anonymizedGists/anonymizedGists.schema.ts @@ -1,6 +1,7 @@ import { Schema } from "mongoose"; const AnonymizedGistSchema = new Schema({ + accessAlertGeneration: { type: String, select: false }, accessAlertClaimedAt: { type: Date, select: false }, gistId: { type: String, diff --git a/src/core/model/anonymizedPullRequests/anonymizedPullRequests.schema.ts b/src/core/model/anonymizedPullRequests/anonymizedPullRequests.schema.ts index 570343b..8ae76ed 100644 --- a/src/core/model/anonymizedPullRequests/anonymizedPullRequests.schema.ts +++ b/src/core/model/anonymizedPullRequests/anonymizedPullRequests.schema.ts @@ -2,6 +2,7 @@ import { repositoryAccessSchema } from "../repository-access.schema"; import { Schema } from "mongoose"; const AnonymizedPullRequestSchema = new Schema({ + accessAlertGeneration: { type: String, select: false }, accessAlertClaimedAt: { type: Date, select: false }, pullRequestId: { type: String, diff --git a/src/core/model/anonymizedRepositories/anonymizedRepositories.schema.ts b/src/core/model/anonymizedRepositories/anonymizedRepositories.schema.ts index 4d77d40..cd5acf9 100644 --- a/src/core/model/anonymizedRepositories/anonymizedRepositories.schema.ts +++ b/src/core/model/anonymizedRepositories/anonymizedRepositories.schema.ts @@ -2,6 +2,7 @@ import { repositoryAccessSchema } from "../repository-access.schema"; import { Schema } from "mongoose"; const AnonymizedRepositorySchema = new Schema({ + accessAlertGeneration: { type: String, select: false }, accessAlertClaimedAt: { type: Date, select: false }, reviewConsentRevision: { type: Number, select: false }, repoId: { diff --git a/src/core/owner-notifications.ts b/src/core/owner-notifications.ts index e3e4882..1122277 100644 --- a/src/core/owner-notifications.ts +++ b/src/core/owner-notifications.ts @@ -1,3 +1,4 @@ +import { randomUUID } from "crypto"; import { Types } from "mongoose"; import { isGitHubRateLimitError } from "./github-rate-limit"; import config from "../config"; @@ -19,7 +20,7 @@ export async function resetOwnerAccessAlerts(ownerId: string, resource?: AlertRe const filter = { owner: new Types.ObjectId(ownerId), ...(resource ? { _id: new Types.ObjectId(resource.id) } : {}) }; for (const collection of resource ? [resourceCollections[resource.kind]] : Object.values(resourceCollections)) { - await UserModel.db.collection(collection).updateMany(filter, { $unset: { accessAlertClaimedAt: "" } }); + await UserModel.db.collection(collection).updateMany(filter, { $unset: { accessAlertClaimedAt: "" }, $set: { accessAlertGeneration: randomUUID() } }); } } catch { logger.warn("owner access alert reset unavailable"); } } @@ -66,14 +67,20 @@ export async function notifyOwnerAccessProblem(ownerId: string, error: unknown, if (!config.RESEND_API_KEY || !config.EMAIL_FROM || !isAccessFailure(error)) return; try { if (!resource || !Types.ObjectId.isValid(resource.id) || !Types.ObjectId.isValid(ownerId)) return; + const collection = UserModel.db.collection(resourceCollections[resource.kind]); + const identity = { _id: new Types.ObjectId(resource.id), owner: new Types.ObjectId(ownerId) }; + const snapshot = await collection.findOne(identity, { projection: { accessAlertGeneration: 1 } }); + if (!snapshot) return; const owner = await UserModel.findOne({ _id: ownerId, status: { $nin: ["removed", "banned"] } }).lean(); if (!owner) return; const to = notificationEmail(owner.emails, owner.notificationEmail); if (!to) return; // Claim the resource atomically before delivery. This has no expiry: neither // repeated readers nor workers can send again until the owner acts. - const claim = await UserModel.db.collection(resourceCollections[resource.kind]).updateOne({ - _id: new Types.ObjectId(resource.id), owner: new Types.ObjectId(ownerId), + const claim = await collection.updateOne({ + ...identity, + // A reset during the owner lookup invalidates this older failure. + accessAlertGeneration: snapshot.accessAlertGeneration ?? { $exists: false }, accessAlertClaimedAt: { $exists: false }, }, { $set: { accessAlertClaimedAt: new Date() } }); if (!claim.modifiedCount) return; diff --git a/src/server/routes/github-app.ts b/src/server/routes/github-app.ts index c818767..4cd003d 100644 --- a/src/server/routes/github-app.ts +++ b/src/server/routes/github-app.ts @@ -149,6 +149,7 @@ router.get("/app/setup", enabled, async (req, res) => { if (req.query.setup_action !== "request") { const installations = await userInstallations(user.id); if (!installations.some(i => String(i.id) === req.query.installation_id)) throw appError("github_app_access_required"); + await resetOwnerAccessAlerts(user.id); } await UserModel.updateOne({ _id: user.id }, { $set: { repositories: [] } }); res.redirect(flow.returnTo); diff --git a/src/server/routes/repository-private.ts b/src/server/routes/repository-private.ts index 1f19bc3..49b9d40 100644 --- a/src/server/routes/repository-private.ts +++ b/src/server/routes/repository-private.ts @@ -115,7 +115,8 @@ router.post("/claim", async (req, res) => { await AnonymizedRepositoryModel.updateOne( { repoId: repoConfig.repoId }, - { $set: { owner: user.model.id, githubAccess: selectedAccess.binding } } + { $set: { owner: user.model.id, githubAccess: selectedAccess.binding, accessAlertGeneration: randomUUID() }, + $unset: { accessAlertClaimedAt: "" } } ).collation({ locale: "en", strength: 2 }); return res.send("Ok"); } catch (error) { @@ -199,6 +200,7 @@ router.post( { $set: updates } ).exec(); if (saved && saved.matchedCount === 0) throw new AnonymousError("invalid_status", { httpStatus: 409 }); + if (user.id === repo.owner.id) await resetOwnerAccessAlerts(user.id, { kind: "repository", id: String(repo.model._id) }); if (reactivating) { // Expiration removes the cached files. Rebuild the saved commit diff --git a/test/owner-alert-actions.test.js b/test/owner-alert-actions.test.js new file mode 100644 index 0000000..59e0fae --- /dev/null +++ b/test/owner-alert-actions.test.js @@ -0,0 +1,65 @@ +const { expect } = require("chai"); +require("ts-node/register/transpile-only"); +const notifications = require("../src/core/owner-notifications"); +const utils = require("../src/server/routes/route-utils"); +const app = require("../src/core/github-app"); +const db = require("../src/server/database"); +const github = require("../src/core/source/GitHubRepository"); +const Resources = require("../src/core/model/anonymizedRepositories/anonymizedRepositories.model").default; +const Sources = require("../src/core/model/repositories/repositories.model").default; +const Users = require("../src/core/model/users/users.model").default; +const repositoryRouter = require("../src/server/routes/repository-private").default; +const appRouter = require("../src/server/routes/github-app").githubAppRouter; +const handler = (router, path) => router.stack.find(layer => layer.route?.path === path).route.stack.at(-1).handle; + +describe("owner actions rearm access alerts", () => { + let restores, resets; + const owner = "507f1f77bcf86cd799439011"; + function stub(object, key, value) { + const old = object[key]; restores.push(() => { object[key] = old; }); object[key] = value; + } + beforeEach(() => { + restores = []; resets = []; + stub(utils, "getUser", async () => ({ id: owner, model: { id: owner } })); + stub(utils, "handleError", error => { throw error; }); + stub(notifications, "resetOwnerAccessAlerts", async (...args) => { resets.push(args); }); + }); + afterEach(() => { while (restores.length) restores.pop()(); }); + + for (const isOwner of [true, false]) it(`extension ${isOwner ? "rearms for its owner" : "does not rearm for an admin"}`, async () => { + const id = isOwner ? owner : "admin"; + stub(utils, "getUser", async () => ({ id, model: { id }, isAdmin: !isOwner })); + const repo = { owner: { id: owner }, status: "ready", model: { _id: "resource", options: {} } }; + stub(utils, "getRepo", async () => repo); + stub(Resources, "updateOne", () => ({ exec: async () => ({ matchedCount: 1 }) })); + await handler(repositoryRouter, "/:repoId/extend")({}, { json() {} }); + expect(resets).to.deep.equal(isOwner ? [[owner, { kind: "repository", id: "resource" }]] : []); + }); + + it("clears the previous owner's claim atomically when a repository is claimed", async () => { + stub(db, "getRepository", async () => ({ repoId: "resource", model: { source: { repositoryId: "source" } } })); + stub(app, "selectRepositoryAccess", async () => ({ token: "token", binding: { kind: "oauth" } })); + stub(github, "getRepositoryFromGitHub", async () => ({ id: 42 })); + stub(Sources, "findById", async () => ({ externalId: 42 })); + let write; + stub(Resources, "updateOne", (_filter, update) => ({ collation: async () => { write = update; } })); + await handler(repositoryRouter, "/claim")({ body: { repoId: "resource", repoUrl: "https://github.com/owner/repo" }, query: {} }, { send() {} }); + expect(write.$set.owner).to.equal(owner); + expect(write.$set.accessAlertGeneration).to.be.a("string"); + expect(write.$unset).to.have.property("accessAlertClaimedAt"); + }); + + for (const action of ["install", "request", "unverified"]) it(`handles ${action} installation completion without premature resets`, async () => { + stub(app, "userInstallations", async () => [{ id: 42 }]); + stub(Users, "updateOne", async () => ({})); + const req = { session: { + githubInstallFlow: { ownerId: owner, state: "state", expires: Date.now() + 60000, returnTo: "/connections" }, + save: callback => callback(), + }, isAuthenticated: () => true, query: { state: "state", setup_action: action, installation_id: action === "unverified" ? "99" : "42" } }; + let failure; + try { await handler(appRouter, "/app/setup")(req, { redirect() {} }); } catch (error) { failure = error; } + expect(resets).to.deep.equal(action === "install" ? [[owner]] : []); + if (action === "unverified") expect(failure?.message).to.equal("github_app_access_required"); + else expect(failure).to.equal(undefined); + }); +}); diff --git a/test/owner-notifications.test.js b/test/owner-notifications.test.js index aa4f59c..74c7b35 100644 --- a/test/owner-notifications.test.js +++ b/test/owner-notifications.test.js @@ -11,10 +11,10 @@ describe("owner access notifications", () => { const ownerId = "507f1f77bcf86cd799439011"; const resource = { kind: "repository", id: "507f1f77bcf86cd799439012" }; const other = { kind: "repository", id: "507f1f77bcf86cd799439013" }; - let owner, sent, claims, rows; + let owner, sent, claims, rows, generations; beforeEach(() => { owner = { status: "active", emails: [{ email: "owner@example.com", default: true }] }; - sent = []; claims = []; rows = new Map(); + sent = []; claims = []; rows = new Map(); generations = new Map(); stub(config, "RESEND_API_KEY", "test-resend-key"); stub(config, "EMAIL_FROM", "alerts@example.com"); stub(config, "APP_HOSTNAME", "anonymous.example.com"); @@ -22,15 +22,23 @@ describe("owner access notifications", () => { Object.defineProperty(User.db, "readyState", { configurable: true, value: 1 }); restores.push(() => { delete User.db.readyState; }); stub(User.db, "collection", name => ({ + findOne: async filter => { + const key = `${name}:${filter.owner}:${filter._id}`; + if (!generations.has(key)) generations.set(key, undefined); + return { accessAlertGeneration: generations.get(key) }; + }, updateOne: async (filter, update) => { claims.push(filter); const key = `${name}:${filter.owner}:${filter._id}`; - if (rows.has(key)) return { modifiedCount: 0 }; + if (rows.has(key) || generations.get(key) !== (typeof filter.accessAlertGeneration === "string" ? filter.accessAlertGeneration : undefined)) return { modifiedCount: 0 }; rows.set(key, update.$set.accessAlertClaimedAt); return { modifiedCount: 1 }; }, - updateMany: async filter => { - for (const key of rows.keys()) if (key.startsWith(`${name}:${filter.owner}:`) && (!filter._id || key.endsWith(`:${filter._id}`))) rows.delete(key); + updateMany: async (filter, update) => { + for (const key of generations.keys()) if (key.startsWith(`${name}:${filter.owner}:`) && (!filter._id || key.endsWith(`:${filter._id}`))) { + rows.delete(key); + generations.set(key, update.$set.accessAlertGeneration); + } }, })); stub(global, "fetch", async (url, options) => { sent.push({ url, options, body: JSON.parse(options.body) }); return { ok: true }; }); @@ -71,6 +79,25 @@ describe("owner access notifications", () => { expect(sent).to.have.length(3); }); + it("rejects a pre-action failure paused during owner lookup", async () => { + let resume, entered; + const waiting = new Promise(resolve => { entered = resolve; }); + stub(User, "findOne", () => ({ lean: async () => { + entered(); + await new Promise(resolve => { resume = resolve; }); + return owner; + } })); + const oldFailure = notifyOwnerAccessProblem(ownerId, "token_expired", resource); + await waiting; + await notifications.resetOwnerAccessAlerts(ownerId, resource); + resume(); + await oldFailure; + expect(sent).to.have.length(0); + stub(User, "findOne", () => ({ lean: async () => owner })); + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + expect(sent).to.have.length(1); + }); + it("reconnecting resets all of the owner's resource types", async () => { const resources = [resource, { ...resource, kind: "pull-request" }, { ...resource, kind: "gist" }]; for (const item of resources) await notifyOwnerAccessProblem(ownerId, "token_expired", item);