feat(terminal-demo): contain untrusted repository paths - #581
Draft
ss-o wants to merge 1 commit into
Draft
Conversation
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
internal/repopath: kernel-enforced containment for every untrusted repository pathopenat2usingRESOLVE_BENEATH | RESOLVE_NO_SYMLINKS | RESOLVE_NO_MAGICLINKSplusO_NOFOLLOWon the final componentOpenRoot,Close,OpenRead,OpenDir, andAtomicReplace*repopath.Rootsatisfies the existingmanifest.Readerinterface, so Task 2 orchestration works unchanged against real containmentRefs #458
Scope: Task 4a of an approved split
The approved plan defines Task 4 as one commit covering
repopath,fixture,snapshot,readme, andmain.gowiring. The maintainer approved splitting it so the security-critical containment layer gets its own review gate rather than being reviewed inside a four-package diff.This PR is Task 4a:
internal/repopathonly. Task 4b (fixture staging, snapshot extraction, README contract,__snapshotwiring) follows on merged 4a and keeps the plan's original commit message. Deliverables, contracts, and verification requirements are unchanged; only review granularity differs.Contract boundaries
This package never resolves a path with
filepath.EvalSymlinksfollowed by an ordinary open. That pattern is racy: a component can be replaced between the check and the open. Lexical validation is a fast sanitized rejection for obviously unsafe input and is explicitly documented as not being the security boundary; the kernel is.Every rejection uses the existing sanitized boundary
failure.UnsafePath/failure.StageSource/failure.RuleSourceMutated, public exit code 3. No public class, stage, rule, or exit code was added or renamed. The structured field names a bounded category only and never carries the offending path, which may contain untrusted repository text. A test asserts that no substantial path leaks into eitherError()or the field.OpenReaddeliberately accepts a directory, becausemanifest.Loadopens the declared fixtures directory through this same boundary and inspects its type afterwards.OpenDiris the strict variant.AtomicReplacecreates its staging file withO_EXCLbeneath the contained parent and renames it into place, so a pre-planted symlink at the destination is replaced rather than followed.Verification, including mutation testing
Because this is security code, passing tests were not treated as sufficient. Each guard was removed in turn to confirm a test actually fails:
RESOLVE_BENEATHalone initially kept the suite green. That exposed a real gap: an intermediate symlink whose target stays beneath the root is not caught byRESOLVE_BENEATH, andO_NOFOLLOWonly guards the final component. Three tests were added for that case, and they now fail against the weakened build. The implementation was restored byte-identical afterwards.RESOLVE_NO_MAGICLINKSalone kills no test, because a magic link is only reachable through a symlink thatRESOLVE_NO_SYMLINKSalready rejects. Rather than claim coverage that does not exist, the flag is retained as defense in depth and the test carries a comment recording exactly this, so a later reader does not mistake it for proven coverage.The decisive containment test swaps a real directory component for an escaping symlink after the path has already validated once. A lexical check passes that case; only kernel-enforced resolution rejects it.
Verification
All commands ran through the pinned
scripts/in-go-image.shwrapper (Go 1.26.5,linux/amd64); host Go was never an acceptance path.go test ./internal/repopath ./internal/manifest -count=1: PASSgo test ./... -count=1: PASS, includinginternal/sandboxgo vet ./...: cleangofmt -l .: cleango mod tidy -diff: still reports only the reserved Goldmark pin, which Task 4b consumes ininternal/readmeStatus
No fixture staging, snapshot extraction, README validation,
main.gowiring, image publication, release, pilot rollout, or pattern promotion occurred in this PR.