feat(execd): add isolation package with bwrap support#1008
Conversation
Pangjiping
force-pushed
the
osep-0013-phase1-isolation-core
branch
14 times, most recently
from
45245e8 to
03cbe0b
Compare
Pangjiping
force-pushed
the
osep-0013-phase1-isolation-core
branch
13 times, most recently
from
19a3775 to
42ce1a6
Compare
![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: 5ff9482fe5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| cmd := exec.Command(p, | ||
| "--ro-bind", "/", "/", | ||
| "--proc", "/proc", | ||
| "--", "true", |
There was a problem hiding this comment.
In environments where bwrap can perform the simple bind/proc setup but cannot create the PID/UTS/IPC namespaces, this smoke test still marks isolation as available even though every real session later adds --unshare-pid, --unshare-uts, and --unshare-ipc in buildArgv. That leaves /v1/isolated/capabilities reporting availability and initializes the runner, but session creation then fails per request instead of being disabled up front; the probe should include the same namespace flags used for actual sessions.
Useful? React with 👍 / 👎.
| if err := os.MkdirAll(filepath.Dir(newUpper), 0o755); err != nil { | ||
| return err | ||
| } | ||
| return os.Rename(oldUpper, newUpper) |
There was a problem hiding this comment.
When oldPath exists only in the lower workspace, this path copies it into upper and then renames that upper copy to the destination, but it never creates a whiteout for the original lower path. In overlay sessions, mv lower.txt new.txt through the isolated file API therefore leaves lower.txt visible via Stat/ReadFile because lookups fall back to the lower layer after oldUpper is moved, so the move behaves like a copy.
Useful? React with 👍 / 👎.
| // File exists only in lower — cannot delete without whiteout. | ||
| lowerPath := m.resolveLower(rel) | ||
| if _, err := os.Stat(lowerPath); err == nil { | ||
| return fmt.Errorf("cannot remove file from read-only workspace lower: %s", path) |
There was a problem hiding this comment.
For overlay sessions, deleting a file that exists only in the lower workspace should create a whiteout in the upper layer so the merged view no longer exposes it. This branch instead returns an error for exactly that common case, so DELETE /v1/isolated/session/:id/files?path=... fails with a 500 and the file remains visible whenever the target was part of the original workspace.
Useful? React with 👍 / 👎.
| filePath := c.ctx.Query("path") | ||
| oldStr := c.ctx.Query("old") | ||
| newStr := c.ctx.Query("new") | ||
| if filePath == "" || oldStr == "" { | ||
| c.RespondError(http.StatusBadRequest, model.ErrorCodeMissingQuery, "path and old are required") | ||
| return |
There was a problem hiding this comment.
The OpenAPI contract for isolatedReplaceContent mirrors /files/replace and sends a JSON map of paths to ReplaceFileContentItem, but this handler ignores the body and requires undocumented path, old, and new query parameters. Generated clients that follow specs/execd-api.yaml will post the documented body and consistently receive path and old are required, making the isolated replace endpoint incompatible with the published API.
Useful? React with 👍 / 👎.
| var req model.CreateIsolatedSessionRequest | ||
| if err := c.bindJSON(&req); err != nil { | ||
| c.RespondError(http.StatusBadRequest, model.ErrorCodeInvalidRequest, err.Error()) | ||
| return |
There was a problem hiding this comment.
bindJSON only decodes JSON, so the validate tags and Validate methods on the new isolated request models never run. A create request such as {} reaches the runtime with an empty workspace and returns a 500, and a run request without code executes an empty command successfully despite the OpenAPI required fields; these should be rejected as 400s immediately after decoding.
Useful? React with 👍 / 👎.
| paths := c.ctx.QueryArray("path") | ||
| for _, p := range paths { | ||
| if err := mv.MkdirAll(p, 0o755); err != nil { | ||
| c.RespondError(http.StatusInternalServerError, model.ErrorCodeRuntimeError, err.Error()) | ||
| return | ||
| } | ||
| } | ||
| c.RespondSuccess(nil) |
There was a problem hiding this comment.
Both isolated directory operations are documented with JSON request bodies, but this implementation reads only path query parameters and ignores the body. A generated client that posts the documented map of directories to permissions hits this code with an empty paths slice, receives 200, and no directories are created; the same query/body mismatch affects the delete handler below.
Useful? React with 👍 / 👎.
| paths := c.ctx.QueryArray("path") | ||
| resp := make(map[string]model.FileInfo) |
There was a problem hiding this comment.
isolatedGetFilesInfo is published in specs/execd-api.yaml with a required JSON body containing the paths to stat, but this handler only looks at repeated path query parameters. Clients generated from the new spec will send the body, paths will be empty, and the endpoint returns an empty 200 response instead of the requested metadata.
Useful? React with 👍 / 👎.
| if s.upperID != "" { | ||
| r.upperMgr.Release(s.upperID) | ||
| } |
There was a problem hiding this comment.
Deleting an overlay session only marks its upper entry as not in use, but no code path ever calls upperMgr.Collect() (the GC loop only calls CollectIdle). As a result, every deleted overlay session leaves its upper/work directories on disk until the pod is destroyed, which can steadily fill the execd filesystem in long-running pods.
Useful? React with 👍 / 👎.
| go func() { | ||
| select { | ||
| case <-ctx.Done(): | ||
| stdin.Close() | ||
| case <-done: |
There was a problem hiding this comment.
When a client disconnects or a request context is canceled during a run, this goroutine closes the persistent bash stdin, which normally causes the session process to exit; however the session remains stored as active and its upper directory remains in use. The next run on that session will fail against a dead pipe/process while GET /v1/isolated/session/:id still reports an active session, so cancellation should stop/delete or mark the session destroyed.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| telemetry.RecordIsolatedRun(ctx, "error", durationMs) | ||
| event := model.ServerStreamEvent{ | ||
| Type: model.StreamEventTypeError, | ||
| Text: err.Error(), | ||
| Timestamp: time.Now().UnixMilli(), | ||
| } | ||
| c.writeSingleEvent("IsolatedError", event.ToJSON(), true, event.Summary()) |
There was a problem hiding this comment.
If sessionId does not exist, RunInIsolatedSession returns ErrContextNotFound, but this branch converts every error into a 200 text/event-stream error frame. Callers using the new OpenAPI contract for /v1/isolated/session/{sessionId}/run cannot distinguish a missing session from an execution failure via the documented 404 response.
Useful? React with 👍 / 👎.
…e 1) - Add pkg/isolation/ package: Isolator interface, bwrap argv builder, startup probe, upper directory management, seccomp loading - Switch bwrap distribution from //go:embed to Dockerfile static build (musl-gcc) and init container injection alongside execd - Add isolation flags (upper root, max bytes, diff max bytes, allowed writable) with env var overrides - Add smoke test: Docker build, extract binaries, verify static link, bwrap namespace test, execd probe - Add smoke_bwrap.sh to CI workflow (ubuntu-latest only) - Defer diff/commit to Phase 2 (stub returning 503) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Add isolated session model types and /v1/isolated/* router (17 endpoints) - Implement session lifecycle: Create/Get/Run/Delete with bwrap+bash - SSE streaming via basicController writeSingleEvent (context-aware) - MergedView overlay filesystem with whiteout support (20 unit tests) - Filesystem proxy endpoints (10 handlers via MergedView) - Idle GC: background goroutine scavenges sessions past idle_timeout - Bwrap integration tests (43 tests, linux+bwrap build tag) - Isolated session unit tests (15 tests, stub isolator) - CI job bwrap-smoke: meson build bwrap v0.11.2 + sudo go test - Windows stubs for cross-platform compilation - Fix: cmd.Wait() zombie cleanup, context cancellation propagation, setpriv skip when uid=0, correct v0.11.x overlay syntax Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ts (OSEP-0013) Seccomp BPF: - pkg/isolation/seccomp_gen.go — BPF generator (elastic/go-seccomp-bpf, pure Go) - Default-allow denylist: 40+ dangerous syscalls blocked (mount, ptrace, etc.) - BPF passed to bwrap via memfd + ExtraFiles fd Telemetry: - execd.isolation.session.count (gauge) - execd.isolation.run.duration (histogram, ms) - execd.isolation.upper.usage_bytes (gauge) - IsolationStatsProvider pattern for gauge callbacks OpenAPI Spec: - specs/execd-api.yaml: 17 endpoints, 10 schemas, ServiceUnavailable response Integration tests (64 total in bwrap_test/): - 5 seccomp tests (filter active, normal syscalls, ptrace block, mount block, persist) - 3 ExtraWritable tests (write, read-write roundtrip, multiple sessions) - 11 gap-coverage tests (stderr, recovery, cancellation, network iso, 100x stress, delete-recreate, bash builtins, large file, subprocess cleanup, buffer size, workspace isolation) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
17 endpoints, 10 new schemas, ServiceUnavailable response. FS proxy endpoints reuse same schemas as /files/* and /directories/*. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…terface (OSEP-0013) Review fixes: - Fix session lifecycle: done channel prevents stdin close on normal return - Fix overlay mode: pass upper/work dirs to bwrap WrapOptions - Fix end marker mid-line concatenation with strings.Index - Extract scanUntilMarker() to reduce cognitive complexity - Add runMu serialization, upperID tracking, validateExtraWritable() - Delete dead seccomp.go (replaced by seccomp_gen.go + memfd) - Fix MergedView.ReadDir upper-takes-precedence via entryMap - Fix UpperManager.Collect delete-on-success only TOML config (replaces 4 --isolation-* flags): - New isolation.Config + LoadConfig() with go-toml/v2 - Seccomp override: [seccomp].deny fully replaces built-in denylist - configs/isolation.example.toml with documented defaults - Single --isolation-config flag + EXECD_ISOLATION_CONFIG env API simplification: - Flatten CreateIsolatedSessionRequest (remove isolation wrapper) - Delete dead PersistSpec, ArtifactURLs, IsolationSpec types - Wire up envs param with shell-escaped export prepend - Remove unused cwd from IsolatedRunRequest - Update OpenAPI spec to match VFS interface: - New pkg/vfs.FS interface for filesystem abstraction - MergedView satisfies vfs.FS (compile-time check) - Isolated file handlers use vfs.FS instead of concrete MergedView - TODO for FilesystemController migration Tests: - 6 end-to-end workflow tests (Run↔File API interop) - 2 seccomp config override e2e tests - 8 config loading tests - Document overlayfs API→Run limitation in MergedView Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pangjiping
force-pushed
the
osep-0013-phase1-isolation-core
branch
from
5ff9482 to
3f8c557
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df7894d773
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ) -> IsolatedSessionInfo: | ||
| try: | ||
| url = self._get_url(self.CREATE_PATH) | ||
| body = request.model_dump(exclude_none=True) |
There was a problem hiding this comment.
When a Python SDK caller supplies only workspace.path, the Pydantic defaults (profile="balanced" and workspace.mode="rw") are included because this dumps with exclude_none only; the sync adapter has the same serialization. That overrides the server/OSEP omitted-field defaults of strict/overlay, so SDK-created sessions unexpectedly share /tmp and write directly to the workspace instead of using the stricter copy-on-write defaults.
Useful? React with 👍 / 👎.
| event := model.ServerStreamEvent{ | ||
| Type: model.StreamEventTypeError, | ||
| Text: err.Error(), | ||
| Timestamp: time.Now().UnixMilli(), |
There was a problem hiding this comment.
When a run fails after the SSE stream has started, this emits an error event with only text; the Python SDK's existing dispatcher handles error events only when the error object is present, so a non-zero command exit is returned to callers with no execution.error and no usable exit code. This affects normal inputs like false or exit 2, and makes SDK callers treat failed isolated runs as ambiguous successful streams.
Useful? React with 👍 / 👎.
| EnvPassthrough EnvPassthroughSpec `json:"env_passthrough,omitempty"` | ||
| Uid *uint32 `json:"uid,omitempty"` | ||
| Gid *uint32 `json:"gid,omitempty"` | ||
| IdleTimeoutSeconds int `json:"idle_timeout_seconds,omitempty"` |
There was a problem hiding this comment.
Because idle_timeout_seconds is a plain int, an omitted field becomes 0, and the GC path treats 0 as disabled rather than applying the OSEP default of 1800 seconds. Any client that creates a session without explicitly setting this field therefore leaves the persistent bash process and upper directory alive indefinitely until it remembers to delete the session.
Useful? React with 👍 / 👎.
| "chmod +x /opt/opensandbox/bootstrap.sh" | ||
| "chmod +x /opt/opensandbox/bootstrap.sh && " | ||
| "cp /usr/local/bin/bwrap /opt/opensandbox/bwrap 2>/dev/null && " | ||
| "chmod +x /opt/opensandbox/bwrap 2>/dev/null; true" |
There was a problem hiding this comment.
In the Kubernetes init container, the final ; true makes the whole installer script exit successfully even when earlier required steps fail, such as copying execd or bootstrap.sh into the shared volume. If that volume is unwritable or a required artifact is missing, the pod proceeds to start the sandbox container without the runtime launcher instead of failing during init; only the optional bwrap copy should be made non-fatal.
Useful? React with 👍 / 👎.
| for k, v := range envs { | ||
| script += fmt.Sprintf("export %s=%s\n", shellescape(k), shellescape(v)) |
There was a problem hiding this comment.
For requests that pass envs, these generated export statements mutate the persistent bash session before running the command, so a value supplied for one run remains visible to every later run on the same session even when omitted. That is especially surprising for per-run secrets or credentials, because run.envs is documented as the command's highest-priority environment rather than durable session state.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| for file, item := range request { | ||
| if chmodErr := mv.Chmod(file, os.FileMode(item.Mode)); chmodErr != nil { |
There was a problem hiding this comment.
The isolated permissions endpoint accepts the same Permission.mode values as /files/permissions, where callers send octal-looking integers such as 755, but this direct cast applies decimal 755 (01363) instead of 0755 and also ignores owner/group changes. A client changing a file to mode 755 will therefore get unexpected sticky/write bits rather than the requested permissions.
Useful? React with 👍 / 👎.
| c.RespondError(http.StatusInternalServerError, model.ErrorCodeRuntimeError, err.Error()) | ||
| return | ||
| } | ||
| c.RespondSuccess(results) |
There was a problem hiding this comment.
The OpenAPI contract for isolatedSearchFiles says the response is an array of FileInfo, matching /files/search, but this returns the raw []string paths from MergedView.Search. Generated clients expecting objects with path, size, and permissions will fail to deserialize successful isolated search responses.
Useful? React with 👍 / 👎.
| session.workDir = workDir | ||
| } | ||
|
|
||
| if err := session.start(); err != nil { |
There was a problem hiding this comment.
The OSEP says workspace.path is auto-created when absent, but this goes straight to session.start() without creating the directory. For a create request that targets a new workspace path, bwrap's bind/overlay setup fails and the API returns a runtime error instead of creating the workspace and starting the session.
Useful? React with 👍 / 👎.
| cleaned := filepath.Clean(path) | ||
| if strings.HasPrefix(cleaned, "..") { | ||
| return "", fmt.Errorf("path traversal denied: %s", path) | ||
| } | ||
| return cleaned, nil |
There was a problem hiding this comment.
The isolated file endpoints reuse the /files/* schemas, so clients send absolute paths like /workspace/a.txt, but safePath returns that cleaned absolute path unchanged and later resolves it under LowerDir/UpperDir, producing lookups such as /workspace/workspace/a.txt. As a result, documented absolute paths inside the workspace miss or write to the wrong nested location unless callers know to send undocumented relative paths.
Useful? React with 👍 / 👎.
| var uid, gid uint32 | ||
| if s.opts.Uid != nil { | ||
| uid = *s.opts.Uid | ||
| } | ||
| if s.opts.Gid != nil { | ||
| gid = *s.opts.Gid | ||
| } |
There was a problem hiding this comment.
When a session is created without explicit uid/gid, the bwrap process runs as execd's current UID/GID, but the merged view defaults both values to 0. In non-root sandbox images, isolated uploads/replaces then try to chown files to root and fail with EPERM, or create files the session user cannot edit; the defaults should match the actual session user.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df7894d773
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ) -> IsolatedSessionInfo: | ||
| try: | ||
| url = self._get_url(self.CREATE_PATH) | ||
| body = request.model_dump(exclude_none=True) |
There was a problem hiding this comment.
When a Python SDK caller supplies only workspace.path, the Pydantic defaults (profile="balanced" and workspace.mode="rw") are included because this dumps with exclude_none only; the sync adapter has the same serialization. That overrides the server/OSEP omitted-field defaults of strict/overlay, so SDK-created sessions unexpectedly share /tmp and write directly to the workspace instead of using the stricter copy-on-write defaults.
Useful? React with 👍 / 👎.
| event := model.ServerStreamEvent{ | ||
| Type: model.StreamEventTypeError, | ||
| Text: err.Error(), | ||
| Timestamp: time.Now().UnixMilli(), |
There was a problem hiding this comment.
When a run fails after the SSE stream has started, this emits an error event with only text; the Python SDK's existing dispatcher handles error events only when the error object is present, so a non-zero command exit is returned to callers with no execution.error and no usable exit code. This affects normal inputs like false or exit 2, and makes SDK callers treat failed isolated runs as ambiguous successful streams.
Useful? React with 👍 / 👎.
| EnvPassthrough EnvPassthroughSpec `json:"env_passthrough,omitempty"` | ||
| Uid *uint32 `json:"uid,omitempty"` | ||
| Gid *uint32 `json:"gid,omitempty"` | ||
| IdleTimeoutSeconds int `json:"idle_timeout_seconds,omitempty"` |
There was a problem hiding this comment.
Because idle_timeout_seconds is a plain int, an omitted field becomes 0, and the GC path treats 0 as disabled rather than applying the OSEP default of 1800 seconds. Any client that creates a session without explicitly setting this field therefore leaves the persistent bash process and upper directory alive indefinitely until it remembers to delete the session.
Useful? React with 👍 / 👎.
| "chmod +x /opt/opensandbox/bootstrap.sh" | ||
| "chmod +x /opt/opensandbox/bootstrap.sh && " | ||
| "cp /usr/local/bin/bwrap /opt/opensandbox/bwrap 2>/dev/null && " | ||
| "chmod +x /opt/opensandbox/bwrap 2>/dev/null; true" |
There was a problem hiding this comment.
In the Kubernetes init container, the final ; true makes the whole installer script exit successfully even when earlier required steps fail, such as copying execd or bootstrap.sh into the shared volume. If that volume is unwritable or a required artifact is missing, the pod proceeds to start the sandbox container without the runtime launcher instead of failing during init; only the optional bwrap copy should be made non-fatal.
Useful? React with 👍 / 👎.
| for k, v := range envs { | ||
| script += fmt.Sprintf("export %s=%s\n", shellescape(k), shellescape(v)) |
There was a problem hiding this comment.
For requests that pass envs, these generated export statements mutate the persistent bash session before running the command, so a value supplied for one run remains visible to every later run on the same session even when omitted. That is especially surprising for per-run secrets or credentials, because run.envs is documented as the command's highest-priority environment rather than durable session state.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| for file, item := range request { | ||
| if chmodErr := mv.Chmod(file, os.FileMode(item.Mode)); chmodErr != nil { |
There was a problem hiding this comment.
The isolated permissions endpoint accepts the same Permission.mode values as /files/permissions, where callers send octal-looking integers such as 755, but this direct cast applies decimal 755 (01363) instead of 0755 and also ignores owner/group changes. A client changing a file to mode 755 will therefore get unexpected sticky/write bits rather than the requested permissions.
Useful? React with 👍 / 👎.
| c.RespondError(http.StatusInternalServerError, model.ErrorCodeRuntimeError, err.Error()) | ||
| return | ||
| } | ||
| c.RespondSuccess(results) |
There was a problem hiding this comment.
The OpenAPI contract for isolatedSearchFiles says the response is an array of FileInfo, matching /files/search, but this returns the raw []string paths from MergedView.Search. Generated clients expecting objects with path, size, and permissions will fail to deserialize successful isolated search responses.
Useful? React with 👍 / 👎.
| session.workDir = workDir | ||
| } | ||
|
|
||
| if err := session.start(); err != nil { |
There was a problem hiding this comment.
The OSEP says workspace.path is auto-created when absent, but this goes straight to session.start() without creating the directory. For a create request that targets a new workspace path, bwrap's bind/overlay setup fails and the API returns a runtime error instead of creating the workspace and starting the session.
Useful? React with 👍 / 👎.
| cleaned := filepath.Clean(path) | ||
| if strings.HasPrefix(cleaned, "..") { | ||
| return "", fmt.Errorf("path traversal denied: %s", path) | ||
| } | ||
| return cleaned, nil |
There was a problem hiding this comment.
The isolated file endpoints reuse the /files/* schemas, so clients send absolute paths like /workspace/a.txt, but safePath returns that cleaned absolute path unchanged and later resolves it under LowerDir/UpperDir, producing lookups such as /workspace/workspace/a.txt. As a result, documented absolute paths inside the workspace miss or write to the wrong nested location unless callers know to send undocumented relative paths.
Useful? React with 👍 / 👎.
| var uid, gid uint32 | ||
| if s.opts.Uid != nil { | ||
| uid = *s.opts.Uid | ||
| } | ||
| if s.opts.Gid != nil { | ||
| gid = *s.opts.Gid | ||
| } |
There was a problem hiding this comment.
When a session is created without explicit uid/gid, the bwrap process runs as execd's current UID/GID, but the merged view defaults both values to 0. In non-root sandbox images, isolated uploads/replaces then try to chown files to root and fail with EPERM, or create files the session user cannot edit; the defaults should match the actual session user.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10deab826b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if err := scanner.Err(); err != nil { | ||
| return 0, fmt.Errorf("read stdout: %w", err) | ||
| } | ||
| return exitCode, nil |
There was a problem hiding this comment.
When user code executes the shell built-in exit (for example exit 2) or otherwise closes the persistent bash before the appended marker runs, the scanner reaches EOF without recording that no marker was seen and this return reports exit code 0. The handler then emits a successful complete event while the session process is dead but still stored, so the original failure is hidden and the next run fails against a broken session.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| startTime := time.Now() | ||
| err := isolatedRunner.RunInIsolatedSession(ctx, sessionID, req.Code, req.Envs, onStdout) |
There was a problem hiding this comment.
When a client supplies timeout_seconds in IsolatedRunRequest (the new Python SDK exposes this option), the value is decoded but dropped here; only the HTTP request context is passed to the runner. A request such as sleep 3600 with timeout_seconds: 1 therefore keeps running until the client disconnects instead of being terminated at the documented server-side timeout.
Useful? React with 👍 / 👎.
| return os.Chmod(upperPath, mode) | ||
| } | ||
| } | ||
| return os.Chmod(m.resolveLower(rel), mode) |
There was a problem hiding this comment.
In overlay sessions, if chmod targets a file that exists only in the lower workspace, this fallthrough applies os.Chmod directly to the original workspace file. That violates the copy-on-write contract for overlay mode and leaks permission changes from an isolated session back into the shared workspace; the file needs to be copied to the upper layer before changing its mode.
Useful? React with 👍 / 👎.
| oldPath := c.ctx.Query("old_path") | ||
| newPath := c.ctx.Query("new_path") |
There was a problem hiding this comment.
The new OpenAPI operation isolatedRenameFiles declares a JSON request body containing an array of RenameFileItem, but this handler ignores the body and requires undocumented old_path/new_path query parameters. Generated clients that follow the spec will POST the body and consistently receive old_path and new_path are required, so isolated rename is unusable through the published API.
Useful? React with 👍 / 👎.
| return | ||
| } | ||
|
|
||
| filePath := c.ctx.Query("path") |
There was a problem hiding this comment.
The isolated upload contract mirrors /files/upload with multipart metadata carrying the destination path, but this handler requires an undocumented path query parameter and ignores the metadata part. Clients generated from specs/execd-api.yaml will send only metadata plus file and get path is required instead of uploading to the documented metadata path.
Useful? React with 👍 / 👎.
| sessionID := s.id | ||
| s.mu.RUnlock() | ||
|
|
||
| if timeout > 0 && idle > timeout { |
There was a problem hiding this comment.
When a command runs longer than idle_timeout_seconds (for example a 2-minute sleep with a 30-second idle timeout), the GC still compares now to the previous lastRunAt and calls DeleteIsolatedSession without checking whether runMu is held. That kills the active bash process in the middle of the request even though the session is not idle; the timeout should be measured from run completion or track an in-flight run state.
Useful? React with 👍 / 👎.
| } | ||
| // ExtraFiles are assigned fds starting at 3 in the child process. | ||
| seccompFd = strconv.Itoa(3 + len(cmd.ExtraFiles)) | ||
| cmd.ExtraFiles = append(cmd.ExtraFiles, os.NewFile(uintptr(fd), "seccomp")) |
There was a problem hiding this comment.
With the default seccomp filter enabled, every wrapped command allocates a memfd and appends it to cmd.ExtraFiles, but neither the success path nor isolatedSession.stop() closes that parent-side *os.File. Repeated create/delete cycles therefore leak one file descriptor per session until execd can no longer start new isolated sessions.
Useful? React with 👍 / 👎.
| return info, nil | ||
| } | ||
| } | ||
| return os.Stat(m.resolveLower(rel)) |
There was a problem hiding this comment.
When code inside an overlay session deletes a file that came from the lower workspace, overlayfs creates an upper .wh.<name> entry. These direct lookups ignore that whiteout and fall back to the lower file, so files/info or files/download can still expose files that the session already removed; check for the whiteout before returning lower metadata/content.
Useful? React with 👍 / 👎.
| if err := os.MkdirAll(filepath.Dir(newUpper), 0o755); err != nil { | ||
| return err | ||
| } | ||
| return os.Rename(oldUpper, newUpper) |
There was a problem hiding this comment.
For an overlay rename of a file that exists only in the lower workspace, this copies the lower file to upper and renames the upper copy, but it never whiteouts the original lower path. The merged view then shows both the old and new names after a successful rename, so callers cannot actually move files that were part of the original workspace.
Useful? React with 👍 / 👎.
| } | ||
| return &UpperManager{ | ||
| root: root, | ||
| maxBytes: maxBytes, |
There was a problem hiding this comment.
upper_max_bytes is documented as the hard limit for session upper data, but the manager only stores the value and no allocation or write path checks it. Overlay runs and isolated uploads can therefore grow upper directories past the configured cap and fill the execd filesystem despite operators setting a lower limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81b1742d96
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| filePath := c.ctx.Query("path") | ||
| oldStr := c.ctx.Query("old") | ||
| newStr := c.ctx.Query("new") |
There was a problem hiding this comment.
The OpenAPI contract for isolatedReplaceContent declares a JSON body mapping file paths to {old,new} replacement specs, but this handler ignores the body and requires undocumented path, old, and new query parameters. Generated clients that follow the spec will POST the body and consistently receive path and old are required, so the isolated replace endpoint is unusable through the published API.
Useful? React with 👍 / 👎.
| return | ||
| } | ||
|
|
||
| paths := c.ctx.QueryArray("path") |
There was a problem hiding this comment.
The isolatedMakeDirs contract mirrors /directories and sends a JSON object keyed by directory path, but this handler only reads undocumented repeated path query parameters. A generated client posting { "/workspace/new": {"mode": 755} } will hit an empty paths slice, return 200, and create nothing; the directory targets and permissions need to be parsed from the body.
Useful? React with 👍 / 👎.
| return | ||
| } | ||
|
|
||
| paths := c.ctx.QueryArray("path") |
There was a problem hiding this comment.
The new isolatedRemoveFiles operation declares a required JSON body containing the files to remove, while this implementation looks only at repeated path query parameters. Spec-generated clients will send the body, the loop will run zero times, and the API will return success without deleting any requested files.
Useful? React with 👍 / 👎.
| return | ||
| } | ||
|
|
||
| paths := c.ctx.QueryArray("path") |
There was a problem hiding this comment.
The isolatedRemoveDirs operation declares a required JSON body with the directories to remove, but this handler reads only repeated path query parameters. Clients generated from the spec will send the body, paths will be empty, and the API will return 200 without deleting any requested directories.
Useful? React with 👍 / 👎.
| return | ||
| } | ||
|
|
||
| paths := c.ctx.QueryArray("path") |
There was a problem hiding this comment.
The OpenAPI contract for isolatedGetFilesInfo has no path query parameter and instead declares a required JSON body, while this handler only reads QueryArray("path"). A spec-generated client that sends the documented body gets an empty 200 response because no query paths are present, so file info appears to succeed while returning no metadata.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| if s.upperID != "" { | ||
| r.upperMgr.Release(s.upperID) |
There was a problem hiding this comment.
DeleteIsolatedSession only marks the upper entry released, but the runtime GC never calls UpperManager.Collect (repo-wide search in components/execd/pkg shows only tests call it). Every deleted or idle-expired overlay session therefore leaves its upper/work directories on disk indefinitely, so create/delete cycles can fill /var/lib/execd/isolation even after sessions are gone.
Useful? React with 👍 / 👎.
| return 0, err | ||
| } | ||
|
|
||
| f, err := os.OpenFile(upperPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm) |
There was a problem hiding this comment.
When an overlay session has an upper entry that is a symlink, this host-side OpenFile follows it outside the session upper directory. A command can create /workspace/link -> /tmp/target and then an isolated upload to link writes /tmp/target from execd, bypassing the bwrap workspace/extra_writable boundary; open the final path without following symlinks or validate the resolved target remains in the upper tree.
Useful? React with 👍 / 👎.
| return err | ||
| } | ||
| content := strings.ReplaceAll(string(data), old, newStr) | ||
| return m.WriteFile(path, []byte(content), 0o644) //nolint:gosec |
There was a problem hiding this comment.
Replacing content always writes the upper copy with 0644, so an isolated replace of an executable or private lower file changes its permissions even though the filesystem replace behavior is expected to preserve metadata. For example, replacing text in a 0755 script makes it non-executable for subsequent runs in the same session.
Useful? React with 👍 / 👎.
| default: | ||
| wrapOpts.Workspace.Mode = isolation.WorkspaceOverlay |
There was a problem hiding this comment.
Any unrecognized non-empty workspace.mode falls into this default branch and creates a writable overlay session instead of rejecting the request. If a client sends a typo while intending ro, the API silently grants a writable copy-on-write workspace; distinguish omitted mode from invalid enum values and return a bad request.
Useful? React with 👍 / 👎.
| if m.UpperDir != "" { | ||
| return os.RemoveAll(m.resolveUpper(rel)) |
There was a problem hiding this comment.
In overlay mode, deleting a directory that exists only in the lower workspace calls RemoveAll on the missing upper path, and os.RemoveAll returns nil for missing paths. The API therefore reports success while the lower directory remains visible to later reads/runs; lower-only directories need a whiteout or an error like lower-only file deletes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a8e0c9762
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| telemetry.RecordIsolatedRun(ctx, "error", durationMs) | ||
| event := model.ServerStreamEvent{ | ||
| Type: model.StreamEventTypeError, | ||
| Text: err.Error(), |
There was a problem hiding this comment.
When an isolated command fails, this emits an SSE error event with only text. The new Python isolated adapters reuse ExecutionEventDispatcher, whose _handle_error returns immediately unless the event has an error object, so sandbox.isolated.run("exit 2") completes the HTTP/SSE stream without setting execution.error or exit_code. Populate the Error field (as the existing command streaming API does) so SDK clients can observe non-zero exits and runtime failures.
Useful? React with 👍 / 👎.
| mode: str = Field( | ||
| default="rw", | ||
| description="Bind mode: 'rw' (read-write), 'overlay' (copy-on-write), or 'ro' (read-only)", |
There was a problem hiding this comment.
Because the adapters serialize with model_dump(exclude_none=True), this default is always sent when callers construct IsolatedWorkspaceSpec(path=...). The execd controller/runtime treats an omitted workspace mode as overlay, but SDK callers silently get direct rw binds instead, so writes from supposedly default isolated sessions leak back into the workspace. Make the SDK field optional or omit defaults when dumping so the server default is preserved unless users explicitly request rw.
Useful? React with 👍 / 👎.
| if err := c.bindJSON(&req); err != nil { | ||
| c.RespondError(http.StatusBadRequest, model.ErrorCodeInvalidRequest, err.Error()) | ||
| return | ||
| } |
There was a problem hiding this comment.
After this JSON bind succeeds, the handler never calls req.Validate(), so a request such as {} or {"code":""} reaches RunInIsolatedSession with empty code and emits a successful completion event. The OpenAPI schema marks code as required, and the existing code execution handler rejects this case; validate the isolated request before starting the SSE run so malformed clients don't see a successful no-op.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| argv = append(argv, wsArgv...) |
There was a problem hiding this comment.
For a strict session whose workspace is /tmp, the earlier strict --tmpfs /tmp is immediately hidden by this later workspace mount (--bind /tmp /tmp in rw mode). That makes two strict sessions using /tmp share the host/container tmp directory instead of getting isolated tmpfs contents, which breaks the advertised strict-profile behavior and the new E2E scenario that uses /tmp as the workspace.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| for file, item := range request { | ||
| if chmodErr := mv.Chmod(file, os.FileMode(item.Mode)); chmodErr != nil { |
There was a problem hiding this comment.
Permission.mode is documented and handled by /files/permissions as octal digits such as 755, but this direct cast treats the JSON number as decimal. An isolated chmod request with {"mode":755} therefore applies mode 01363 instead of 0755, adding the wrong special/write bits and making permission round-trips incorrect; parse the integer as base 8 before calling mv.Chmod.
Useful? React with 👍 / 👎.
| var req model.CreateIsolatedSessionRequest | ||
| if err := c.bindJSON(&req); err != nil { | ||
| c.RespondError(http.StatusBadRequest, model.ErrorCodeInvalidRequest, err.Error()) | ||
| return | ||
| } |
There was a problem hiding this comment.
After decoding the create body, this proceeds straight to runtime setup without checking the required workspace.path or enum fields. A malformed request like {"workspace":{}} is client input that should return the documented 400, but it reaches bwrap startup, may allocate an overlay upper directory, and is reported as a 500 runtime error; validate the create request before constructing the runtime options.
Useful? React with 👍 / 👎.
| "chmod +x /opt/opensandbox/bootstrap.sh" | ||
| "chmod +x /opt/opensandbox/bootstrap.sh && " | ||
| "cp /usr/local/bin/bwrap /opt/opensandbox/bwrap 2>/dev/null && " | ||
| "chmod +x /opt/opensandbox/bwrap 2>/dev/null; true" |
There was a problem hiding this comment.
Because the bwrap best-effort copy is appended as ... && cp bwrap ... && chmod ...; true, the whole shell command exits with true even when an earlier required step such as copying execd or bootstrap.sh fails. In Kubernetes sandboxes this lets the init container report success while /opt/opensandbox/execd or the bootstrap launcher is missing, so the main container starts broken instead of surfacing the install failure; scope the || true only to the optional bwrap copy.
Useful? React with 👍 / 👎.
| Text: err.Error(), | ||
| Timestamp: time.Now().UnixMilli(), | ||
| } | ||
| c.writeSingleEvent("IsolatedError", event.ToJSON(), true, event.Summary()) |
There was a problem hiding this comment.
When sessionId is unknown, RunInIsolatedSession returns ErrContextNotFound before any output is streamed, but this branch still commits a 200 text/event-stream error event. Clients following the OpenAPI response map will not see the documented 404 and SDK callers may treat a missing session as a completed stream rather than a not-found API error; special-case ErrContextNotFound before writing SSE.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bb7a5cedc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return os.Chmod(upperPath, mode) | ||
| } | ||
| } | ||
| return os.Chmod(m.resolveLower(rel), mode) |
There was a problem hiding this comment.
In overlay sessions, when the target exists only in the lower workspace this falls through to os.Chmod(m.resolveLower(rel), mode), so POST /v1/isolated/.../files/permissions mutates the original workspace permissions outside the isolated copy-on-write session. A chmod of a lower-only script or private file should be copied up (or rejected) rather than changing the shared lower tree.
Useful? React with 👍 / 👎.
| oldPath := c.ctx.Query("old_path") | ||
| newPath := c.ctx.Query("new_path") |
There was a problem hiding this comment.
The isolatedRenameFiles OpenAPI operation declares a required JSON array of RenameFileItem, but this handler only looks for undocumented old_path and new_path query parameters. Spec-generated clients that POST the documented body will always hit the missing-query 400 and cannot rename files through this endpoint.
Useful? React with 👍 / 👎.
| c.RespondError(http.StatusInternalServerError, model.ErrorCodeRuntimeError, err.Error()) | ||
| return | ||
| } | ||
| c.RespondSuccess(results) |
There was a problem hiding this comment.
The isolated search contract mirrors /files/search and declares an array of FileInfo, but this responds with the raw []string returned by mv.Search. Clients generated from the spec will expect metadata objects with fields like path, size, and permission, so successful searches either fail to decode or lose the documented file metadata.
Useful? React with 👍 / 👎.
| return | ||
| } | ||
|
|
||
| ctx, cancel := context.WithCancel(c.ctx.Request.Context()) |
There was a problem hiding this comment.
When clients send timeout_seconds (the SDKs expose this as the maximum run duration), the handler never uses req.TimeoutSeconds; it builds a plain request context here and passes it to RunInIsolatedSession. A run like sleep 3600 with timeout_seconds: 1 will keep occupying the session/SSE stream until the client disconnects instead of being terminated at the requested deadline.
Useful? React with 👍 / 👎.
| security_context.seccomp_profile = V1SeccompProfile(type="Unconfined") | ||
| security_context.app_armor_profile = V1AppArmorProfile(type="Unconfined") |
There was a problem hiding this comment.
In the Kubernetes path with bootstrap.execd.isolation=enable, these unconfined profiles are assigned to the V1SecurityContext, but _container_to_dict serializes through serialize_security_context_to_dict, which only emits capabilities/privileged. The generated Sandbox/BatchSandbox pod spec therefore contains SYS_ADMIN but not unconfined seccomp/AppArmor, so bwrap can still be blocked by the default profiles even though isolation was requested.
Useful? React with 👍 / 👎.
| if err := scanner.Err(); err != nil { | ||
| return 0, fmt.Errorf("read stdout: %w", err) | ||
| } | ||
| return exitCode, nil |
There was a problem hiding this comment.
If the submitted script exits the persistent shell before the appended marker runs (for example exit 7 or set -e; false), the scanner loop reaches EOF with exitCode still zero and this returns success. The controller then emits an execution_complete event and leaves a dead session instead of reporting that the marker was never seen.
Useful? React with 👍 / 👎.
|
|
||
| // safePath validates and cleans a relative path. | ||
| func (m *MergedView) safePath(path string) (string, error) { | ||
| cleaned := filepath.Clean(path) |
There was a problem hiding this comment.
The isolated file endpoints pass sandbox paths such as /workspace/app.py, but this helper keeps the leading workspace component and later joins it under LowerDir/UpperDir; with a /workspace session, /workspace/app.py resolves to /workspace/workspace/app.py. That makes normal absolute paths fail or target the wrong file, so absolute paths inside the workspace need to be converted to workspace-relative paths before resolving.
Useful? React with 👍 / 👎.
| if spec.Mode == "" { | ||
| return nil |
There was a problem hiding this comment.
When env_passthrough is omitted (the default create request shape), this returns no bwrap environment arguments, so the child inherits execd's full environment and the built-in strictEnvBlacklist below is never applied. In sandboxes where execd has values like *_TOKEN or *_SECRET, an isolated session gets those secrets unless the caller explicitly opts into deny mode, which defeats the safe default implied by the denylist.
Useful? React with 👍 / 👎.
| return 0, err | ||
| } | ||
|
|
||
| upperPath := m.resolveUpper(rel) |
There was a problem hiding this comment.
For the default overlay workspace, isolated uploads and other host-side writes go directly into the upper directory here, but the running bwrap overlay mount is already established and the file header notes those direct upper-dir writes are not visible inside the namespace. A client can successfully upload a file to an overlay session and then run cannot see it; either perform writes through the session's mounted view or reject these file APIs for overlay mode.
Useful? React with 👍 / 👎.
| go func() { | ||
| select { | ||
| case <-ctx.Done(): | ||
| stdin.Close() |
There was a problem hiding this comment.
When the request context is cancelled, this closes the persistent bash stdin, which causes the shell to exit, but RunInIsolatedSession then returns the context error without deleting or marking the session dead. A client disconnect during a long run therefore leaves GET /session/{id} reporting active while subsequent runs fail on the closed pipe until someone explicitly deletes the zombie session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96752411dc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for _, p := range paths { | ||
| found := false | ||
| for _, allowed := range r.allowedWritable { | ||
| if strings.HasPrefix(p, allowed) { |
There was a problem hiding this comment.
When an allowlist entry is a prefix of another path, this raw HasPrefix check accepts paths that are not actually under the allowed directory, e.g. allowing /workspace/scratch also allows /workspace/scratch2 or /workspace/scratch/../../etc; buildArgv later bind-mounts the requested path writable. Clean/resolve both paths and require an exact match or a path-separator boundary so callers cannot escape the configured writable allowlist.
Useful? React with 👍 / 👎.
| upperDir = filepath.Join(m.root, id, "upper") | ||
| workDir = filepath.Join(m.root, id, "work") | ||
|
|
||
| if err := os.MkdirAll(upperDir, 0o755); err != nil { |
There was a problem hiding this comment.
Even when upper_max_bytes is configured, allocation always creates another upper/work directory without checking m.maxBytes or current usage, and the limit is otherwise only stored. Overlay sessions can therefore keep consuming the upper root until the filesystem fills despite the advertised hard cap; reject new allocations or writes once the configured total limit is reached.
Useful? React with 👍 / 👎.
| isolated.POST("/session/:sessionId/directories", withIsolated(func(c *controller.IsolatedSessionController) { c.MakeDirs() })) | ||
| isolated.DELETE("/session/:sessionId/directories", withIsolated(func(c *controller.IsolatedSessionController) { c.RemoveDirs() })) |
There was a problem hiding this comment.
The isolated session handles expose a full filesystem client scoped to /v1/isolated/session/{id}, and those clients call GET /directories/list?path=... for directory listings; checked this router and only POST/DELETE are registered for isolated directories. As a result, session.files.ListDirectory(...) on an isolated session returns 404 even though the SDK exposes it; register the GET route with an isolated list handler or avoid exposing the full filesystem interface.
Useful? React with 👍 / 👎.
| Size: info.Size(), | ||
| ModifiedAt: info.ModTime(), | ||
| Permission: model.Permission{ | ||
| Mode: int(info.Mode()), |
There was a problem hiding this comment.
When isolated file info is returned, int(info.Mode()) serializes the Go FileMode value in decimal, so a normal 0644 file is reported as 420 instead of the octal-style 644 used by the existing file API. Clients that display or round-trip FileInfo.permission.mode into chmod will apply the wrong permissions; build this metadata the same way as buildFileInfo using Mode().Perm() formatted in base 8.
Useful? React with 👍 / 👎.
|
|
||
| func (c *IsolatedSessionController) getMergedView() (vfs.FS, error) { | ||
| sessionID := c.ctx.Param("sessionId") | ||
| mv, err := isolatedRunner.GetMergedView(sessionID) |
There was a problem hiding this comment.
When isolation is unavailable, isolatedRunner is left nil and the non-file isolated endpoints guard with c.probed(), but every isolated file handler reaches this helper directly. A request such as GET /v1/isolated/session/x/files/download on a host without bwrap dereferences nil here and becomes a recovered 500 instead of the documented service-unavailable response; add the same availability check before calling the runner.
Useful? React with 👍 / 👎.
| if err := os.MkdirAll(filepath.Dir(newUpper), 0o755); err != nil { | ||
| return err | ||
| } | ||
| return os.Rename(oldUpper, newUpper) |
There was a problem hiding this comment.
In overlay mode, when the source exists only in the lower workspace this copies it up and renames the copy, but it never creates a whiteout for oldPath. The API reports a successful move while subsequent reads/runs still see the original lower file at the old path plus the new upper copy, so lower-only files are duplicated rather than moved.
Useful? React with 👍 / 👎.
| "setpriv", | ||
| fmt.Sprintf("--reuid=%d", uid), | ||
| fmt.Sprintf("--regid=%d", gid), | ||
| "--init-groups", |
There was a problem hiding this comment.
When callers provide an arbitrary numeric uid/gid, setpriv --init-groups requires looking up that UID in the container passwd database; I checked setpriv --help for the group options and verified setpriv --reuid=12345 --regid=12345 --init-groups true fails with “uid 12345 not found”. Since the API accepts numeric IDs, sessions for valid but non-named IDs fail to start; use --clear-groups or only initialize groups when a named user is known.
Useful? React with 👍 / 👎.
| sessionID := s.id | ||
| s.mu.RUnlock() | ||
|
|
||
| if timeout > 0 && idle > timeout { |
There was a problem hiding this comment.
For sessions with idle_timeout_seconds shorter than a long-running command, lastRunAt is only updated after the run returns, so this GC check still sees the session as idle while code is actively executing. A session that immediately runs sleep 120 with a 30-second idle timeout can be deleted by the GC despite being busy; mark active runs or update the activity timestamp before execution starts.
Useful? React with 👍 / 👎.
| } | ||
| // ExtraFiles are assigned fds starting at 3 in the child process. | ||
| seccompFd = strconv.Itoa(3 + len(cmd.ExtraFiles)) | ||
| cmd.ExtraFiles = append(cmd.ExtraFiles, os.NewFile(uintptr(fd), "seccomp")) |
There was a problem hiding this comment.
With the default seccomp filter, every isolated session appends a memfd-backed *os.File to cmd.ExtraFiles, but the parent never closes that file after bwrap inherits it and the session retains cmd. Create/delete cycles therefore keep one extra fd per session until garbage collection happens, which can eventually exhaust the execd process fd limit; close the parent-side ExtraFiles after cmd.Start or during session stop.
Useful? React with 👍 / 👎.
|
|
||
| perm := os.FileMode(0o644) | ||
| if meta.Permission.Mode != 0 { | ||
| perm = os.FileMode(meta.Permission.Mode) |
There was a problem hiding this comment.
Permission.mode is represented by octal digits such as 755 (the normal file APIs parse it in base 8), but this direct cast treats the JSON number as decimal. An isolated upload that requests mode 755 creates the file with mode 01363 instead of 0755, so uploaded executables or private files get the wrong permission bits; parse the mode as base 8 before writing.
Useful? React with 👍 / 👎.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c11f02dfcc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // The marker may appear mid-line if the previous command's | ||
| // output didn't end with a newline (e.g. cat of a file | ||
| // without trailing newline). | ||
| if idx := strings.Index(line, isolatedRunEndMarker); idx >= 0 { |
There was a problem hiding this comment.
Because the scanner accepts the fixed marker anywhere in user stdout, code that prints __ISOLATED_RUN_END__ 0 before it finishes makes RunInIsolatedSession return success early while the same persistent shell keeps running and can interleave output/state with later runs. Use a per-run nonce and only accept the appended marker in a form user output cannot accidentally or intentionally spoof.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| if s.upperID != "" { | ||
| r.upperMgr.Release(s.upperID) |
There was a problem hiding this comment.
Deleting an overlay/default session only marks the upper directory released, but the runner's GC loop only calls CollectIdle and no runtime path invokes UpperManager.Collect, so explicit deletes and idle-GC deletes leave every upper/work tree under upper_root on disk indefinitely. This will leak storage across normal session churn; delete the allocation here or run the upper-manager collector.
Useful? React with 👍 / 👎.
| filePath := c.ctx.Query("path") | ||
| oldStr := c.ctx.Query("old") | ||
| newStr := c.ctx.Query("new") |
There was a problem hiding this comment.
The isolated filesystem handle reuses the normal FilesystemAdapter, which sends POST .../files/replace as the documented JSON map of paths to replacement items. This handler ignores that body and requires undocumented path, old, and new query parameters instead, so SDK calls such as session.files.replaceContents(...) always hit the missing-query 400 instead of replacing content.
Useful? React with 👍 / 👎.
| return | ||
| } | ||
|
|
||
| paths := c.ctx.QueryArray("path") |
There was a problem hiding this comment.
For POST .../directories, the shared filesystem SDK and the spec send a JSON map of directory paths to permissions, matching the normal MakeDirs handler. This isolated handler only reads path query parameters, so session.files.createDirectories(...) sends a valid body, gets a 200, and creates nothing because paths is empty.
Useful? React with 👍 / 👎.
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| additionalProperties: | ||
| $ref: "#/components/schemas/FileInfo" |
There was a problem hiding this comment.
This isolated contract declares a required GET request body instead of the path query array that the registered handler and reused filesystem adapters actually use; the same body-vs-query drift appears on the isolated delete endpoints. Generated clients following this spec cannot call /files/info or removals correctly, while handwritten clients send undocumented query parameters, so mirror the normal filesystem operations' query parameter definitions here.
Useful? React with 👍 / 👎.
| if m.UpperDir != "" { | ||
| return os.RemoveAll(m.resolveUpper(rel)) |
There was a problem hiding this comment.
In overlay sessions, deleting a directory that exists only in the lower workspace removes the nonexistent upper path and returns nil, so DELETE .../directories?path=... reports success while the directory remains visible to both later file API reads and runs. Create an overlay whiteout/opaque marker or return an error instead of silently doing nothing for lower-only paths.
Useful? React with 👍 / 👎.
| upperDir = filepath.Join(m.root, id, "upper") | ||
| workDir = filepath.Join(m.root, id, "work") | ||
|
|
||
| if err := os.MkdirAll(upperDir, 0o755); err != nil { |
There was a problem hiding this comment.
Default overlay sessions store private copy-on-write data under upper_root, but the bwrap root still ro-binds /, so every session can see /var/lib/execd/isolation and these 0755 per-session upper directories. With two overlay sessions in the same sandbox, code in one session can list that root and read another session's modified files directly; hide/bind over the upper root inside bwrap and avoid world-traversable upper directories.
Useful? React with 👍 / 👎.
| } | ||
| cmd.Stderr = cmd.Stdout | ||
|
|
||
| if err := cmd.Start(); err != nil { |
There was a problem hiding this comment.
cmd.Start() only confirms that the bwrap process was spawned; mount/setup errors such as a nonexistent workspace path occur immediately after start and this still stores an active session id. Clients then get a successful create response for a dead shell and only see broken pipes/EOF on the first run, so add a readiness handshake or short exit check before storing the session.
Useful? React with 👍 / 👎.
| return result | ||
| } | ||
|
|
||
| result.Available = true |
There was a problem hiding this comment.
This marks isolation available after only bwrap --version, but the runtime's default workspace mode emits --tmp-overlay/--overlay options that the builder comments identify as bwrap v0.11.x behavior. On hosts where $PATH finds an older system bwrap, the smoke test can pass while default isolated session creation fails later with unsupported overlay options; require/probe the needed overlay support before returning available=true.
Useful? React with 👍 / 👎.
| if data, err := os.ReadFile(m.resolveUpper(rel)); err == nil { | ||
| return data, nil | ||
| } | ||
| } | ||
| return os.ReadFile(m.resolveLower(rel)) |
There was a problem hiding this comment.
When code inside an overlay session deletes a lower-layer file, the upper layer records a whiteout, but direct file APIs still try the upper path and then fall through to the lower file whenever ReadFile fails. That makes session.files.read(...) or download resurrect files the running session has deleted; check for whiteouts/deletion markers before falling back to the lower layer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c11f02dfcc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // The marker may appear mid-line if the previous command's | ||
| // output didn't end with a newline (e.g. cat of a file | ||
| // without trailing newline). | ||
| if idx := strings.Index(line, isolatedRunEndMarker); idx >= 0 { |
There was a problem hiding this comment.
Because the scanner accepts the fixed marker anywhere in user stdout, code that prints __ISOLATED_RUN_END__ 0 before it finishes makes RunInIsolatedSession return success early while the same persistent shell keeps running and can interleave output/state with later runs. Use a per-run nonce and only accept the appended marker in a form user output cannot accidentally or intentionally spoof.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| if s.upperID != "" { | ||
| r.upperMgr.Release(s.upperID) |
There was a problem hiding this comment.
Deleting an overlay/default session only marks the upper directory released, but the runner's GC loop only calls CollectIdle and no runtime path invokes UpperManager.Collect, so explicit deletes and idle-GC deletes leave every upper/work tree under upper_root on disk indefinitely. This will leak storage across normal session churn; delete the allocation here or run the upper-manager collector.
Useful? React with 👍 / 👎.
| filePath := c.ctx.Query("path") | ||
| oldStr := c.ctx.Query("old") | ||
| newStr := c.ctx.Query("new") |
There was a problem hiding this comment.
The isolated filesystem handle reuses the normal FilesystemAdapter, which sends POST .../files/replace as the documented JSON map of paths to replacement items. This handler ignores that body and requires undocumented path, old, and new query parameters instead, so SDK calls such as session.files.replaceContents(...) always hit the missing-query 400 instead of replacing content.
Useful? React with 👍 / 👎.
| return | ||
| } | ||
|
|
||
| paths := c.ctx.QueryArray("path") |
There was a problem hiding this comment.
For POST .../directories, the shared filesystem SDK and the spec send a JSON map of directory paths to permissions, matching the normal MakeDirs handler. This isolated handler only reads path query parameters, so session.files.createDirectories(...) sends a valid body, gets a 200, and creates nothing because paths is empty.
Useful? React with 👍 / 👎.
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| additionalProperties: | ||
| $ref: "#/components/schemas/FileInfo" |
There was a problem hiding this comment.
This isolated contract declares a required GET request body instead of the path query array that the registered handler and reused filesystem adapters actually use; the same body-vs-query drift appears on the isolated delete endpoints. Generated clients following this spec cannot call /files/info or removals correctly, while handwritten clients send undocumented query parameters, so mirror the normal filesystem operations' query parameter definitions here.
Useful? React with 👍 / 👎.
| if m.UpperDir != "" { | ||
| return os.RemoveAll(m.resolveUpper(rel)) |
There was a problem hiding this comment.
In overlay sessions, deleting a directory that exists only in the lower workspace removes the nonexistent upper path and returns nil, so DELETE .../directories?path=... reports success while the directory remains visible to both later file API reads and runs. Create an overlay whiteout/opaque marker or return an error instead of silently doing nothing for lower-only paths.
Useful? React with 👍 / 👎.
| upperDir = filepath.Join(m.root, id, "upper") | ||
| workDir = filepath.Join(m.root, id, "work") | ||
|
|
||
| if err := os.MkdirAll(upperDir, 0o755); err != nil { |
There was a problem hiding this comment.
Default overlay sessions store private copy-on-write data under upper_root, but the bwrap root still ro-binds /, so every session can see /var/lib/execd/isolation and these 0755 per-session upper directories. With two overlay sessions in the same sandbox, code in one session can list that root and read another session's modified files directly; hide/bind over the upper root inside bwrap and avoid world-traversable upper directories.
Useful? React with 👍 / 👎.
| } | ||
| cmd.Stderr = cmd.Stdout | ||
|
|
||
| if err := cmd.Start(); err != nil { |
There was a problem hiding this comment.
cmd.Start() only confirms that the bwrap process was spawned; mount/setup errors such as a nonexistent workspace path occur immediately after start and this still stores an active session id. Clients then get a successful create response for a dead shell and only see broken pipes/EOF on the first run, so add a readiness handshake or short exit check before storing the session.
Useful? React with 👍 / 👎.
| return result | ||
| } | ||
|
|
||
| result.Available = true |
There was a problem hiding this comment.
This marks isolation available after only bwrap --version, but the runtime's default workspace mode emits --tmp-overlay/--overlay options that the builder comments identify as bwrap v0.11.x behavior. On hosts where $PATH finds an older system bwrap, the smoke test can pass while default isolated session creation fails later with unsupported overlay options; require/probe the needed overlay support before returning available=true.
Useful? React with 👍 / 👎.
| if data, err := os.ReadFile(m.resolveUpper(rel)); err == nil { | ||
| return data, nil | ||
| } | ||
| } | ||
| return os.ReadFile(m.resolveLower(rel)) |
There was a problem hiding this comment.
When code inside an overlay session deletes a lower-layer file, the upper layer records a whiteout, but direct file APIs still try the upper path and then fall through to the lower file whenever ReadFile fails. That makes session.files.read(...) or download resurrect files the running session has deleted; check for whiteouts/deletion markers before falling back to the lower layer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c11f02dfcc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // The marker may appear mid-line if the previous command's | ||
| // output didn't end with a newline (e.g. cat of a file | ||
| // without trailing newline). | ||
| if idx := strings.Index(line, isolatedRunEndMarker); idx >= 0 { |
There was a problem hiding this comment.
Because the scanner accepts the fixed marker anywhere in user stdout, code that prints __ISOLATED_RUN_END__ 0 before it finishes makes RunInIsolatedSession return success early while the same persistent shell keeps running and can interleave output/state with later runs. Use a per-run nonce and only accept the appended marker in a form user output cannot accidentally or intentionally spoof.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| if s.upperID != "" { | ||
| r.upperMgr.Release(s.upperID) |
There was a problem hiding this comment.
Deleting an overlay/default session only marks the upper directory released, but the runner's GC loop only calls CollectIdle and no runtime path invokes UpperManager.Collect, so explicit deletes and idle-GC deletes leave every upper/work tree under upper_root on disk indefinitely. This will leak storage across normal session churn; delete the allocation here or run the upper-manager collector.
Useful? React with 👍 / 👎.
| filePath := c.ctx.Query("path") | ||
| oldStr := c.ctx.Query("old") | ||
| newStr := c.ctx.Query("new") |
There was a problem hiding this comment.
The isolated filesystem handle reuses the normal FilesystemAdapter, which sends POST .../files/replace as the documented JSON map of paths to replacement items. This handler ignores that body and requires undocumented path, old, and new query parameters instead, so SDK calls such as session.files.replaceContents(...) always hit the missing-query 400 instead of replacing content.
Useful? React with 👍 / 👎.
| return | ||
| } | ||
|
|
||
| paths := c.ctx.QueryArray("path") |
There was a problem hiding this comment.
For POST .../directories, the shared filesystem SDK and the spec send a JSON map of directory paths to permissions, matching the normal MakeDirs handler. This isolated handler only reads path query parameters, so session.files.createDirectories(...) sends a valid body, gets a 200, and creates nothing because paths is empty.
Useful? React with 👍 / 👎.
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| additionalProperties: | ||
| $ref: "#/components/schemas/FileInfo" |
There was a problem hiding this comment.
This isolated contract declares a required GET request body instead of the path query array that the registered handler and reused filesystem adapters actually use; the same body-vs-query drift appears on the isolated delete endpoints. Generated clients following this spec cannot call /files/info or removals correctly, while handwritten clients send undocumented query parameters, so mirror the normal filesystem operations' query parameter definitions here.
Useful? React with 👍 / 👎.
| if m.UpperDir != "" { | ||
| return os.RemoveAll(m.resolveUpper(rel)) |
There was a problem hiding this comment.
In overlay sessions, deleting a directory that exists only in the lower workspace removes the nonexistent upper path and returns nil, so DELETE .../directories?path=... reports success while the directory remains visible to both later file API reads and runs. Create an overlay whiteout/opaque marker or return an error instead of silently doing nothing for lower-only paths.
Useful? React with 👍 / 👎.
| upperDir = filepath.Join(m.root, id, "upper") | ||
| workDir = filepath.Join(m.root, id, "work") | ||
|
|
||
| if err := os.MkdirAll(upperDir, 0o755); err != nil { |
There was a problem hiding this comment.
Default overlay sessions store private copy-on-write data under upper_root, but the bwrap root still ro-binds /, so every session can see /var/lib/execd/isolation and these 0755 per-session upper directories. With two overlay sessions in the same sandbox, code in one session can list that root and read another session's modified files directly; hide/bind over the upper root inside bwrap and avoid world-traversable upper directories.
Useful? React with 👍 / 👎.
| } | ||
| cmd.Stderr = cmd.Stdout | ||
|
|
||
| if err := cmd.Start(); err != nil { |
There was a problem hiding this comment.
cmd.Start() only confirms that the bwrap process was spawned; mount/setup errors such as a nonexistent workspace path occur immediately after start and this still stores an active session id. Clients then get a successful create response for a dead shell and only see broken pipes/EOF on the first run, so add a readiness handshake or short exit check before storing the session.
Useful? React with 👍 / 👎.
| return result | ||
| } | ||
|
|
||
| result.Available = true |
There was a problem hiding this comment.
This marks isolation available after only bwrap --version, but the runtime's default workspace mode emits --tmp-overlay/--overlay options that the builder comments identify as bwrap v0.11.x behavior. On hosts where $PATH finds an older system bwrap, the smoke test can pass while default isolated session creation fails later with unsupported overlay options; require/probe the needed overlay support before returning available=true.
Useful? React with 👍 / 👎.
| if data, err := os.ReadFile(m.resolveUpper(rel)); err == nil { | ||
| return data, nil | ||
| } | ||
| } | ||
| return os.ReadFile(m.resolveLower(rel)) |
There was a problem hiding this comment.
When code inside an overlay session deletes a lower-layer file, the upper layer records a whiteout, but direct file APIs still try the upper path and then fall through to the lower file whenever ReadFile fails. That makes session.files.read(...) or download resurrect files the running session has deleted; check for whiteouts/deletion markers before falling back to the lower layer.
Useful? React with 👍 / 👎.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
3 similar comments
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Pangjiping
force-pushed
the
osep-0013-phase1-isolation-core
branch
from
6472029 to
f09f3f7
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
…ts (OSEP-0013) Python/Go/JS/Kotlin/C# SDKs: - IsolationService + IsolationSession handle pattern (sandbox.isolation.create() → session.run/get/delete/files) - Models, service interfaces, adapters for all 5 languages Server: - bwrap binary distribution (Docker cache + K8s init container) - bootstrap.execd.isolation=enable extension grants CAP_SYS_ADMIN + apparmor/seccomp=unconfined for bwrap namespace operations - K8s: seccompProfile + appArmorProfile on container securityContext Execd: - Probe diagnostic message in capabilities API response - bwrap path /opt/opensandbox/bwrap (alongside execd) - Shared parseUploadForm between filesystem and isolated handlers E2E tests (Python/Go/JS/Java/C#): - capabilities, lifecycle, echo, PID isolation, env injection, state persistence, /tmp isolation (strict), SSE handlers, overlay mode, file operations via run Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pangjiping
force-pushed
the
osep-0013-phase1-isolation-core
branch
from
f09f3f7 to
effa60c
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
… EOF (OSEP-0013) 1. P1: Apply deny-blacklist env defaults when EnvPassthroughMode is empty — prevents leaking execd secrets (*_TOKEN, *_SECRET) into isolated sessions 2. P1: Use path-component comparison for extra_writable allowlist validation — rejects sibling paths like /workspace/cache-escape when only /workspace/cache is allowed 3. P2: Honor timeout_seconds in Run handler — wrap context with WithTimeout when value is positive 4. P2: Detect EOF without end marker — return error instead of exit code 0 when bash process dies before outputting the marker Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
1 participant
Summary
Testing
Breaking Changes
Checklist