test(sandbox-proxy): cover branch-name validation trust boundary - #5350
Open
pedrofrxncx wants to merge 1 commit into
Open
test(sandbox-proxy): cover branch-name validation trust boundary#5350pedrofrxncx wants to merge 1 commit into
pedrofrxncx wants to merge 1 commit into
Conversation
assertSandboxBranchParam parses the untrusted URL branch param before it composes the sandbox claim ref/handle and is forwarded to the daemon, which uses it as a live git ref (checkout/fetch). No test anywhere in the repo exercised it, so a regression here (e.g. a loosened regex letting ".."- or ".lock"-suffixed input through) would only surface as a downstream daemon error rather than a caught local failure.
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.
Trust-boundary test for
assertSandboxBranchParaminapps/api/src/api/routes/sandbox-proxy.ts.Why this is a trust boundary (C4/R4):
assertSandboxBranchParamparses thebranchURL path param — fully client-controlled — before it's used to compose the sandbox claim ref (composeSandboxRef) and claim handle (computeClaimHandle), which are ultimately forwarded to the sandbox daemon wherebranchis used as a live git ref for checkout/fetch (seepackages/sandbox/daemon/git/ref-name.ts'sisValidRemoteBranchName, which encodes the same git-ref-injection rules: no.., no leading/trailing/, no.locksuffix). This is exactly "a URL used for auth or a network call" / "user-supplied paths feeding a filesystem/proxy op" per the gate. I confirmed via repo-wide grep that no test anywhere (inapps/apiorpackages/sandbox) previously exercised this specific function — a regression here (e.g. a loosened regex, or a brokenthread:prefix strip letting..through) would only surface later as an opaque daemon-side git error, not a caught local failure.Change: exported
assertSandboxBranchParam(previously module-private) and added adescribe("assertSandboxBranchParam", ...)block in the existing test file covering: valid plain/thread:-prefixed branches, empty branch, emptythread:id,..traversal (both plain and inside athread:id), leading/trailing slash,.locksuffix, >255 chars, and disallowed charset (space,:, leading-). No production logic changed — just anexportkeyword plus the new tests.Reviewer check:
bun test apps/api/src/api/routes/sandbox-proxy.test.ts(19 tests, all pass).Locally verified:
bun run fmt,cd apps/api && bunx tsc --noEmit(clean), and the targeted test file above. Full CI covers the rest.Summary by cubic
Add trust-boundary tests for
assertSandboxBranchParamin the sandbox proxy to catch unsafe branch inputs before they reach the daemon. Exported the function for testing; covers valid plain andthread:branches and rejects empty,..traversal, leading/trailing/,.locksuffix, >255 chars, and invalid charset.Written for commit 05386fe. Summary will update on new commits.