Home

boring / v2

publicBoring-Software-Inc/v2
Overview Code History Branches Pull requestsIssuesInsights Campfire Docs
main
HomeOverview Code PRsIssues

#165 [Detail Bug] Account deletion can strand an armed repo due to missing in-transaction recheck

open

Opened by detail-app[bot] · 6 days ago

detail-app[bot]opened this issueAuthor· 6 days ago

Sign in to comment.

Linked pull requests

No linked pull requests yet.

Detail Bug Report

app.detail.dev/org_0f605249-55f3-46eb-880c-20ae3e45ae99/…/bug_dc290e36-8de8-49ea-9587-ffadfabdc08a

Introduced in #82 by @ripgrim on Oct 24, 2026

Summary

  • Context: deleteAccount guards against leaving a team org with an armed tripwire repo that nobody can disarm (the repos-on block), and deleteUserAccount performs the actual deletion inside a locked transaction.
  • Bug: The repos-on check is only evaluated on an unlocked, outside-transaction plan; deleteUserAccount's locked transaction re-validates the sole-admin condition but never re-validates that the repos are still disarmed, so a repo armed concurrently between the plan read and the delete is not stopped.
  • Actual vs. expected: Expected — mirroring the intentionally fixed sole-admin race (commit 2c75d5d1) — the deletion transaction should re-check armed repos in sole-turn-off orgs and refuse; actual — the deletion succeeds and strands an armed repo that no remaining member can disarm.
  • Impact: A team org is left with an active tripwire (armed repo) that enforces workflow runs and can block PRs, with no remaining member able to turn it off — the exact state the repos-on check exists to prevent; recovery requires out-of-band intervention.

Code with Bug

// packages/auth/src/account-deletion.ts
export async function deleteAccount(
	db: Db,
	userId: string,
): Promise<AccountDeletionResult> {
	const plan = await planAccountDeletion(db, userId);
	if (plan.soleAdminOrgs.length > 0) {
		return {
			ok: false,
			reason: "sole-admin",
			orgSlugs: plan.soleAdminOrgs.map((org) => org.slug),
		};
	}
	if (plan.onRepos.length > 0) {
		return {
			ok: false,
			reason: "repos-on",       // <-- BUG 🔴 checked only on unlocked plan; can go stale
			repos: plan.onRepos.map((repo) => repo.fullName),
		};
	}
	try {
		return {
			ok: true,
			deleted: await accountDeletionServices.deleteUserAccount(db, userId),
		};
	} catch (error) {
		if (error instanceof accountDeletionServices.AccountDeletionBlockedError) {
			return { ok: false, reason: "sole-admin", orgSlugs: error.orgSlugs };
		}
		throw error;
	}
}
// packages/db/src/services/account-deletion.ts (inside deleteUserAccount's transaction)
	await tx
		.select({ id: member.id })
		.from(member)
		.where(eq(member.userId, userId))
		.for("update");
	await lockMemberOrgs(tx, userId);
	const soleAdmin = await listSoleAdminOrgs(tx, userId);  // <-- re-checked under lock
	if (soleAdmin.length > 0) {
		throw new AccountDeletionBlockedError(soleAdmin.map((org) => org.slug));
	}
	// <-- BUG 🔴 no re-check of armed repos in sole-turn-off orgs under lock
	const personal = await tx
		.select({ id: organization.id })
		.from(member)
		.innerJoin(organization, eq(organization.id, member.organizationId))
		.where(and(eq(member.userId, userId), eq(organization.isPersonal, true)));

Explanation

deleteAccount decides whether deletion is allowed by running planAccountDeletion (unlocked reads) and blocking on plan.onRepos. However, the actual deletion happens later in deleteUserAccount inside a transaction that locks membership/org rows and re-checks only the sole-admin condition. If a repo is armed after the plan is computed but before/during the deletion transaction, nothing re-validates repos-on, so the delete can commit and remove the last connections:manage holder.

A deterministic integration reproduction was built by forcing ordering so a second session arms a repo after planAccountDeletion finishes and before the deletion transaction begins. The delete succeeds, and afterwards the repo remains armed: true while the remaining org member lacks connections:manage, making disarm impossible via normal product paths.

Codebase Inconsistency

A prior concurrency bug in this same flow was fixed for sole-admin: commit 2c75d5d1 added lockMemberOrgs plus an in-transaction listSoleAdminOrgs re-check that throws. The repos-on decision has no corresponding in-transaction re-check, even though it is the same class of stale-plan race.

Exploit Scenario

A user who is the sole holder of connections:manage in a team org can race their own deletion:

  1. Session A starts account deletion; planAccountDeletion sees no armed repos.
  2. Session B arms a repo in the org during the window between planning and deleteUserAccount's transaction.
  3. Deletion completes (only sole-admin is re-validated).
  4. Org is left with an armed: true repo and no remaining member who can disarm it.

Recommended Fix

Inside deleteUserAccount's transaction (after lockMemberOrgs), re-run the armed-repo check for applicable orgs and block deletion (surface as { ok: false, reason: "repos-on" }), mirroring the existing sole-admin in-transaction re-validation.

History

This bug was introduced in commit 7bc280c5 ("feat(db): an account can be deleted without orphaning org history"). repos-on was implemented only as a plan-time guard in deleteAccount, while deleteUserAccount’s transaction re-checked only sole-admin; later commit 2c75d5d1 strengthened the sole-admin path but did not add any in-transaction repos-on re-check.