Skip to content

fix(orgs): make organization deletion temporarily unavailable (#6827) - #7560

Merged
bokelley merged 1 commit into
mainfrom
fix/organization-deletion-containment
Sep 18, 2026
Merged

bokelley merged 1 commit into
mainfrom
fix/organization-deletion-containment

Conversation

@bokelley

@bokelley bokelley commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

The self-service and administrative organization-deletion routes now return the generic 503 organization_deletion_unavailable response before route authentication or identity hydration, provider calls, billing reads, and deletion work. Their authorization observer is excluded so the response cannot trigger provider reads afterward. Global parsing and CSRF behavior remain in place.

Mounted tests cover credentials, linked identities, roles, selectors, replay, observer behavior, and unchanged PostgreSQL state. The containment document records the remaining limitations and the durable lifecycle requirements for re-enabling deletion.

This eight-file change preserves the membership and authorization correction landed in #7538. Merge and operator bypass containment remains the separate follow-on in #7561. No deletion journal, reconciliation implementation, or rollback guarantee is added.

The document now includes a unique description and AdCP-prefixed og:title, fixing the SEO failure on the first published candidate. These five frontmatter lines are the only addition to the original #7560 delta: the containment source, tests, and document body remain unchanged. The document is therefore no longer byte-identical to the original #7560 blob.

Validation on the amended candidate:

  • SEO: all 378 pages have both required fields, with zero errors and no new warnings on the containment document.
  • 587 focused unit/integration tests across 17 files, using a fresh isolated local PostgreSQL database.
  • Fresh ordinary migrations and an idempotent repeat; both typechecks; repository migration, schema-link, navigation, changeset-scope, title, and whitespace checks.
  • All five repository Semgrep security rules on the three changed production files, with zero findings or scan errors.
  • Full unmodified pre-commit hook: 1,063 root tests passed; 10,748 server tests passed, with 31 existing skips; both typechecks passed. Full unmodified pre-push hook passed.

Candidate commit: 95f9e0f22bfa24650b3ec805b3150213439cae42
Sole parent: 9d247db2e6e2af755951cba268b060425f246e2f
Reviewed tree: 8cfdad362eab77f39ece3e515f18be647d40f88c

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean defensive containment slice for #6827.

Both organization-deletion entry points (self-service DELETE /api/organizations/:orgId and admin DELETE /api/admin/accounts/:orgId) now return an unconditional 503 organization_deletion_unavailable before authentication, authority resolution, billing/provider reads, audit writes, and cascades, with the authorization observer correctly excluded. Reviewer verified return-before-auth (per-route guards removed, no dangling imports), effect-free proof in the 299-line integration test, app-only scope (no protocol surface, no changeset required or present), and the merge-path writer as genuinely pre-existing.

No blocking, High, or Medium code defects. Two documented non-blocking follow-ups noted (503-driven 5xx dashboard inflation, stale admin-UI delete copy) — neither actionable here.

Decision path: no critical/high findings (row 1 n/a); gated_paths false (row 2 n/a); high-risk reasons are (modified) not (deleted) (row 3 n/a); no medium findings at all (rows 4/5/8 n/a); no prior decision (row 6 n/a); no author team gate (row 7 n/a). The high_risk flag is a heuristic on modified server/src/routes/** files with zero medium-or-higher concerns, so the modification is presumed safe. Falls through to row 9 → approve.

@bokelley
bokelley marked this pull request as draft September 17, 2026 08:04
@bokelley
bokelley force-pushed the fix/organization-deletion-containment branch from 97652f8 to 468adfa Compare September 17, 2026 08:32
@bokelley
bokelley marked this pull request as ready for review September 17, 2026 08:35
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Sep 17, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean pass, no blocking or medium findings.

This PR is a patch-identical restack of a previously clean-approved tree (prior decision: approve). Both DELETE org-deletion handlers (organizations.ts:503, accounts-billing.ts:362) now return an unconditional 503 before any auth/authority/billing/provider/audit work, with the observer exclusion applied. Route matching is not broadened and no org data reaches the wire. Mounted Postgres integration and unit suites confirm zero DB/provider/audit/cache/notification effect across sibling substitution, all roles, platform admin, selectors, subscription states, replay, and CSRF ordering.

No protocol surface (static/schemas/source/, docs/reference/, dist/**), changeset, or dist artifact is touched — this is an operational/app change, so changeset, schema↔docs, oneOf, and immutability gates do not apply. The only operational cost (503 inflating /api 5xx metrics) is documented with mitigation.

Decision-table walk: no critical/high (row 1 no), gated_paths false (row 2 no), no deletions in high-risk reasons — both are (modified) (row 3 no), no medium findings (rows 4/5/8 no), prior decision was approve not escalate (row 6 no), no no-auto-approve team match (row 7 no). Falls through to row 9 → approve. high_risk is true only via (modified) route files with zero medium-or-higher findings, so the modifications are presumed safe per the high-risk-flag guidance. review_decision is APPROVED.

@bokelley
bokelley force-pushed the fix/organization-membership-exact-credential-writes branch from 3085c09 to acad5ed Compare September 17, 2026 22:21
Base automatically changed from fix/organization-membership-exact-credential-writes to main September 17, 2026 22:42
@bokelley
bokelley dismissed aao-secretariat[bot]’s stale review September 17, 2026 22:42

The base branch was changed.

@bokelley
bokelley marked this pull request as draft September 18, 2026 00:00
@bokelley
bokelley force-pushed the fix/organization-deletion-containment branch from 468adfa to 2a97bee Compare September 18, 2026 00:00
Return a stable 503 from the self-service and administrative organization
deletion routes before route authentication, provider calls, or local effects.
Exclude these contained requests from post-response authorization observation.

Preserve the landed membership and authentication correction. Keep organization
merge and operator bypass containment in the separate dependent change. Document
the durable lifecycle requirements for re-enabling deletion without claiming a
journal, compensation, or rollback guarantee.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bokelley
bokelley force-pushed the fix/organization-deletion-containment branch from 2a97bee to 95f9e0f Compare September 18, 2026 01:05
@bokelley
bokelley marked this pull request as ready for review September 18, 2026 01:14

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — metadata-only follow-up to a previously clean approval.

The only change since the prior clean approve (head 468adfa) is three frontmatter lines added to docs/contributing/organization-deletion-containment.md (description + AdCP-prefixed og:title) to satisfy the SEO gate. Naming is correct ("AdCP"), and no wire/schema/changeset/normative reference surface is touched — this is a contributing doc, not docs/reference/** or static/schemas/source/**. The containment code and test suites are identical to the prior approved candidate.

Checks applied:

  • No critical/high/medium findings this run (rows 1, 4, 8 do not fire).
  • gated_paths: false (row 2 does not fire).
  • high_risk is true only via server/src/routes/** files that are (modified) but carry no findings — under the flag-reading rules a modified high-risk file with no medium-or-higher concern is presumed safe; rows 3 and 5 require a deletion or a medium finding respectively, neither present.
  • Prior decision was approve, so sticky escalation (row 6) does not apply.
  • No no-auto-approve team match (row 7 does not fire).

None of rows 1–8 fire, so the outcome is row 9: approve.

@bokelley
bokelley merged commit 82b429a into main Sep 18, 2026
37 checks passed
@bokelley
bokelley deleted the fix/organization-deletion-containment branch September 18, 2026 01:19
bokelley added a commit that referenced this pull request Sep 18, 2026
Organization merge deleted the secondary organization as its final step.
mergeOrganizations removed the local organization row inside its transaction,
committed, and only then attempted workos.organizations.deleteOrganization,
downgrading a provider failure to a warning while still reporting success. The
Addie path attempted the provider delete twice — once inside mergeOrganizations
and again at step 4 of the tool handler — with WorkOS membership creation
interleaved between the local commit and that second attempt. That is the split
provider/local state commit 97652f8 closed for the two DELETE endpoints, so
merge is now unavailable on the same terms and with the same stable shape.

Three layers, each refusing before any merge-specific effect:

- POST /api/admin/cleanup/merge returns a stable 503
  organization_merge_unavailable before body validation and before any
  merge-specific transaction, WorkOS membership write, provider organization
  deletion, Stripe operation, audit row, cache invalidation or notification.
  Administrative gating is deliberately UNCHANGED — requireGlobalAdmin still
  authenticates and authorizes, unlike the contained deletion routes, because
  nothing here needs to run before authentication.
- The Addie merge_organizations tool refuses execution before it reads provider
  memberships and no longer acquires a WorkOS client on that path; the client
  moved into the read-only preview branch.
- mergeOrganizations itself throws OrganizationMergeUnavailableError before
  getPool(), pool.connect() and BEGIN, so an overlooked caller, a new caller or
  a script bypassing both entry points cannot execute the sequence either.

The unsafe body is removed rather than left unreachable behind the guard: that
sequence is the hazard, so re-enabling merge must be a new reviewed
implementation meeting the lifecycle contract in
docs/contributing/organization-deletion-containment.md, not the deletion of a
guard. After this change no production call site of
workos.organizations.deleteOrganization remains in server/src.

A containment that only covers routes and services is one an operator can walk
around, and two checked-in surfaces handed them the prohibited operation:

- scripts/incidents/2026-05-cleanup-duplicate-prospect-stubs.ts deleted
  organization rows with direct SQL, reasoning from the absence of an admin
  delete endpoint that a script was the sanctioned route. Its destructive path
  is removed, not gated: no database client, no writes, and --execute
  hard-refuses above every other statement. Read-only analysis is preserved.
- The unique-org-per-email-domain invariant emitted a ready-to-paste raw delete
  with the organization ID pre-filled, and pointed at a merge endpoint that does
  not exist. Its remediation_hint now routes to read-only preview-merge
  inspection plus escalation and names both containment codes.

server/tests/unit/organization-delete-statement-guard.test.ts is the static
guard: it scans the JavaScript, TypeScript and SQL files under server/src,
server/scripts and scripts for a raw organization delete and fails on anything
outside an allowlist recording why each remaining occurrence is not
operator-reachable. SQL is covered because a .sql runbook is the likeliest shape
for the next bypass. server/scripts/setup-sandbox.ts is documented and
allowlisted rather than changed: it is sk_test-gated and ID-bounded.

previewMerge and GET /api/admin/cleanup/preview-merge are untouched and remain
read-only (SELECT/COUNT only). The WorkOS preview section describes provider
effects conditionally and the tool footer states that execution refuses;
the existing Stripe and data-movement warnings remain unchanged.

An existing assertion in server/tests/unit/integrity/invariants.test.ts pinned
the raw delete this change removes; it now pins the inverse contract. The three
org-merge suites keep their fixtures and prove the contained service performs no
work on them, and a pre-existing identities cleanup leak in the
primary-organization suite is fixed deterministically by id.

No migration, lifecycle journal, compensation, partial merge or rollback fiction
is introduced. The merge_organizations tool's declared description and usage
hints are intentionally left alone because the Addie tool-surface budget pins a
reviewed profile-contract hash across every declared tool profile; that
re-blessing is recorded as its own follow-up.

Reconstructed directly over landed #7560 at 82b429a.
Preserve main's authorization correction and SEO metadata. Bump Addie
CODE_VERSION to 2026.09.60 as required for this tool implementation change.
Correct the containment document to acknowledge the identity consolidation
refusal already on main, and narrow its conditional-wording claim to the
WorkOS preview section. Original rationale and co-author attribution are
retained; the original public commit and description remain archived.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request Sep 18, 2026
… (#7561)

Organization merge deleted the secondary organization as its final step.
mergeOrganizations removed the local organization row inside its transaction,
committed, and only then attempted workos.organizations.deleteOrganization,
downgrading a provider failure to a warning while still reporting success. The
Addie path attempted the provider delete twice — once inside mergeOrganizations
and again at step 4 of the tool handler — with WorkOS membership creation
interleaved between the local commit and that second attempt. That is the split
provider/local state commit 97652f8 closed for the two DELETE endpoints, so
merge is now unavailable on the same terms and with the same stable shape.

Three layers, each refusing before any merge-specific effect:

- POST /api/admin/cleanup/merge returns a stable 503
  organization_merge_unavailable before body validation and before any
  merge-specific transaction, WorkOS membership write, provider organization
  deletion, Stripe operation, audit row, cache invalidation or notification.
  Administrative gating is deliberately UNCHANGED — requireGlobalAdmin still
  authenticates and authorizes, unlike the contained deletion routes, because
  nothing here needs to run before authentication.
- The Addie merge_organizations tool refuses execution before it reads provider
  memberships and no longer acquires a WorkOS client on that path; the client
  moved into the read-only preview branch.
- mergeOrganizations itself throws OrganizationMergeUnavailableError before
  getPool(), pool.connect() and BEGIN, so an overlooked caller, a new caller or
  a script bypassing both entry points cannot execute the sequence either.

The unsafe body is removed rather than left unreachable behind the guard: that
sequence is the hazard, so re-enabling merge must be a new reviewed
implementation meeting the lifecycle contract in
docs/contributing/organization-deletion-containment.md, not the deletion of a
guard. After this change no production call site of
workos.organizations.deleteOrganization remains in server/src.

A containment that only covers routes and services is one an operator can walk
around, and two checked-in surfaces handed them the prohibited operation:

- scripts/incidents/2026-05-cleanup-duplicate-prospect-stubs.ts deleted
  organization rows with direct SQL, reasoning from the absence of an admin
  delete endpoint that a script was the sanctioned route. Its destructive path
  is removed, not gated: no database client, no writes, and --execute
  hard-refuses above every other statement. Read-only analysis is preserved.
- The unique-org-per-email-domain invariant emitted a ready-to-paste raw delete
  with the organization ID pre-filled, and pointed at a merge endpoint that does
  not exist. Its remediation_hint now routes to read-only preview-merge
  inspection plus escalation and names both containment codes.

server/tests/unit/organization-delete-statement-guard.test.ts is the static
guard: it scans the JavaScript, TypeScript and SQL files under server/src,
server/scripts and scripts for a raw organization delete and fails on anything
outside an allowlist recording why each remaining occurrence is not
operator-reachable. SQL is covered because a .sql runbook is the likeliest shape
for the next bypass. server/scripts/setup-sandbox.ts is documented and
allowlisted rather than changed: it is sk_test-gated and ID-bounded.

previewMerge and GET /api/admin/cleanup/preview-merge are untouched and remain
read-only (SELECT/COUNT only). The WorkOS preview section describes provider
effects conditionally and the tool footer states that execution refuses;
the existing Stripe and data-movement warnings remain unchanged.

An existing assertion in server/tests/unit/integrity/invariants.test.ts pinned
the raw delete this change removes; it now pins the inverse contract. The three
org-merge suites keep their fixtures and prove the contained service performs no
work on them, and a pre-existing identities cleanup leak in the
primary-organization suite is fixed deterministically by id.

No migration, lifecycle journal, compensation, partial merge or rollback fiction
is introduced. The merge_organizations tool's declared description and usage
hints are intentionally left alone because the Addie tool-surface budget pins a
reviewed profile-contract hash across every declared tool profile; that
re-blessing is recorded as its own follow-up.

Reconstructed directly over landed #7560 at 82b429a.
Preserve main's authorization correction and SEO metadata. Bump Addie
CODE_VERSION to 2026.09.60 as required for this tool implementation change.
Correct the containment document to acknowledge the identity consolidation
refusal already on main, and narrow its conditional-wording claim to the
WorkOS preview section. Original rationale and co-author attribution are
retained; the original public commit and description remain archived.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant