fix(gpg): resolve tmp file collision breaking GPG forwarding for browser IDEs - #869
Draft
skevetter wants to merge 27 commits into
Draft
fix(gpg): resolve tmp file collision breaking GPG forwarding for browser IDEs#869skevetter wants to merge 27 commits into
skevetter wants to merge 27 commits into
Conversation
Required to keep the tree building after Task 1's CreateSSHCommand signature change; full Task 2 scope (tests, browser_tunnel.go audit) is out of scope for this task.
…ith non-root remoteUser
…ustification acquireGPGSetupLock's os.Chmod(0o666) ran unconditionally, so a non-owning second acquirer (e.g. a different container user than the one that created the lock file) would get EPERM even when the mode was already correct. Skip the chmod when the file's mode already matches, and only attempt (and propagate errors from) it when a real change is needed.
Resolves revive's argument-limit without a nolint suppression: the function's 5 positional args (3 of them same-typed strings) become one sshCommandArgsParams struct, removing the transposition hazard along with the lint finding.
Reasoning is preserved (multi-user /tmp collision, umask vs chmod ownership semantics, EPERM-on-redundant-chmod) — condensed to the essential point rather than removed.
📝 WalkthroughWalkthroughThe change adds secure shared-file permission handling, applies it to GPG and activity files, propagates resolved SSH users, and adds non-root GPG forwarding tests. ChangesShared-file coordination and non-root workspace support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for devsydev canceled.
|
✅ Deploy Preview for images-devsy-sh canceled.
|
skevetter
changed the title
…ode assertion os.Chmod follows symlinks; the GPG setup lock path is a fixed, predictable, world-writable /tmp path, so a symlink planted there could redirect the chmod onto an arbitrary root-owned file. lockFileNeedsChmod now Lstats and refuses to proceed if the path is a symlink. Also compares the activity-file test's mode assertion against the literal 0o666 instead of the production constant it's meant to be checking. Addresses CodeRabbit findings on PR #869.
A lock file left behind by a pre-fix binary (or created by a different user) could sit at 0600. flock's own open() would EACCES on it before acquireGPGSetupLock's existing post-lock chmod logic ever ran, wedging a non-owning acquirer exactly like the original bug this branch fixes. widenStaleLockFile runs before the flock attempt: it chmods a restrictive existing lock file to 0666, escalating to sudo when the current process doesn't own it. A sudo failure is non-fatal — the subsequent flock attempt surfaces a clearer error if widening didn't help. Addresses the Major follow-up noted on PR #869's CodeRabbit review.
skevetter
marked this pull request as ready for review
August 3, 2026 19:18
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/internal/agentworkspace/setup_gpg.go`:
- Around line 162-189: Update widenStaleLockFile to accept a context and invoke
the sudo chmod fallback through exec.CommandContext with a bounded timeout,
including sudo’s non-interactive -n option so unavailable credentials fail
immediately. Update its call site to pass the existing context while preserving
the current non-fatal logging and error behavior.
In `@pkg/tunnel/browser_test.go`:
- Around line 66-155: Add the missing testCtxName declaration in
pkg/tunnel/browser_test.go before the helper functions baseSSHArgs and
baseParams use it, using the existing test constant/declaration style and the
context value expected by these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c3362ead-0eed-492b-ab6d-489f9e0c386c
📒 Files selected for processing (10)
cmd/internal/agentworkspace/setup_gpg.gocmd/internal/agentworkspace/setup_gpg_test.gocmd/internal/ssh_server_test.goe2e/tests/ide/browser_returns.goe2e/tests/ide/testdata-gpg-nonroot/.devcontainer.jsonpkg/gpg/forward_test.gopkg/ide/opener/browser_tunnel_test.gopkg/ide/opener/opener.gopkg/tunnel/browser.gopkg/tunnel/browser_test.go
Comment on lines
+66
to
+155
| func baseSSHArgs(ctx, user, ws string) []string { | ||
| return []string{ | ||
| "workspace", "ssh", "--user=root", "--agent-forwarding=false", | ||
| "workspace", "ssh", "--user=" + user, "--agent-forwarding=false", | ||
| "--start-services=false", "--context", ctx, ws, | ||
| } | ||
| } | ||
|
|
||
| func baseParams(user string) sshCommandArgsParams { | ||
| return sshCommandArgsParams{ | ||
| clientContext: testCtxName, workspace: testWorkspaceName, user: user, | ||
| } | ||
| } | ||
|
|
||
| func TestBuildSSHCommandArgs(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| context string | ||
| workspace string | ||
| debug bool | ||
| extraArgs []string | ||
| expected []string | ||
| name string | ||
| params sshCommandArgsParams | ||
| expected []string | ||
| }{ | ||
| { | ||
| name: "basic", context: "default", workspace: "my-workspace", | ||
| expected: baseSSHArgs("default", "my-workspace"), | ||
| name: "basic root user", | ||
| params: baseParams(testUserRoot), | ||
| expected: baseSSHArgs(testCtxName, testUserRoot, testWorkspaceName), | ||
| }, | ||
| { | ||
| name: "non-root workspace user", | ||
| params: baseParams(testUserVSCode), | ||
| expected: baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), | ||
| }, | ||
| { | ||
| name: "empty user falls back to root", | ||
| params: baseParams(""), | ||
| expected: baseSSHArgs(testCtxName, testUserRoot, testWorkspaceName), | ||
| }, | ||
| { | ||
| name: "with debug", context: "default", workspace: "my-workspace", | ||
| debug: true, | ||
| expected: append(baseSSHArgs("default", "my-workspace"), "--debug"), | ||
| name: "with debug", | ||
| params: sshCommandArgsParams{ | ||
| clientContext: testCtxName, workspace: testWorkspaceName, | ||
| user: testUserVSCode, debug: true, | ||
| }, | ||
| expected: append( | ||
| baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), "--debug", | ||
| ), | ||
| }, | ||
| { | ||
| name: "with extra args", context: "prod", workspace: "ws", | ||
| extraArgs: []string{"--stdio", "--log-output=raw"}, | ||
| expected: append(baseSSHArgs("prod", "ws"), "--stdio", "--log-output=raw"), | ||
| name: "with extra args", | ||
| params: sshCommandArgsParams{ | ||
| clientContext: "prod", workspace: "ws", user: testUserVSCode, | ||
| extraArgs: []string{"--stdio", "--log-output=raw"}, | ||
| }, | ||
| expected: append( | ||
| baseSSHArgs("prod", testUserVSCode, "ws"), "--stdio", "--log-output=raw", | ||
| ), | ||
| }, | ||
| { | ||
| name: "with debug and extra args", context: "default", workspace: "my-workspace", | ||
| debug: true, extraArgs: []string{"--stdio"}, | ||
| expected: append(baseSSHArgs("default", "my-workspace"), "--debug", "--stdio"), | ||
| name: "with debug and extra args", | ||
| params: sshCommandArgsParams{ | ||
| clientContext: testCtxName, workspace: testWorkspaceName, user: testUserVSCode, | ||
| debug: true, extraArgs: []string{"--stdio"}, | ||
| }, | ||
| expected: append( | ||
| baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), "--debug", "--stdio", | ||
| ), | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := buildSSHCommandArgs(tt.context, tt.workspace, tt.debug, tt.extraArgs) | ||
| got := buildSSHCommandArgs(tt.params) | ||
| assert.Equal(t, tt.expected, got) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestBuildBackhaulCmd_UsesResolvedRemoteUser(t *testing.T) { | ||
| writer := &bytes.Buffer{} | ||
| cmd := buildBackhaulCmd(context.Background(), backhaulCmdParams{ | ||
| execPath: "/usr/bin/true", | ||
| remoteUser: testUserVSCode, | ||
| client: fakeWorkspaceClient{}, | ||
| authSockID: "sock123", | ||
| writer: writer, | ||
| }) | ||
|
|
||
| joined := strings.Join(cmd.Args, " ") | ||
| assert.Contains(t, joined, "--user vscode", | ||
| "backhaul connection must use the resolved workspace user, not root, "+ | ||
| "so it doesn't fight the primary tunnel's ssh-server/setup-gpg sessions "+ | ||
| "over the shared /tmp coordination files") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm testCtxName is declared in pkg/tunnel/browser_test.go.
rg -n 'testCtxName' pkg/tunnel/browser_test.goRepository: devsy-org/devsy
Length of output: 780
🏁 Script executed:
#!/bin/bash
# Inspect the test file declarations and all references to testCtxName.
set -euo pipefail
echo "Declaration / reference occurrences in pkg/tunnel/browser_test.go:"
rg -n '\b(testCtxName|testUserRoot|testUserVSCode|testWorkspaceName)\b' pkg/tunnel/browser_test.go || true
echo
echo "Top of file:"
sed -n '1,110p' pkg/tunnel/browser_test.goRepository: devsy-org/devsy
Length of output: 4270
Define testCtxName before using it.
baseSSHArgs and baseParams reference testCtxName, but pkg/tunnel/browser_test.go does not declare it. Add the missing declaration to avoid a compile-time failure for this test file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/tunnel/browser_test.go` around lines 66 - 155, Add the missing
testCtxName declaration in pkg/tunnel/browser_test.go before the helper
functions baseSSHArgs and baseParams use it, using the existing test
constant/declaration style and the context value expected by these tests.
skevetter
marked this pull request as draft
August 3, 2026 19:26
Without a context or sudo -n, a sudo prompt for credentials could block acquireGPGSetupLock indefinitely, defeating gpgSetupLockTimeout entirely. exec.CommandContext ties the subprocess to the caller's existing lock timeout; -n makes sudo fail immediately instead of prompting. Addresses a CodeRabbit finding on PR #869. The paired finding (missing testCtxName in pkg/tunnel/browser_test.go) is a false positive: the constant is declared in the same package's services_test.go:13 and the package already builds and vets clean.
…s root Task 1 of this fix threaded the resolved remoteUser into startBrowserTunnelSSH's CreateSSHCommand call to align it with the GPG-forwarding tunnel's user, avoiding a collision on shared /tmp coordination files. That connection is also the one runBrowserTunnelServices/RunServices uses to read root-owned files (e.g. DevContainerResultPath, 0600 root:root) and to bootstrap the container-side ssh-server that serves the actual port-forward listener. Su-wrapping that bootstrap into remoteUser broke both: RunServices's forwardDevContainerPorts got permission denied on result.json, and the browser-tunnel's port-forward stopped surviving past the idle-timeout window — reproduced deterministically against real Docker containers, twice. The GPG lock/activity file collision this was meant to prevent is already resolved independently by making /tmp/devsy-gpg-setup.lock and /tmp/devsy.activity safe for a mixed root+remoteUser pair (see acquireGPGSetupLock's world-lockable mode and ensureActivityFile's 0666 mode) — this connection never needed to match remoteUser in the first place. Verified against real Docker containers: the previously-flaky "browser- tunnel port-forward survives past the old 5s idle timeout" e2e spec now passes consistently (5/5 local runs) with this revert; it failed consistently before it, on both the pre- and post-Task-1 code.
…Path ownership Widens /var/run/devsy/result.json to 0644 (readable by any container user; it holds no secrets, just devcontainer.json's merged config/mounts/port attributes) instead of the previous 0600 root:root, and adds a chmod to correct a stale restrictive mode left by a pre-fix binary. This removes the actual coupling behind the prior commit's "keep the primary browser-tunnel connection root" workaround: getContainerResult and credentials_server.go's portOptionsFromResult both read this file over an SSH session that can be authenticated as either root or the workspace's remoteUser (pkg/ssh/server/ssh_container.go's containerServer.handler sets the process credential directly from the authenticated session user — one identity per session, no separate privilege-drop step). With the file world-readable, startBrowserTunnelSSH's connection can use p.User again, matching the GPG-forwarding tunnel's user as originally intended, without breaking RunServices' privileged-file read. Verified against real Docker containers: the full "browser IDE returns instead of blocking" Ordered container (7 specs, including both the idle- timeout regression from the prior commit and the GPG-collision e2e test) now passes cleanly with p.User restored.
…o pkg/sharedfile A devsy container runs commands over SSH sessions authenticated as either root or the workspace's remoteUser, so any file two sessions both touch needs its permissions to survive being created by either one — the standard Unix pattern for this (like /tmp/.X11-unix or /var/run/utmp), not a workaround. Three call sites implemented that pattern's safety requirements inconsistently, having drifted apart over time: - cmd/internal/agentworkspace/setup_gpg.go (acquireGPGSetupLock): correct, stat-first chmod plus a sudo fallback for stale root-owned files — the reference implementation this refactor generalizes. - cmd/internal/agentcontainer/daemon.go (setupTimeout): unconditional os.Chmod on every call, which EPERMs when a non-owning process's activity file is already correctly moded. - cmd/internal/fleet_server.go: os.Create with no explicit mode or chmod at all, so under a typical 022 umask it silently creates the activity file at 0644 and locks out whichever user isn't its own UID. pkg/sharedfile centralizes the safety logic (create-if-missing without truncating a concurrent creator's write, stat-first chmod to avoid EPERM for a non-owning widener, Lstat + symlink rejection so a planted symlink at the fixed coordination-file path can't redirect a chmod onto an arbitrary target, and an optional non-interactive sudo fallback for the one caller that needs it). All three call sites, plus cmd/internal's own ensureActivityFile, now delegate to it instead of re-implementing it. fleet_server.go's activity-touch is extracted into ssh_server.go's new touchActivityFile, parameterized on path (was previously hardcoded to config.ContainerActivityFile, making it untestable). Verified against real Docker containers: the full "browser IDE returns instead of blocking" Ordered container (7 specs, including the idle- timeout and GPG-collision regressions from prior commits on this branch) passes cleanly with this refactor applied.
- CreateSSHCommand, WidenWithSudoFallback, touchActivityFile, and gpgSetupLockMode: trim WHY comments that had grown into WHAT restatements or cross-package implementation references. - writeResultFileTo: use sharedfile.WidenIfNeeded's umask-safe chmod instead of a fourth hand-rolled copy of the same write-then-chmod logic this session's refactor consolidated everywhere else. - containsAdjacent (browser_tunnel_test.go): restore a doc comment stating argument order, and rename needle/value to flag/value to match.
log has no dependency on pkg/sharedfile, so there's no cycle risk in calling pkg/log directly instead of threading a logFn parameter through for the one real caller.
CodeRabbit correctly flagged that WidenIfNeeded's separate Lstat-then- Chmod-by-path had a race window: a symlink planted at path between the check and the chmod would redirect the chmod onto an arbitrary target, since chmod(2) follows symlinks. The prior symlink-rejection fix only caught a symlink already in place at check time, not one swapped in after. WidenIfNeeded now opens path with O_NOFOLLOW and chmods the resulting file descriptor instead of the path — once open, the descriptor is pinned to that inode regardless of what the path later resolves to, so there's no window left to race. WidenWithSudoFallback had the same race one layer up: `sudo chmod <path>` has no way to refuse following a symlink at its target, unlike chmod's lesser-known variants for other attributes. It now re-execs `<self> internal widen-shared-file <path> <mode>` under sudo instead, so the escalated mode change still goes through WidenIfNeeded's O_NOFOLLOW open. Also fixes a related bug CodeRabbit flagged in writeResultFileTo: the unchanged-content early return skipped mode-widening entirely, so a stale restrictively-moded file with matching content never got repaired until its content next changed. Verified against real Docker containers: the full "browser IDE returns instead of blocking" Ordered container passes with these changes, including the GPG-collision spec.
syscall.O_NOFOLLOW is undefined on windows, breaking the devsy CLI's windows-latest cross-compile — every sharedfile caller (setup-gpg, the SSH server's activity file, the devcontainer result file) only ever runs inside the Linux container, never on a Windows host, but the CLI binary itself still needs to build there. Extracts the platform-specific open into openNoFollow, split supported/unsupported like the existing pkg/file convention. The windows stub errors if ever called, since nothing should reach it in practice.
…d is needed WidenIfNeeded's own open required write access to the file, even for the "mode already matches, skip the chmod" fast path — the exact case meant for a non-owning caller. A coordination-file mode like 0644 grants read to everyone but write only to the owner, so a non-owning process calling WidenIfNeeded on an already-correct 0644 file got a false permission error instead of the intended no-op. fchmod cares about ownership, not how the fd was opened, so O_RDONLY is sufficient — it only needs write access in the branch that's about to Chmod anyway, at which point ownership is what governs that regardless. This also meant WidenWithSudoFallback would escalate to sudo needlessly on every call against a 0644 file it doesn't own, even when the mode was already correct, since WidenIfNeeded's false EACCES looked identical to a real EPERM-from-wrong-mode to that caller. Currently masked in production because every real call site but writeResultFileTo's uses 0666 (write for everyone), so this only bit the one 0644 caller — and even there, by luck of that caller always running as the same user (root) on every invocation today. Found by manually re-auditing this package's file-open modes against the actual permission bits each caller uses, not by a failing test.
…nging CodeRabbit correctly flagged that openNoFollow's open could block forever: a FIFO planted at the coordination-file path with no writer hangs an O_RDONLY open indefinitely, letting any container user with write access to the parent directory (/tmp) wedge every future caller of acquireGPGSetupLock/ensureActivityFile/etc. openNoFollow now also passes O_NONBLOCK, which makes opening a FIFO return immediately instead of waiting for a writer. WidenIfNeeded then rejects any non-regular file via IsRegular() before comparing modes or chmodding — O_NONBLOCK only keeps the open itself from hanging, it doesn't stop a FIFO fd from being returned. Verified: a regression test creates a real FIFO at the coordination path and confirms WidenIfNeeded returns an error within 2s instead of hanging; fails against the pre-fix code (confirmed by temporarily reverting O_NONBLOCK), passes with it.
skevetter
marked this pull request as ready for review
August 4, 2026 01:30
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/internal/ssh_server_test.go (1)
225-238: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover stale permissions in the existing-file test.
Line 229 creates the fixture with mode
0o666, so this test cannot detect iftouchActivityFilestops widening a restrictive existing file. Start with0o600, then assert that the mode is0o666after the call. IfensureActivityFileowns this behavior, add the assertion to its existing-file test instead.Suggested regression test
- require.NoError(t, os.WriteFile(path, nil, 0o666)) + require.NoError(t, os.WriteFile(path, nil, 0o600)) require.NoError(t, os.Chtimes(path, old, old)) touchActivityFile(path) info, err := os.Stat(path) require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) assert.WithinDuration(t, time.Now(), info.ModTime(), 2*time.Second,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/internal/ssh_server_test.go` around lines 225 - 238, Update TestTouchActivityFile_UpdatesMtimeOfExistingFile to create the existing fixture with restrictive mode 0o600, then stat it after touchActivityFile and assert its permissions are 0o666 while preserving the mtime assertion. If ensureActivityFile owns the permission widening, place this regression assertion in its existing-file test instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/devcontainer/setup/setup.go`:
- Around line 224-238: Update the result-file flow around the existing
os.ReadFile, os.WriteFile, and sharedfile.WidenIfNeeded calls to use no-follow,
nonblocking file descriptors for reading, content comparison, creation/update,
and permission changes. Ensure symlink substitution and FIFO paths cannot
redirect or block the root caller, while preserving the unchanged-content
widening behavior. Add regression tests covering both symlink substitution and
FIFO inputs.
In `@pkg/sharedfile/sharedfile_unsupported.go`:
- Around line 11-14: Replace the unsupported openNoFollow implementation in
sharedfile_unsupported.go with a Windows-safe regular-file widening path,
preserving symlink safety when used by WidenIfNeeded and writeResultFileTo. Use
the platform-specific implementation or Windows APIs needed to reject symlinks
while allowing regular result files to be reopened successfully.
---
Nitpick comments:
In `@cmd/internal/ssh_server_test.go`:
- Around line 225-238: Update TestTouchActivityFile_UpdatesMtimeOfExistingFile
to create the existing fixture with restrictive mode 0o600, then stat it after
touchActivityFile and assert its permissions are 0o666 while preserving the
mtime assertion. If ensureActivityFile owns the permission widening, place this
regression assertion in its existing-file test instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 907ef724-04db-4a56-a26a-a7fd15874d22
📒 Files selected for processing (20)
cmd/internal/agentcontainer/daemon.gocmd/internal/agentworkspace/setup_gpg.gocmd/internal/agentworkspace/setup_gpg_test.gocmd/internal/fleet_server.gocmd/internal/internal.gocmd/internal/ssh_server.gocmd/internal/ssh_server_test.gocmd/internal/widen_shared_file.gocmd/internal/widen_shared_file_test.gopkg/devcontainer/setup/setup.gopkg/devcontainer/setup/setup_test.gopkg/ide/opener/browser_tunnel_test.gopkg/sharedfile/sharedfile.gopkg/sharedfile/sharedfile_supported.gopkg/sharedfile/sharedfile_supported_test.gopkg/sharedfile/sharedfile_test.gopkg/sharedfile/sharedfile_unsupported.gopkg/sharedfile/sudo.gopkg/sharedfile/sudo_test.gopkg/tunnel/browser.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/internal/agentworkspace/setup_gpg.go
- cmd/internal/agentworkspace/setup_gpg_test.go
- pkg/ide/opener/browser_tunnel_test.go
- pkg/tunnel/browser.go
Comment on lines
+224
to
+238
| existing, _ := os.ReadFile(path) | ||
| if string(rawBytes) == string(existing) { | ||
| // Widen even when skipping the write: a stale file left at a | ||
| // restrictive mode by a pre-fix binary must still get readable by | ||
| // the other session's user, not just on the next content change. | ||
| return sharedfile.WidenWithSudoFallback(context.Background(), path, 0o644) | ||
| } | ||
|
|
||
| if err := os.WriteFile(pkgconfig.DevContainerResultPath, rawBytes, 0o600); err != nil { | ||
| log.Warnf("error write result to %s: %v", pkgconfig.DevContainerResultPath, err) | ||
| if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { // #nosec G301 | ||
| return fmt.Errorf("create %s: %w", filepath.Dir(path), err) | ||
| } | ||
| if err := os.WriteFile(path, rawBytes, 0o644); err != nil { //nolint:gosec // see doc comment | ||
| return fmt.Errorf("write: %w", err) | ||
| } | ||
| return sharedfile.WidenIfNeeded(path, 0o644) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Use a no-follow, nonblocking update path for the result file.
Line 224 follows a symlink and can block on a FIFO. Line 235 also follows a symlink. If a workspace user can plant or replace this /tmp coordination path, a root caller can write to an attacker-selected target.
WidenIfNeeded runs after the unsafe write, so it cannot prevent this result. Perform the read, content check, create or update, and permission change through no-follow, nonblocking file descriptors. Add regression tests for symlink substitution and FIFOs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/devcontainer/setup/setup.go` around lines 224 - 238, Update the
result-file flow around the existing os.ReadFile, os.WriteFile, and
sharedfile.WidenIfNeeded calls to use no-follow, nonblocking file descriptors
for reading, content comparison, creation/update, and permission changes. Ensure
symlink substitution and FIFO paths cannot redirect or block the root caller,
while preserving the unchanged-content widening behavior. Add regression tests
covering both symlink substitution and FIFO inputs.
Comment on lines
+11
to
+14
| // openNoFollow has no symlink-safe open on Windows. | ||
| func openNoFollow(string) (*os.File, error) { | ||
| return nil, fmt.Errorf("sharedfile: not supported on %s", runtime.GOOS) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Provide a Windows-safe implementation.
Line 13 makes every Windows call to WidenIfNeeded fail. writeResultFileTo calls this function after each write, so the new result-file tests fail on Windows and callers receive an error after the file is created.
Implement a Windows-safe regular-file path, or use a platform-specific widening implementation that preserves the required symlink safety.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/sharedfile/sharedfile_unsupported.go` around lines 11 - 14, Replace the
unsupported openNoFollow implementation in sharedfile_unsupported.go with a
Windows-safe regular-file widening path, preserving symlink safety when used by
WidenIfNeeded and writeResultFileTo. Use the platform-specific implementation or
Windows APIs needed to reject symlinks while allowing regular result files to be
reopened successfully.
skevetter
marked this pull request as draft
August 4, 2026 01:37
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Browser-based IDE workspaces (openvscode, code-server, vscode-web, jupyter, marimo) opened two independent SSH sessions into the dev container as different OS users: the primary browser-IDE tunnel was hardcoded to
root, while the GPG-agent-forwarding tunnel correctly resolved and used the workspace'sremoteUser. Both sessions wrote to the same owner-exclusive files under/tmp(devsy-gpg-setup.lock,devsy.activity), so whichever session's user created the file first locked the other out withEACCES/EPERM— surfacing aspermission denied,operation not permitted, and ultimately "GPG agent forwarding failed ... continuing without it" for any browser IDE against a devcontainer with a non-rootremoteUser.buildSSHCommandArgs/CreateSSHCommand, plus thestartFleetcall site), so it matches the GPG-forwarding tunnel's user./tmp/devsy-gpg-setup.lockworld-lockable (0o666) as defense-in-depth, with a stat-first check so a non-owning second acquirer doesn't hit a redundant, EPERM-pronechmod.0o666.--ssh-gpg-forwardingagainst aremoteUser-set devcontainer) — independently run against real Docker containers and confirmed passing.Test plan
go build ./...,go vet ./...,go test ./...all pass.golangci-lint run --new-from-rev=<base> ./...reports 0 new issues.ide/sshlabels pass, including the new regression spec and the pre-existing GPG specs.--ide=vscode-web --ide-launch=headless --ssh-gpg-forwardingagainst aremoteUser-set devcontainer):gpg -Kinside the container lists the forwarded key with zero occurrences of the three failure substrings.Summary by CodeRabbit
Bug Fixes
Tests