fix(git): reject SSH-URL-shaped codebase names in worktree path resolution#1583
fix(git): reject SSH-URL-shaped codebase names in worktree path resolution#1583blankse wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces manual codebaseName splitting with strict ChangesOwner/Repo Validation Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
55663ca to
2a87f72
Compare
…ution Closes coleam00#1582. Background `resolveOwnerRepo()` (used by `getWorktreeBase()`) split the codebase `name` field at the first `/` and accepted both halves verbatim as owner/repo. A codebase registered with an SSH-style remote URL as its name (e.g. `git@host:org/repo`) therefore produced `{ owner: "git@host:org", repo: "repo" }` and worktrees ended up under ~/.archon/workspaces/git@host:org/repo/worktrees/... The colon in that path silently breaks anything that uses `:` as a separator. Most visible symptom: docker-compose short-form volume specs in devcontainer setups. A spec like `./postgres-init:/docker-entrypoint-initdb.d:ro` is parsed as `HOST:CONTAINER:OPT`, and when HOST itself already contains a colon the whole spec gets rejected with `invalid volume specification`. The failure was hard to spot because the bash-node executor head-truncates captured error output, dropping exactly the line with the root cause. The same workspace concept reached via the CLI path-derived heuristic correctly produced `<parent>/<repo>` (no colon, no `@`), so CLI runs worked while WebUI runs (which pass `codebaseName` through) failed deterministically for any codebase whose name was an SSH URL. Change Use the existing strict `parseOwnerRepo()` validator from `@archon/paths` (which already enforces `[A-Za-z0-9._-]` per segment for path-traversal safety) for the codebase-name branch. When the name doesn't parse, log `worktree.invalid_codebase_name_format` and fall through to the path-derived heuristic — same fallback as the existing `'invalid-no-slash'` case, just covering one more shape. Behaviour delta is strictly safer: - Valid `owner/repo` codebase names — unchanged. - Invalid names (no slash, multiple slashes, dot-segments) — unchanged. - SSH-URL-shaped names with `:`/`@` — previously produced a corrupted workspace path; now logged and falls back to the path-derived layout. Tests New regression test in `git.test.ts` covering the SSH-URL shape; asserts the resulting base contains neither `:` nor `@`. The test mock for `@archon/paths` now also exposes `parseOwnerRepo` (mirrored SAFE_NAME, kept aligned with the real implementation). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2a87f72 to
b1d5b28
Compare
Summary
name(e.g.git@host.example:org/repo) produces worktree paths like~/.archon/workspaces/git@host.example:org/repo/worktrees/.... The:in that path silently corrupts any downstream tool that uses:as a separator — most visibly docker-compose short-form volume specs (HOST:CONTAINER:OPT) inside devcontainers, which then fail withinvalid volume specification.codebaseNamethrough), so users see the same workflow succeed via CLI and fail via Web UI for no obvious reason. The bash-node executor head-truncates failure output, so the actualinvalid volume specificationline gets dropped — diagnosis is painful.resolveOwnerRepo()now routes thecodebaseNamebranch through the existing strictparseOwnerRepo()validator from@archon/paths(SAFE_NAME = /^[a-zA-Z0-9._-]+$/). Invalid names logworktree.invalid_codebase_name_formatand fall through to the path-derived heuristic — same fallback that already covered the'invalid-no-slash'case.owner/repocodebase names behave identically. The path-derived fallback (extractOwnerRepo(repoPath)) is unchanged. No public API or schema change. No change to codebase registration / persistence — that normalization is a separate, larger discussion (called out at the end of bug(git): SSH-URL codebase names produce worktree paths with ':' that break docker volume parsing #1582).UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
worktree.ts@archon/pathsparseOwnerRepoto existing importresolveOwnerRepoparseOwnerReposplit('/')resolveOwnerRepogetWorktreeBasecallersresolveOwnerRepoLabel Snapshot
risk: lowsize: Sgitgit:worktreeChange Metadata
buggitLinked Issue
Validation Evidence (required)
'ignores SSH-URL-shaped codebaseName ... and falls back to path'inpackages/git/src/git.test.tsexercises the regression directly and asserts the resulting worktree base contains neither:nor@. Mirrors the structure of the existing'invalid-no-slash'test. The test mock for@archon/pathsnow also exposes aparseOwnerRepomirror kept aligned with the realSAFE_NAMEregex.bun run lintwas skipped because root-level ESLint OOMs on this machine (V8 heap exhaustion in the monorepo lint run — independent of this PR). Linted the two touched files directly instead.Security Impact (required)
getProjectWorktreesPath()output, only the input owner/repo segments are now constrained to[A-Za-z0-9._-]. If anything, this narrows the set of paths the resolver can produce (path-traversal-safe by construction, sinceSAFE_NAMEalready excludes./..).Compatibility / Migration
owner/reponames see no change.git@host:org/repo/...path: those keep working (git doesn't care), but new worktrees for the same codebase will land under the path-derived layout instead. Old worktree dirs can be cleaned up manually if desired.Human Verification (required)
namecontained an SSH URL — failure mode (invalid volume specificationfrom docker-compose) confirmed againstdev.resolveOwnerRepo("git@host.example:org/repo")returns the path-derived owner/repo, and the resulting worktree base contains no:/@.@archon/gittests still pass; new regression test passes.codebaseNamealready valid (e.g.acme/widget-app) → unchanged.codebaseNamewith multiple slashes (e.g.acme/sub/repo) → unchanged (still rejected byparseOwnerRepolength check, falls through).codebaseNamewith leading/trailing dots → unchanged (rejected bySAFE_NAME, falls through).codebaseNamewith whitespace,+,~, etc. → now also rejected (was previously accepted as a path segment); this is the intended tightening.namein the first place) — out of scope; called out as a follow-up in bug(git): SSH-URL codebase names produce worktree paths with ':' that break docker volume parsing #1582.Side Effects / Blast Radius (required)
getWorktreeBase()resolution (and any code that depends on the worktree base layout —@archon/isolation, workflow run dispatch, artifact paths derived from the worktree). All consumers ride the same path-derived fallback the CLI already used, so behaviour for new runs converges with the previously-working CLI behaviour.namehappened to contain non-alphanumeric chars but wasn't an SSH URL and where the corrupted path was somehow load-bearing — they'll now silently get the path-derived layout instead. Mitigated by the warn-log (worktree.invalid_codebase_name_format) and by the fact that the corrupted layout was never functional (it's the bug being fixed).'invalid-no-slash'remains; new test pins the SSH-URL shape; warn-level log event makes the fallback observable in operations.Rollback Plan (required)
invalid volume specificationin docker-compose volume parsing (or any other consumer that splits on:). Logged eventworktree.invalid_codebase_name_formatlets operators see when the fallback fires.Risks and Mitigations
'invalid-no-slash'precedent two lines above). Erroring out at the resolver level would break legitimate CLI runs that pass any unparseable name; a separate registration-time validator (mentioned in bug(git): SSH-URL codebase names produce worktree paths with ':' that break docker volume parsing #1582) is the right place to enforce this strictly.Summary by CodeRabbit
Bug Fixes
Tests