diff --git a/README.md b/README.md index 03c1e75..4484c70 100644 --- a/README.md +++ b/README.md @@ -110,11 +110,14 @@ Alerts cover invalid GitHub credentials and repository access errors encountered while reading or refreshing content. No periodic access scan is performed. 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. 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. +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. 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. Leaving either Resend setting empty disables delivery and the email prompt. ## Scope of anonymization diff --git a/src/core/AnonymizedFile.ts b/src/core/AnonymizedFile.ts index 6bdd354..858a4bb 100644 --- a/src/core/AnonymizedFile.ts +++ b/src/core/AnonymizedFile.ts @@ -271,7 +271,7 @@ export default class AnonymizedFile { }; if (typeof source.fetchFileInfoFromPath !== "function") return null; const recovered = await source.fetchFileInfoFromPath(this.anonymizedPath).catch(error => { - void notifyOwnerAccessProblem(this.repository.owner.id, error); + void notifyOwnerAccessProblem(this.repository.owner.id, error, { kind: "repository", id: String(this.repository.model._id) }); throw error; }); if (!recovered) return null; @@ -360,10 +360,10 @@ export default class AnonymizedFile { }); } const content = await this.repository.source?.getFileContent(this).catch(error => { - void notifyOwnerAccessProblem(this.repository.owner.id, error); + void notifyOwnerAccessProblem(this.repository.owner.id, error, { kind: "repository", id: String(this.repository.model._id) }); throw error; }); - content.on("error", error => { void notifyOwnerAccessProblem(this.repository.owner.id, error); }); + content.on("error", error => { void notifyOwnerAccessProblem(this.repository.owner.id, error, { kind: "repository", id: String(this.repository.model._id) }); }); const cacheWasReset = this.repository.model.isReseted; if (cacheWasReset) { await this.repository.markCachePresent(); @@ -405,7 +405,7 @@ export default class AnonymizedFile { }); content.on("error", err => { const { error } = streamerErrorToAnonymous(err, { repoId: this.repository.repoId, filePath: this.anonymizedPath }); - void notifyOwnerAccessProblem(this.repository.owner.id, error); + void notifyOwnerAccessProblem(this.repository.owner.id, error, { kind: "repository", id: String(this.repository.model._id) }); }); return content; } @@ -485,7 +485,7 @@ export default class AnonymizedFile { filePath: this.anonymizedPath, } ); - void notifyOwnerAccessProblem(this.repository.owner.id, error); + void notifyOwnerAccessProblem(this.repository.owner.id, error, { kind: "repository", id: String(this.repository.model._id) }); error.value = this; handleError(error, res); if (res.headersSent && !res.writableEnded) res.destroy(); diff --git a/src/core/Gist.ts b/src/core/Gist.ts index 10e1e6b..0f9dd34 100644 --- a/src/core/Gist.ts +++ b/src/core/Gist.ts @@ -70,7 +70,7 @@ export default class Gist { async download() { try { await this.downloadWithAccess(); } catch (error) { - void notifyOwnerAccessProblem(this.owner.id, error); + void notifyOwnerAccessProblem(this.owner.id, error, { kind: "gist", id: String(this.model._id) }); throw error; } } diff --git a/src/core/GitHubUtils.ts b/src/core/GitHubUtils.ts index 040b3cd..97e5868 100644 --- a/src/core/GitHubUtils.ts +++ b/src/core/GitHubUtils.ts @@ -263,7 +263,7 @@ export function octokit(token: string) { export { waitForTokenGate }; -export async function checkToken(token: string, ownerId?: string) { +export async function checkToken(token: string) { const oct = octokit(token); try { const context = githubTokenContext(token); @@ -275,7 +275,6 @@ export async function checkToken(token: string, ownerId?: string) { // 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; } } @@ -286,7 +285,7 @@ export async function getToken(repository: Repository) { try { return await measureStage("authorization", () => resolveRepositoryToken(repository)); } catch (error) { - void notifyOwnerAccessProblem(repository.owner.id, error); + void notifyOwnerAccessProblem(repository.owner.id, error, { kind: "repository", id: String(repository.model._id) }); throw error; } } @@ -321,11 +320,12 @@ async function resolveRepositoryToken(repository: Repository) { return refreshed; } } - const check = await checkToken(ownerAccessToken, repository.owner.id); + const check = await checkToken(ownerAccessToken); if (check) { checkedRepositoryTokens.set(repository, ownerAccessToken); return ownerAccessToken; } + void notifyOwnerAccessProblem(repository.owner.id, "token_expired", { kind: "repository", id: String(repository.model._id) }); return config.GITHUB_TOKEN; } return (await getCredentialToken(repository.owner.id, "github", { diff --git a/src/core/PullRequest.ts b/src/core/PullRequest.ts index 3b63401..35b4198 100644 --- a/src/core/PullRequest.ts +++ b/src/core/PullRequest.ts @@ -36,7 +36,7 @@ export default class PullRequest { async download() { try { await this.downloadContent(); } catch (error) { - void notifyOwnerAccessProblem(this.owner.id, error); + void notifyOwnerAccessProblem(this.owner.id, error, { kind: "pull-request", id: String(this.model._id) }); throw error; } } diff --git a/src/core/Repository.ts b/src/core/Repository.ts index 0e1ce4a..e99dc58 100644 --- a/src/core/Repository.ts +++ b/src/core/Repository.ts @@ -225,7 +225,7 @@ export default class Repository { const streamed = this.model.source.type !== "Zip"; const source = this.source; const files = await source.getFiles(opt.progress).catch(error => { - void notifyOwnerAccessProblem(this.owner.id, error); + void notifyOwnerAccessProblem(this.owner.id, error, { kind: "repository", id: String(this.model._id) }); throw error; }); const sourceWithTruncation = source as unknown as { truncatedFolderList?: string[] }; @@ -630,7 +630,7 @@ export default class Repository { async updateIfNeeded(opt?: { force: boolean }): Promise { try { await this.refreshIfNeeded(opt); } catch (error) { - void notifyOwnerAccessProblem(this.owner.id, error); + void notifyOwnerAccessProblem(this.owner.id, error, { kind: "repository", id: String(this.model._id) }); throw error; } } diff --git a/src/core/github-app.ts b/src/core/github-app.ts index 1fb72eb..83d0bea 100644 --- a/src/core/github-app.ts +++ b/src/core/github-app.ts @@ -1,4 +1,3 @@ -import { notifyOwnerAccessProblem } from "./owner-notifications"; import { registerGitHubToken } from "./github-token-context"; import { createSign, randomUUID } from "crypto"; import { readFileSync } from "fs"; @@ -88,14 +87,6 @@ export async function saveAppGrant(ownerId: string, data: AppTokenResponse) { } export async function appUserToken(ownerId: string): Promise { - try { return await resolveAppUserToken(ownerId); } - catch (error) { - void notifyOwnerAccessProblem(ownerId, error); - throw error; - } -} - -async function resolveAppUserToken(ownerId: string): Promise { if (!config.GITHUB_APP_ENABLED) throw appError("github_app_disabled", 503); const user = await UserModel.findById(ownerId).select("status externalIDs").lean(); if (!user || user.status === "removed" || user.status === "banned") throw appError(); diff --git a/src/core/model/anonymizedGists/anonymizedGists.schema.ts b/src/core/model/anonymizedGists/anonymizedGists.schema.ts index f825010..65f06f1 100644 --- a/src/core/model/anonymizedGists/anonymizedGists.schema.ts +++ b/src/core/model/anonymizedGists/anonymizedGists.schema.ts @@ -1,6 +1,8 @@ import { Schema } from "mongoose"; const AnonymizedGistSchema = new Schema({ + accessAlertGeneration: { type: String, select: false }, + accessAlertClaimedAt: { type: Date, select: false }, gistId: { type: String, index: { unique: true }, diff --git a/src/core/model/anonymizedPullRequests/anonymizedPullRequests.schema.ts b/src/core/model/anonymizedPullRequests/anonymizedPullRequests.schema.ts index 9fab883..8ae76ed 100644 --- a/src/core/model/anonymizedPullRequests/anonymizedPullRequests.schema.ts +++ b/src/core/model/anonymizedPullRequests/anonymizedPullRequests.schema.ts @@ -2,6 +2,8 @@ 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, index: { unique: true }, diff --git a/src/core/model/anonymizedRepositories/anonymizedRepositories.schema.ts b/src/core/model/anonymizedRepositories/anonymizedRepositories.schema.ts index c09db82..cd5acf9 100644 --- a/src/core/model/anonymizedRepositories/anonymizedRepositories.schema.ts +++ b/src/core/model/anonymizedRepositories/anonymizedRepositories.schema.ts @@ -2,6 +2,8 @@ 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: { type: String, diff --git a/src/core/owner-notifications.ts b/src/core/owner-notifications.ts index 5f1b315..1122277 100644 --- a/src/core/owner-notifications.ts +++ b/src/core/owner-notifications.ts @@ -1,10 +1,29 @@ +import { randomUUID } from "crypto"; +import { Types } from "mongoose"; import { isGitHubRateLimitError } from "./github-rate-limit"; import config from "../config"; import UserModel from "./model/users/users.model"; import { createLogger } from "./logger"; const logger = createLogger("owner-notifications"); -const DAY = 24 * 60 * 60 * 1000; +const resourceCollections = { + repository: "anonymizedrepositories", + "pull-request": "anonymizedpullrequests", + gist: "anonymizedgists", +} as const; +export interface AlertResource { kind: keyof typeof resourceCollections; id: string; } + +// Only explicit owner actions call this; background fetches never reset alerts. +export async function resetOwnerAccessAlerts(ownerId: string, resource?: AlertResource): Promise { + if (UserModel.db.readyState !== 1 || !Types.ObjectId.isValid(ownerId)) return; + try { + 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: "" }, $set: { accessAlertGeneration: randomUUID() } }); + } + } catch { logger.warn("owner access alert reset unavailable"); } +} const accessErrors = new Set([ "token_expired", "github_oauth_required", "github_app_reconnect_required", "github_app_access_required", "repo_not_found", "repository_not_found", @@ -44,23 +63,29 @@ export function isAccessFailure(error: unknown): boolean { } /** Best effort; mail and database failures must not change repository behavior. */ -export async function notifyOwnerAccessProblem(ownerId: string, error: unknown): Promise { +export async function notifyOwnerAccessProblem(ownerId: string, error: unknown, resource: AlertResource): Promise { if (!config.RESEND_API_KEY || !config.EMAIL_FROM || !isAccessFailure(error)) return; try { - const now = new Date(); - // The atomic claim limits all repositories and workers to one email per owner - // per day. Claim before sending so concurrent requests cannot flood an inbox. - const owner = await UserModel.findOneAndUpdate({ - _id: ownerId, status: { $nin: ["removed", "banned"] }, - $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 (!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 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; 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", @@ -72,13 +97,10 @@ 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`, }), }); - rejected = !response.ok; - if (rejected) throw new Error("email_delivery_failed"); + if (!response.ok) throw new Error("email_delivery_failed"); } catch { - // 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) } }); + // Keep the claim even on failure: delivery can be ambiguous and no + // automatic retry may send another email before owner action. logger.warn("owner access email delivery failed"); } } catch { diff --git a/src/queue/processes/downloadRepository.ts b/src/queue/processes/downloadRepository.ts index ba44600..2b9a8e1 100644 --- a/src/queue/processes/downloadRepository.ts +++ b/src/queue/processes/downloadRepository.ts @@ -143,7 +143,7 @@ export default async function (job: SandboxedJob) { if (error instanceof DelayedError || (error instanceof Error && error.name === "DelayedError")) { throw error; } - await notifyOwnerAccessProblem(repo.owner.id, error); + await notifyOwnerAccessProblem(repo.owner.id, error, { kind: "repository", id: String(repo.model._id) }); logger.error("finished with error", { ...serializeError(error), repoId: job.data.repoId, diff --git a/src/server/routes/connection.ts b/src/server/routes/connection.ts index 66e24eb..1e1fe00 100644 --- a/src/server/routes/connection.ts +++ b/src/server/routes/connection.ts @@ -1,3 +1,4 @@ +import { resetOwnerAccessAlerts } from "../../core/owner-notifications"; import { createClient } from "redis"; import * as passport from "passport"; import * as session from "express-session"; @@ -120,6 +121,7 @@ export const verify = async ( return; } await setCredential(String(user!._id), accessToken); + await resetOwnerAccessAlerts(String(user!._id)); done(null, { username: user!.username, user }); } catch (error) { logger.error("verify failed", serializeError(error)); diff --git a/src/server/routes/gist-private.ts b/src/server/routes/gist-private.ts index 59882a1..548ad3f 100644 --- a/src/server/routes/gist-private.ts +++ b/src/server/routes/gist-private.ts @@ -1,3 +1,4 @@ +import { resetOwnerAccessAlerts } from "../../core/owner-notifications"; import * as express from "express"; import { ensureAuthenticated } from "./connection"; @@ -29,6 +30,7 @@ router.post( const user = await getUser(req); isOwnerOrAdmin([gist.owner.id], user); + if (user.id === gist.owner.id) await resetOwnerAccessAlerts(user.id, { kind: "gist", id: String(gist.model._id) }); await gist.updateIfNeeded({ force: true }); res.json({ status: gist.status }); } catch (error) { @@ -68,6 +70,7 @@ router.post( { $set: { "options.expirationDate": newExpiration } } ).exec(); + if (user.id === gist.owner.id) await resetOwnerAccessAlerts(user.id, { kind: "gist", id: String(gist.model._id) }); await gist.updateIfNeeded({ force: true }); res.json({ status: gist.status, expirationDate: newExpiration }); } catch (error) { @@ -222,6 +225,7 @@ router.post( } ).exec(); await gist.updateStatus(RepositoryStatus.PREPARING); + if (user.id === gist.owner.id) await resetOwnerAccessAlerts(user.id, { kind: "gist", id: String(gist.model._id) }); await gist.updateIfNeeded({ force: true }); res.json(gist.toJSON()); } catch (error) { diff --git a/src/server/routes/github-app.ts b/src/server/routes/github-app.ts index efb3900..4cd003d 100644 --- a/src/server/routes/github-app.ts +++ b/src/server/routes/github-app.ts @@ -1,4 +1,4 @@ -import { notificationEmail, normalizeEmail } from "../../core/owner-notifications"; +import { notificationEmail, normalizeEmail, resetOwnerAccessAlerts } from "../../core/owner-notifications"; import * as express from "express"; import { createHmac, randomBytes, randomUUID, timingSafeEqual } from "crypto"; import config from "../../config"; @@ -94,6 +94,7 @@ router.get("/app/callback", enabled, async (req, res) => { } if (isDisabledAccount(user.status)) throw appError("not_connected", 403); await saveAppGrant(user.id, tokens); + await resetOwnerAccessAlerts(user.id); await new Promise((resolve, reject) => req.login({ username: user!.username, user }, err => err ? reject(err) : resolve())); res.redirect(flow.install ? `/github/app/install?returnTo=${encodeURIComponent(flow.returnTo)}&repository=${encodeURIComponent(flow.repository || "")}` : flow.returnTo); } catch (error) { handleError(error, res, req); } @@ -148,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); @@ -251,6 +253,7 @@ router.post("/connections/migrate", async (req, res) => { } }; const result = isRepo ? await RepositoryModel.updateOne(filter, change) : await PullRequestModel.updateOne(filter, change); if (!result.modifiedCount) throw appError("connection_changed", 409); + await resetOwnerAccessAlerts(user.id, { kind: isRepo ? "repository" : "pull-request", id: String(model._id) }); res.json({ connection }); } catch (error) { handleError(error, res, req); } }); diff --git a/src/server/routes/pullRequest-private.ts b/src/server/routes/pullRequest-private.ts index e620604..5c16444 100644 --- a/src/server/routes/pullRequest-private.ts +++ b/src/server/routes/pullRequest-private.ts @@ -1,3 +1,4 @@ +import { resetOwnerAccessAlerts } from "../../core/owner-notifications"; import { selectRepositoryAccess } from "../../core/github-app"; import * as express from "express"; import { ensureAuthenticated } from "./connection"; @@ -30,6 +31,7 @@ router.post( const user = await getUser(req); isOwnerOrAdmin([pullRequest.owner.id], user); + if (user.id === pullRequest.owner.id) await resetOwnerAccessAlerts(user.id, { kind: "pull-request", id: String(pullRequest.model._id) }); await pullRequest.updateIfNeeded({ force: true }); res.json({ status: pullRequest.status }); } catch (error) { @@ -71,6 +73,7 @@ router.post( { $set: { "options.expirationDate": newExpiration } } ).exec(); + if (user.id === pullRequest.owner.id) await resetOwnerAccessAlerts(user.id, { kind: "pull-request", id: String(pullRequest.model._id) }); await pullRequest.updateIfNeeded({ force: true }); res.json({ status: pullRequest.status, expirationDate: newExpiration }); } catch (error) { @@ -242,6 +245,7 @@ router.post( } ).exec(); await pullRequest.updateStatus(RepositoryStatus.PREPARING); + if (user.id === pullRequest.owner.id) await resetOwnerAccessAlerts(user.id, { kind: "pull-request", id: String(pullRequest.model._id) }); await pullRequest.updateIfNeeded({ force: true }); res.json({ ...pullRequest.toJSON(), connection: pullRequest.model.githubAccess?.kind || "oauth" }); } catch (error) { diff --git a/src/server/routes/repository-private.ts b/src/server/routes/repository-private.ts index f4983ad..49b9d40 100644 --- a/src/server/routes/repository-private.ts +++ b/src/server/routes/repository-private.ts @@ -1,3 +1,4 @@ +import { resetOwnerAccessAlerts } from "../../core/owner-notifications"; import { randomUUID } from "crypto"; import { githubQuotaKey } from "../../core/github-token-context"; import { selectRepositoryAccess, boundAppToken, appError } from "../../core/github-app"; @@ -114,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) { @@ -146,6 +148,7 @@ router.post( throw new AnonymousError("invalid_status", { httpStatus: 409 }); } + if (user.id === repo.owner.id) await resetOwnerAccessAlerts(user.id, { kind: "repository", id: String(repo.model._id) }); await repo.refresh(); res.json({ status: repo.status }); } catch (error) { @@ -197,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 @@ -621,6 +625,7 @@ router.post( } ).exec(); if (!saved.matchedCount) throw appError("connection_changed", 409); + if (user.id === repo.owner.id) await resetOwnerAccessAlerts(user.id, { kind: "repository", id: String(repo.model._id) }); if (!sourceChanged && !reactivating) { return res.json({ status: repo.status }); } 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 54c94fa..74c7b35 100644 --- a/test/owner-notifications.test.js +++ b/test/owner-notifications.test.js @@ -8,20 +8,39 @@ const { normalizeEmail, notificationEmail, notifyOwnerAccessProblem } = notifica describe("owner access notifications", () => { const restores = []; const stub = (object, key, value) => { const old = object[key]; restores.push(() => object[key] = old); object[key] = value; }; - let sent, claims, retryWrites, owner, after; + const ownerId = "507f1f77bcf86cd799439011"; + const resource = { kind: "repository", id: "507f1f77bcf86cd799439012" }; + const other = { kind: "repository", id: "507f1f77bcf86cd799439013" }; + let owner, sent, claims, rows, generations; beforeEach(() => { - sent = []; claims = []; retryWrites = []; after = null; owner = { status: "active", emails: [{ email: "owner@example.com", default: true }] }; + 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"); - stub(User, "findOneAndUpdate", (filter, update) => ({ lean: async () => { - claims.push(filter); - if (!owner || ["removed", "banned"].includes(owner.status) || (!owner.emails.length && !owner.notificationEmail) || after > new Date()) return null; - after = update.$set.accessAlertAfter; - return owner; - } })); - stub(User, "updateOne", async (filter, update) => { retryWrites.push({ filter, update }); after = update.$set.accessAlertAfter; }); + stub(User, "findOne", () => ({ lean: async () => ["removed", "banned"].includes(owner?.status) ? null : owner })); + 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) || 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, 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 }; }); }); afterEach(() => { while (restores.length) restores.pop()(); }); @@ -34,63 +53,92 @@ describe("owner access notifications", () => { expect(notificationEmail([{ email: "first@example.com", default: false }, { email: "primary@example.com", default: true }])).to.equal("primary@example.com"); }); - it("sends one private Resend message across concurrent failures", async () => { - await Promise.all(Array.from({ length: 10 }, () => notifyOwnerAccessProblem("owner-id", "token_expired"))); + it("sends once per resource across concurrent readers and repeated errors, with no time expiry", async () => { + await Promise.all(Array.from({ length: 10 }, () => notifyOwnerAccessProblem(ownerId, "token_expired", resource))); expect(sent).to.have.length(1); + for (const key of rows.keys()) rows.set(key, new Date("2000-01-01")); + await notifyOwnerAccessProblem(ownerId, "repo_not_found", resource); + expect(sent).to.have.length(1); + await notifyOwnerAccessProblem(ownerId, "repo_not_found", other); + expect(sent).to.have.length(2); expect(sent[0].url).to.equal("https://api.resend.com/emails"); - expect(sent[0].options.headers.Authorization).to.equal("Bearer test-resend-key"); expect(sent[0].body.to).to.deep.equal(["owner@example.com"]); expect(sent[0].body.text).to.include("https://anonymous.example.com/connections"); - expect(JSON.stringify(sent[0].body)).not.to.include("test-resend-key"); - expect(claims[0].status).to.deep.equal({ $nin: ["removed", "banned"] }); - expect(claims[0].$or).to.have.length(2); + expect(claims[0].accessAlertClaimedAt).to.deep.equal({ $exists: false }); }); - it("ignores disabled delivery, missing recipients and transient failures", async () => { - for (const code of ["github_rate_limit_exceeded", "github_unavailable", "ETIMEDOUT", "quota_exceeded"]) { - await notifyOwnerAccessProblem("owner-id", new Error(code)); - } - await notifyOwnerAccessProblem("owner-id", Object.assign(new Error("repo_not_found"), { cause: { status: 503 } })); - await notifyOwnerAccessProblem("owner-id", Object.assign(new Error("token_expired"), { cause: new Error("network timeout") })); - expect(claims).to.have.length(0); - owner.emails = []; - await notifyOwnerAccessProblem("owner-id", "repo_not_found"); - owner.emails = [{ email: "owner@example.com", default: true }]; - owner.status = "banned"; - await notifyOwnerAccessProblem("owner-id", "repo_not_found"); - config.RESEND_API_KEY = ""; - await notifyOwnerAccessProblem("owner-id", "token_expired"); - expect(sent).to.have.length(0); - }); - - it("backs off after delivery failure without throwing", async () => { - stub(global, "fetch", async () => ({ ok: false })); - await notifyOwnerAccessProblem("owner-id", "repo_not_accessible"); - expect(retryWrites).to.have.length(1); - expect(after.getTime() - Date.now()).to.be.within(590000, 600000); - await notifyOwnerAccessProblem("owner-id", "repo_not_accessible"); - 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"); + it("owner action resets only the selected resource and cannot reset another owner's alerts", async () => { + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + await notifyOwnerAccessProblem(ownerId, "token_expired", other); + await notifications.resetOwnerAccessAlerts("507f1f77bcf86cd799439099", resource); + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); expect(sent).to.have.length(2); + await notifications.resetOwnerAccessAlerts(ownerId, resource); + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + await notifyOwnerAccessProblem(ownerId, "token_expired", other); + 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); + expect(sent).to.have.length(3); + await notifications.resetOwnerAccessAlerts(ownerId); + for (const item of resources) await notifyOwnerAccessProblem(ownerId, "token_expired", item); + expect(sent).to.have.length(6); + }); + + for (const failure of ["rejected", "timeout"]) it(`retains the claim after ${failure} until owner action`, async () => { + let attempts = 0; + stub(global, "fetch", async () => { attempts++; if (failure === "timeout") throw new Error("timeout"); return { ok: false }; }); + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + expect(attempts).to.equal(1); + await notifications.resetOwnerAccessAlerts(ownerId, resource); + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + expect(attempts).to.equal(2); + }); + + it("does not claim an alert without configuration, an active owner, and an address", async () => { + config.RESEND_API_KEY = ""; + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + config.RESEND_API_KEY = "test-key"; + owner.status = "banned"; + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + owner.status = "active"; owner.emails = []; + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + expect(claims).to.have.length(0); + owner.notificationEmail = "alerts@example.com"; + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + expect(sent[0].body.to).to.deep.equal(["alerts@example.com"]); + }); + + it("ignores transient failures and contains database failures", async () => { + await notifyOwnerAccessProblem(ownerId, new Error("github_rate_limit_exceeded"), resource); + await notifyOwnerAccessProblem(ownerId, Object.assign(new Error("repo_not_found"), { cause: { status: 503 } }), resource); + expect(claims).to.have.length(0); + stub(User, "findOne", () => { throw new Error("database down"); }); + await notifyOwnerAccessProblem(ownerId, "token_expired", resource); + expect(sent).to.have.length(0); }); it("recognizes raw authentication and missing-resource errors without alerting for outages", () => { @@ -113,14 +161,8 @@ describe("owner access notifications", () => { 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"); - expect(sent).to.have.length(0); - }); }); - describe("owner email settings", () => { const utils = require("../src/server/routes/route-utils"); const router = require("../src/server/routes/github-app").githubAppRouter; @@ -238,14 +280,14 @@ describe("repository notification hooks", () => { const error = new Error("github_app_access_required"); stub(app, "boundAppToken", async () => { throw error; }); await failsWith(() => tokens.getToken(repo), error); - expect(sent).to.deep.equal([[repo.owner.id, error]]); + expect(sent).to.deep.equal([[repo.owner.id, error, { kind: "repository", id: String(repo.model._id) }]]); }); it("notifies the saved owner when reading the repository tree fails", async () => { const error = new Error("repo_not_found"); stub(Files, "exists", () => ({ exec: async () => null })); Object.defineProperty(repo, "source", { value: { getFiles: async () => { throw error; } } }); await failsWith(() => repo.files(), error); - expect(sent).to.deep.equal([[repo.owner.id, error]]); + expect(sent).to.deep.equal([[repo.owner.id, error, { kind: "repository", id: String(repo.model._id) }]]); }); for (const step of ["branches", "commit"]) it(`notifies on ${step} access errors during refresh`, async () => { const error = Object.assign(new Error("Forbidden"), { status: 403 }); @@ -256,14 +298,14 @@ describe("repository notification hooks", () => { getCommitInfo: async () => { throw error; }, })); await failsWith(() => repo.updateIfNeeded({ force: true }), error); - expect(sent).to.deep.equal([[repo.owner.id, error]]); + expect(sent).to.deep.equal([[repo.owner.id, error, { kind: "repository", id: String(repo.model._id) }]]); }); it("notifies the saved owner when a repository refresh cannot read GitHub", async () => { const error = new Error("repo_not_found"); repo.getToken = async () => "token"; stub(github, "getRepositoryFromGitHub", async () => { throw error; }); await failsWith(() => repo.updateIfNeeded({ force: true }), error); - expect(sent).to.deep.equal([[repo.owner.id, error]]); + expect(sent).to.deep.equal([[repo.owner.id, error, { kind: "repository", id: String(repo.model._id) }]]); }); }); @@ -297,7 +339,7 @@ describe("gist and pull-request notification hooks", () => { 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]]); + expect(sent).to.deep.equal([[resource.owner.id, caught, { kind: type, id: String(resource.model._id) }]]); }); } }); diff --git a/test/repository-refresh.test.js b/test/repository-refresh.test.js index 7014877..fd1dd25 100644 --- a/test/repository-refresh.test.js +++ b/test/repository-refresh.test.js @@ -50,6 +50,22 @@ describe("repository refresh and restoration", () => { expect(failure.httpStatus).to.equal(403); }); + for (const actingOwner of [true, false]) { + it(`${actingOwner ? "rearms" : "does not rearm"} alerts when ${actingOwner ? "the owner" : "an admin"} refreshes`, async () => { + const repo = repository("ready"); + const notifications = require("../src/core/owner-notifications"); + const resets = []; + stub(notifications, "resetOwnerAccessAlerts", async (...args) => { resets.push(args); }); + const id = actingOwner ? repo.owner.id : "admin"; + stub(utils, "getUser", async () => ({ id, model: { id }, isAdmin: !actingOwner })); + repo.refresh = async () => { + expect(resets).to.have.length(actingOwner ? 1 : 0); + }; + await refresh({}, { json: () => {} }); + expect(resets).to.deep.equal(actingOwner ? [[repo.owner.id, { kind: "repository", id: String(repo.model._id) }]] : []); + }); + } + for (const commit of ["saved-sha", "new-sha"]) { it(`rebuilds a removed repository at ${commit} while preserving its ID`, async () => { const repo = repository("removed");