fix(server): make session WorkingDir containment opt-in - #3787
Closed
trungutt wants to merge 2 commits into
Closed
Conversation
The containment guard added in docker#3758 fell back to os.Getwd() when runConfig.WorkingDir was unset, which confined session working dirs to whatever directory the process happened to be launched from. Callers that use the API as a long-lived daemon and legitimately point sessions at arbitrary paths on the user's filesystem then hit 'outside the permitted root' for every session. Make containment opt-in: only enforce it when --working-dir is explicitly set. When it isn't, resolveWithinRoot still canonicalises via EvalSymlinks (so the CodeQL data-flow is broken) but does not constrain the path. Deployments that need containment can opt in by passing --working-dir.
trungutt
marked this pull request as ready for review
July 22, 2026 14:10
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The PR correctly implements opt-in containment: workingDirRoot() now returns "" when --working-dir is unset, and resolveWithinRoot skips the boundary check in that case while still running filepath.EvalSymlinks to canonicalise the path. The intent is clearly documented in both the PR description and the new function comment. Test coverage for the new opt-in behaviour is thorough (nil config, empty config, whitespace-only config all tested).
Dismissed security findings (review manually)
pkg/server/session_manager.go:601 — Path containment disabled by default when--working-diris unset: arbitrary working directories accepted without restriction (verifier mitigation: intentional design decision — the PR explicitly documents that--working-dirwas never meant to be a sandbox root; the previousos.Getwd()fallback was incorrect overloading of the flag's purpose; deployments that need containment must opt in by setting--working-dir)
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A --working-dir value that trims to empty (e.g. an unresolved shell variable) previously fell through workingDirRoot's trim and silently disabled containment. The operator expects containment to be active; they get none, invisible in logs. Fail loudly at startup instead.
Contributor
Author
|
Superseded — going with a straight revert of #3758 instead of opt-in containment. See follow-up PR. |
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
2 participants
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.
Why
#3758 added a containment check for user-supplied
working_dironPOST /api/sessions. When no--working-dirwas configured it fellback to
os.Getwd()as the permitted root — i.e. the process's currentdirectory at launch became a hard boundary for every session.
That breaks any deployment where docker-agent runs as a long-lived
daemon and clients legitimately create sessions targeting arbitrary
paths on the host: every session outside the launch directory is
rejected with
--working-dir's documented purpose (cmd/root/flags.go) is a defaultcwd for tool execution — not a sandbox root. Overloading it as one
makes the check unusable for the majority of callers, who don't set it
and don't want containment.
What changes
Make containment opt-in:
resolveWithinRootstill runsfilepath.EvalSymlinkson the candidatein both branches, so the CodeQL data-flow that alert #57 flagged
remains broken. Multi-user / hosted deployments that need containment
opt in by passing
--working-dir.