Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75d08ecf23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| func protectedRuntimeHostTreeV1(hostPath string) (string, error) { | ||
| resolved, err := filepath.EvalSymlinks(hostPath) |
There was a problem hiding this comment.
When another process can rename or retarget a configured source symlink, it can change the link from an ordinary path to /, /proc, /dev, or /sys after EvalSymlinks returns but before Docker resolves the original source.HostPath. The validation therefore approves one object while command_execution.go later passes a different, mutable pathname to Docker, bypassing the protection this change adds; preserve and launch from a stable resolved object or otherwise prevent source replacement through container creation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR hardens runtime host bind validation by rejecting host sources that resolve to the host filesystem root or protected kernel system trees/filesystems, including symlink and filesystem-identity aliases, and updates the design docs/backlog plus adds a security changelog fragment.
Changes:
- Add host bind preflight rejection for filesystem-root binds and protected system trees (
/proc,/dev,/sys) including symlink aliases. - Add platform-specific filesystem identity detection (Linux/macOS) to reject protected kernel filesystems even when mounted elsewhere.
- Update controlled-session design documentation/backlog and add a Security changelog fragment.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/dockerdeploy/runtime_host_preflight.go | Adds protected-host-tree/filesystem detection to runtime host source validation. |
| internal/dockerdeploy/runtime_host_preflight_test.go | Adds cross-platform tests for rejecting protected host trees and aliases. |
| internal/dockerdeploy/runtime_host_preflight_unix_test.go | Adds Unix-only test ensuring explicit directory binds with nested special objects remain allowed. |
| internal/dockerdeploy/runtime_host_filesystem_linux.go | Implements Linux filesystem identity checks (statfs + mountinfo/statx) for protected kernel filesystems. |
| internal/dockerdeploy/runtime_host_filesystem_linux_test.go | Adds Linux tests for kernel filesystem detection and tmpfs/devtmpfs distinction behavior. |
| internal/dockerdeploy/runtime_host_filesystem_darwin.go | Implements macOS filesystem identity checks for devfs/procfs. |
| internal/dockerdeploy/runtime_host_filesystem_other.go | Provides no-op implementation for non-Linux/non-macOS platforms. |
| docs/CONTROLLED_SESSION_DESIGN.md | Documents narrowed direct-bind security contract and updated root-runtime messaging. |
| docs/BACKLOG.md | Updates backlog items reflecting completed/shifted security work and future network gateway work. |
| .changes/unreleased/+protect-host-system-trees.yaml | Adds a Security changelog fragment describing the new bind rejections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return fmt.Errorf( | ||
| "runtime plan %q mount %q host source resolves to protected host system source %q; ordinary host binds cannot expose the host filesystem root, /proc, /dev, or /sys", | ||
| planID, mount.Destination, protected, | ||
| ) |
690e66b to
bbcaeea
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 277861570e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return "", fmt.Errorf("resolve canonical path: %w", err) | ||
| } | ||
| resolved = filepath.Clean(resolved) | ||
| filesystem, err := protectedRuntimeHostFilesystemV1(resolved) |
There was a problem hiding this comment.
When DOCKER_HOST or the active Docker context targets a remote Linux daemon, this validates the source in Reploy's local filesystem namespace, while compose.go:72-76 inherits that Docker configuration and command_execution.go:243 sends the same pathname to the daemon. Docker documents that bind mounts are created on the daemon host, not the client, so a path that is ordinary locally but maps to /, /proc, /dev, or /sys remotely bypasses this protection. Fresh evidence beyond the earlier symlink-retargeting report is that the two processes can resolve permanently different objects without any race; reject remote-daemon host binds or perform validation in the daemon's namespace. See Docker's bind-mount constraints.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2083678b49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| switch strings.ToLower(scheme) { | ||
| case "unix", "npipe": | ||
| return true |
There was a problem hiding this comment.
When Reploy runs in a container with the host Docker socket mounted, or connects through a Unix-socket proxy, this branch accepts the endpoint even though the daemon can use a different mount namespace or machine. Docker resolves bind sources in the daemon namespace while ValidateRuntimeHostSourcesV1 inspected Reploy's namespace, so a path that is benign locally can resolve to /, /proc, /dev, or /sys for the daemon and bypass the new guard. Fresh evidence beyond the earlier remote-endpoint finding is that the newly added check classifies every Unix socket as local; require proof that the daemon shares the validated namespace or reject configurations where that cannot be established.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e10d22577
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if length == bestLength && identity != best { | ||
| return runtimeHostMountIdentityV1{}, fmt.Errorf("host path %q has ambiguous mountinfo identity", path) | ||
| } |
There was a problem hiding this comment.
On Linux kernels that do not expose STATX_MNT_ID, a valid overmount leaves multiple mountinfo records with the same mount point, and this fallback treats those records as ambiguous instead of selecting the currently visible topmost mount. Consequently, any ordinary host bind at or below that mount point fails validation on older kernels. Unlike the earlier missing-mount-ID issue, the new path-based fallback now reaches this error; resolve the visible mount through the mount topology rather than rejecting equal-length matches.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 617a789293
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if defaultDockerEndpointCheckV1.checked { | ||
| return nil |
There was a problem hiding this comment.
When the active Docker context changes after the first successful executeDockerOutput call—for example, another command runs docker context use remote while the Reploy process remains active—this process-wide checked flag skips all subsequent endpoint inspection even though each Docker CLI invocation rereads the active context. Later image probes, builds, or cleanup commands can therefore run against the newly selected remote daemon despite the new remote-endpoint prohibition; either pin the verified endpoint for every command or revalidate it before execution.
Useful? React with 👍 / 👎.
| if !pathWithinV1(identity.mountPoint, protectedTree) || identity.device != candidate.device { | ||
| continue | ||
| } | ||
| if pathWithinV1(effective, identity.root) || pathWithinV1(identity.root, effective) { | ||
| return true, nil |
There was a problem hiding this comment.
When a bind below /proc, /dev, or /sys is subsequently hidden by an overmount at the same location, /proc/self/mountinfo retains both records, but this loop treats the hidden record as still exposing its backing path through the protected tree. For example, if /secret was bound at /dev/x and /dev/x is then overmounted with another filesystem, an ordinary source of /secret is still rejected even though the only visible /dev/x mount is unrelated; filter these records through the visible-mount topology before testing backing-path overlap.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b94f4d1ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := requireLocalDockerEndpointV1(ctx, spec, timeout); err != nil { | ||
| return err |
There was a problem hiding this comment.
When the installed system service starts, its actual compose up is passed to runCommandWithoutDockerPreflight from installed_service_container.go:101-108, so this endpoint check is never applied to the container-creation command. If the active context changes after the earlier image-reference probe, that command can target a remote daemon after host sources were validated locally, recreating the namespace mismatch this restriction is intended to prevent. Fresh evidence beyond the earlier cached-check finding is that removing the cache still leaves this independent command path bypassing endpoint validation entirely.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bff9ea2245
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| environment, | ||
| options, | ||
| runCommandWithoutDockerPreflight, | ||
| runCommand, |
There was a problem hiding this comment.
When the installed system service starts with a remote active Docker context, this replacement still bypasses endpoint validation: RunInstalledServiceContainerV1 requires dockerPath to be absolute, while runCommand invokes dockerPreflight only when spec.Name == "docker". The compose up therefore runs against the remote daemon after host sources were validated locally. Fresh evidence beyond the earlier bypass comment is that switching the runner to runCommand does not activate its preflight for the absolute executable path used here; either recognize the configured Docker executable or call endpoint validation explicitly.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9249110975
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if kind := protectedRuntimeHostFilesystemKindV1(filesystemMagic); kind != "" { | ||
| return kind, nil | ||
| } | ||
| protectedMount, err := runtimeHostSharesProtectedMountV1(path) |
There was a problem hiding this comment.
When an otherwise ordinary source such as /safe contains a visible bind mount like /safe/root -> / or /safe/proc -> /proc, this check examines only /safe itself and runtimeHostSharesProtectedMountV1 only correlates mounts located beneath /proc, /dev, or /sys. The runtime renderers do not disable recursive bind behavior (execution_render.go:130-142 and command_execution.go:240-252), and Docker documents that bind mounts are recursive by default, so the protected nested mount is included in the container. Fresh evidence beyond the earlier exact-alias findings is that selecting the alias's parent still exposes the entire protected tree; inspect visible submounts below the selected source and reject any whose effective backing path exposes root or a protected tree.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
reploy/internal/dockerdeploy/compose.go
Lines 64 to 66 in 4fc7f60
Route provider-install starts through endpoint preflight
For Docker-managed and Docker Desktop installs, provider_install_host_commands.go:38-49 builds an independent compose up command using the absolute Docker path, but provider_install_host_execute.go:41-54 still invokes it through this bypass runner. If the active context is remote, or changes after an earlier build probe, installation therefore creates the workload on that daemon while its host paths were validated locally, contrary to the new remote-endpoint restriction in docs/CONTROLLED_SESSION_DESIGN.md:691-698. Fresh evidence beyond the earlier installed-service comments is that this separate provider-install startup path remains unchecked; pass it through runDockerCommand.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return false, err | ||
| } | ||
| current := string(filepath.Separator) | ||
| for _, component := range strings.Split(strings.TrimPrefix(filepath.Clean(absolute), current), current) { |
There was a problem hiding this comment.
On Linux kernels without openat2, a source such as /safe/jump/.. where jump -> /proc/self/cwd is cleaned to /safe here before Lstat, so the fallback never notices the symlink. EvalSymlinks can then resolve the magic link relative to Reploy's benign working directory, while Docker receives the original path and can resolve it relative to the daemon (commonly exposing /), bypassing the protected-tree check and the documented fail-closed behavior for symlinked sources on older kernels. Walk the original components without lexically eliminating .., or reject such components in this fallback.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fc7f6076f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if dev.Dev == root.Dev { | ||
| return false, nil |
There was a problem hiding this comment.
When /dev is a static directory on the root filesystem and is bind-mounted at an alias such as /safe/dev, this early return disables the device-filesystem comparison. EvalSymlinks cannot collapse the bind alias, Statfs reports the ordinary root filesystem, and mountinfo contains no canonical /dev mount identity to match, so validation accepts the alias and Docker exposes the host device nodes. Fresh evidence beyond the earlier tmpfs-backed /dev report is that the new same-device branch explicitly opts out on rootfs-backed /dev; compare the candidate's effective backing path with /dev even in this configuration.
Useful? React with 👍 / 👎.
Reject runtime host sources that resolve to the filesystem root or protected kernel system trees. Use platform-specific filesystem and mount topology identity to catch direct, aliased, and nested kernel-control filesystems while preserving explicit ordinary directory binds with nested application objects. Reject unsupported remote Docker endpoints before runtime and recovered-container operations, including admission cleanup paths. Document the narrowed direct-bind security contract, close the corresponding backlog item, and add a security changelog fragment.
|
@codex review Please review the exact current head |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Reject runtime host sources that resolve to the filesystem root or protected kernel system trees. Use platform-specific filesystem identity to catch aliases while preserving explicit ordinary directory binds with nested special objects.
Document the narrowed direct-bind security contract, close the corresponding backlog item, and add a security changelog fragment.
Stack created with Sapling. Best reviewed with ReviewStack.