fix(security): complete organisation-name uniqueness coverage - #160
Merged
Merged
Conversation
Finding 003a9234: createOrganization's duplicate-name protection (PR 159) is solely an atomic conditional put of a NAME# reservation row. Nothing created reservation rows for organisations that already existed, so a duplicate of any such name (including the four seeded orgs) passed both the tombstone lookup and the condition. Three mechanisms, per the security-architect's design: 1. seed-organizations now writes a NAME# reservation row for every organisation it seeds, derived from the same ORGANIZATIONS list that seeds the org row itself, so the two cannot drift. Reservation puts are conditional (attribute_not_exists), so re-running the custom resource is idempotent. 2. New backfill script (scripts/backfill-org-name-reservations.ts) for organisations that already exist. Idempotent, dry-run by default. A name held by 2+ existing orgs is a pre-existing tenancy collision (decision 228b3cc8) and is reported loudly (non-zero exit code), never auto-resolved. Chose the standalone-script shape (matching backfill-org-ids.ts) over a custom-resource/Lambda (matching backfill-project-org.ts): no CDK wiring or IAM role is needed, and the blast radius is a single table/purpose, same profile as backfill-org-ids.ts. 3. createOrganization gets a row-level Scan for an existing org with the same name, filtered to exclude NAME# reservation/tombstone rows. This is a defence-in-depth backstop against reservation-table drift -- the conditional put remains the atomic uniqueness authority; the Scan is not race-free and does not replace it. Adds the seeded-name duplicate regression test (rejects a duplicate of a live org row that has no backing reservation row -- the exact gap that let PR 159's defect pass review), plus idempotency and collision-reporting tests for the backfill.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 159 replaced
createOrganization's scan-then-write duplicate check with an atomic conditional put on aNAME# <name>reservation row. That closed the race but opened a hole: nothing creates reservation rows for organisations that already exist, so a duplicate of any such name passes both the tombstone lookup and the 'attribute_not_exists' condition.We had traded a check that was complete but non-atomic for one that was atomic but incomplete.
This is not an upgrade-path problem:
seed-organizationswrites only the four organisation rows and no reservation rows, so even a fresh deployment's seeded names were duplicable. Only names created through 'createOrganization after deploy were protected.The organisation name is the canonical tenancy key, two organisations sharing a name means two tenants sharing one key - the non-admin users and name-stamped rows of each become visible to the other.
createOrganizationis admin-only, so the realistic risk is an accidental duplicate silently merging two tenants' visibility rather than escalation.Three mechanisms, because any one alone leaves a hole
NAME#reservation row for every organisation it seeds, derived from the same constant as the organisation row. Proven undriftable: changing the seeded name in one place fails a test rather than silently divergingbackfill-org-ids.tsover a custom resource, since it needs no CDK wiring or IAM role. A re-run writes nothing new. A name held by two or more organisations is reported as a collision with a non-zero exit, never auto-resolved - that's a pre-existing tenancy collision and an owner decisioncreateOrganizationregains an existence check against real organisation rows. The conditional put remains the sole atomic authority; comments state explicitly which is primary and which is defence in depth, so the guarantee no longer rests solely on a side table that can driftThe test that would have caught the original gap
A duplicate of a seeded name is rejected - both with its reservation row present, and with it absent, where the backstop must catch it. The existing test only proved a duplicate of a name created within the same test was rejected, which is why the gap passed review.
Plus backfill idempotency, and a test asserting the backfill reports a pre-existing duplicate rather than collapsing it.
Testing
tsc 0, lint 0, targeted 46, all 16 guard suites (363), full backend jest 7862, nag-enabled synth of all 9 stacks clean.
No IAM changes, so 'splitigates" is out of scope. Tombstones verified intact, and the
backfill will not resurrect a tombstoned name.
After merging
Existing deployments need
scripts/backfill-org-name-reservations.tsrun to gain reservation rows. Until then the row-level backstop is what protects them.