fix(control-plane): add a collision-resistant suffix to Neon branch names that would otherwise truncate - #8134
Merged
Conversation
…ames that would otherwise truncate branchNameFor's unconditional .slice(0, 63) had no collision guard -- two tenant names sharing the same first ~52 characters (after the "tenant-<product>-" prefix and sanitization) truncated to the identical Neon branch name. provisionNeonDatabase's findBranchByName would then resolve the OTHER tenant's already-existing branch and hand back its connection/role/password to the new tenant, a cross-tenant isolation bug. Only names that actually need truncating get a hash-of-the-untruncated- name suffix; a short tenant name's branch name is byte-identical to before. This repo has never provisioned against a live Neon project (no live credentials anywhere in this codebase, per this file's own header comment), so there is no pre-existing long-name branch a suffix could orphan. Closes #8026.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8134 +/- ##
==========================================
- Coverage 92.02% 91.49% -0.54%
==========================================
Files 760 760
Lines 77394 77410 +16
Branches 23392 23395 +3
==========================================
- Hits 71225 70826 -399
- Misses 5061 5523 +462
+ Partials 1108 1061 -47
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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
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
branchNameFor(control-plane/src/neon-database-driver.ts) sanitizes a tenant/product-derived name, then unconditionally.slice(0, 63)s it (Neon's branch-name length limit) with no check that the truncated result stays unique.tenant-<product>-prefix and sanitization) truncated to the identical Neon branch name.provisionNeonDatabase'sfindBranchByNamewould then resolve the OTHER tenant's already-existing branch and hand back its connection/role/password to the new tenant — a cross-tenant data-isolation bug.[a-z0-9_-]character set. A short, unambiguous tenant name's branch name is completely unchanged — this only engages when truncation would otherwise happen.Note on the issue's live-infra verification deliverable: #8026 explicitly calls for validating this scheme against Neon's real API constraints before merging. This repo has no live Neon credentials anywhere (confirmed via the file's own header comment: "the test suite mocks every call; no live Neon credentials are used anywhere in this repo") and
control-plane/has never been deployed against a live Neon project (provisionNeonDatabaseisn't wired into a deployable service yet — see #7654). I can't perform that live-account verification from this environment. The chosen suffix scheme stays entirely within the length/character constraints this module already enforces ([a-z0-9_-], ≤63 chars), which are conservative relative to Neon's actual documented limits, but a maintainer should do a final live-account sanity check before or shortly after the first real deploy, same as the file's pre-existing "verify against a live account before the first real deploy" disclaimer already calls for independent of this fix.Since this repo has zero pre-existing tenants provisioned against a real Neon project, there's no already-provisioned long-name branch this change could orphan by changing its derived name.
Closes #8026.
Test plan
control-plane/test/neon-database-driver.test.ts: two long, prefix-similar tenant names (identical for the first 60 characters) now produce differentprovisionNeonDatabasebranch-create POST bodies; a short tenant name's branch name is unaffected.npm run build(control-plane workspace)npm run test(control-plane workspace, 182/182 passing,node --testrunner)npx tsc --noEmit(repo root)npm run engine-parity:drift-check— clean, not an engine twin-pair file