fix(infra/ec2): validate POST /dbs body name against path traversal#106
Merged
Conversation
The router.param('name') allowlist guard only fires for the path :name
parameter, so it covers GET/POST/DELETE /dbs/:name* routes but NOT the
create endpoint, which reads name from req.body. That name flows unchecked
into resolve(projects_dir, name), resolve(data_dir, name), sync_seeds ->
resolve(SEEDS_BASE, name), plus mkdirSync/writeFileSync — a path-traversal
write primitive (e.g. {"name":"../../../tmp/x"}) on an unauthenticated,
network-isolated db-host API.
Apply the same VALID_NAME (/^[a-zA-Z0-9_-]+$/) check to the body name in
POST /dbs, reusing the param middleware's error message. No shell injection
path (all spawnSync use array args, no shell).
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.
What
infra/src/ec2/ec2-router.jsvalidates the:namepath parameter on all/dbs/:name*routes via a globalrouter.param('name')allowlist (/^[a-zA-Z0-9_-]+$/). ButPOST /dbs(create) readsnamefromreq.body, which that middleware never sees. The bodynamethen flows unchecked into:resolve(projects_dir, name)→mkdirSync(project_dir)+writeFileSync(resolve(project_dir, 'docker-compose.yml'))resolve(data_dir, name)(mount path)sync_seeds(name)→resolve(SEEDS_BASE, name)+mkdirSyncA request like
{"name":"../../../../tmp/x","engine":"postgres"}is an arbitrary-directory-create + file-write primitive outsideprojects_dir/data_dir. The router is mounted with no auth (server.js—app.use(create_ec2_router(...))), so it relies on network isolation alone; per our security-review guidance, internal-only isn't a reason to skip input validation (SSRF/IDOR targets).This was surfaced by an automated security review that flagged the
GET /dbs/:nameline; that specific line is actually already protected byrouter.param. The real, unprotected sink is the bodynameinPOST /dbs— fixed here.Fix
Apply the same
VALID_NAMEcheck to the bodynameinPOST /dbs, reusing the param middleware's exact error message. One guard, no behavior change for valid names.No command-injection vector exists: every
spawnSyncuses array args (no shell).Verification
node --checkclean.VALID_NAMErejects../../../../tmp/evil,/etc/passwd,foo/../bar,a/b,..,.,foo bar,foo;rm -rfand acceptsmydb,saga-api,test_db,DB123.Note: the ec2-router has no existing unit-test harness (no test file references it; it'd need an express/supertest scaffold). Happy to add one if desired.