style: add go fumpt formatter#550
Merged
Merged
Conversation
Signed-off-by: Samuel K <skevetter@pm.me>
📝 WalkthroughWalkthroughThis PR is a large-scale refactoring that updates octal permission literals across the codebase to Go 1.13+ syntax (0o-prefix format), refactors the platform instance lookup API to use an options struct instead of multiple parameters, adds context support to permission checks, improves error handling in lock initialization and file operations, and groups related declarations (constants, variables, types) into cohesive blocks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/image/image.go (1)
64-71:⚠️ Potential issue | 🟠 MajorUse the package keychain flow for push checks, not a hardcoded default keychain.
Line 70 uses
authn.DefaultKeychain, while this package’s image operations useGetKeychain(ctx)(Line 27 and Line 46). That mismatch can incorrectly fail push-permission checks for users whose credentials are available through your custom keychain resolution.Suggested fix
-func CheckPushPermissions(image string) error { +func CheckPushPermissions(ctx context.Context, image string) error { ref, err := name.ParseReference(image) if err != nil { - return err + return fmt.Errorf("parse image reference %q: %w", image, err) } - err = remote.CheckPushPermission(ref, authn.DefaultKeychain, http.DefaultTransport) + keychain, err := GetKeychain(ctx) + if err != nil { + return fmt.Errorf("create authentication keychain: %w", err) + } + err = remote.CheckPushPermission(ref, keychain, http.DefaultTransport) if err != nil { - return err + return fmt.Errorf("check push permission for %q: %w", image, err) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/image/image.go` around lines 64 - 71, Change CheckPushPermissions to accept a context (e.g., func CheckPushPermissions(ctx context.Context, image string) error), use GetKeychain(ctx) to obtain the keychain (instead of authn.DefaultKeychain), propagate any error from GetKeychain, and pass that keychain into remote.CheckPushPermission(ref, keychain, http.DefaultTransport); update all callers to provide a context. This ensures consistency with the other image functions that call GetKeychain(ctx) (see GetKeychain and other uses at lines ~27 and ~46) and prevents using the hardcoded authn.DefaultKeychain.pkg/daemon/platform/local_server.go (1)
377-380:⚠️ Potential issue | 🟡 MinorReturn a non-500 status when no clusters are available.
For the “no clusters allowed in project” case, returning
http.StatusInternalServerErrormakes a normal access/availability condition look like a server fault.💡 Suggested fix
} else if len(clusterList.Clusters) == 0 { err := fmt.Errorf("seems like there is no cluster allowed in project %s, please make sure to at least have a single cluster available", projectName) - http.Error(w, err.Error(), http.StatusInternalServerError) + http.Error(w, err.Error(), http.StatusForbidden) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/daemon/platform/local_server.go` around lines 377 - 380, The handler branch that checks len(clusterList.Clusters) == 0 currently returns http.StatusInternalServerError; change this to a non-500 status such as http.StatusNotFound (or another appropriate 4xx) so "no clusters allowed in project" is treated as a client/resource condition, i.e., replace the http.Error call that uses http.StatusInternalServerError with http.StatusNotFound while keeping the same error message and variables (clusterList.Clusters, projectName).pkg/daemon/platform/daemon.go (1)
75-88:⚠️ Potential issue | 🟠 MajorPrevent blocked goroutine sends on
errChaninStart.At line 75,
errChanhas capacity 1, but three goroutines send to it (lines 79, 83, 87). After theselectstatement at line 97 receives the first error and returns, the remaining two goroutines will attempt to send to a channel that no longer has a receiver, causing them to block indefinitely.This issue also appears in another function at lines 152–160, where two goroutines send to a channel with capacity 1 that only receives once.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/daemon/platform/daemon.go` around lines 75 - 88, The errChan in Start is created with capacity 1 but has three senders (the goroutines calling d.localServer.ListenAndServe, d.Listen, and d.watchNetmap) which can block after the first receiver returns; change the channel to have a buffer equal to the number of senders (e.g., errChan := make(chan error, 3) in this function) so leftover sends never block, and apply the same fix in the other function where two goroutines send to errChan (make(chan error, 2)); locate the channel creation and the goroutines by the calls to d.localServer.ListenAndServe, d.Listen(d.socketListener), and d.watchNetmap(ctx) to update the capacities accordingly.
🧹 Nitpick comments (10)
e2e/tests/up-features/up_features.go (1)
72-73: Strengthen the#nosecrationale with a concrete justification.The suppression is appropriately scoped, but the current TODO-style comment is vague. Prefer a clear, present-tense reason this write is safe in this test context (temp fixture file, non-secret content), so future audits don’t treat it as unresolved risk.
Suggested comment-only tweak
- // `#nosec` G306 -- TODO Consider using a more secure permission setting and ownership if needed. + // `#nosec` G306 -- E2E temp test fixture; file is non-sensitive and 0o644 is intentional for readability during test setup. err = os.WriteFile(path.Join(tempDir, ".devcontainer.json"), []byte(output), 0o644)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/up-features/up_features.go` around lines 72 - 73, Replace the vague TODO in the `#nosec` comment above the os.WriteFile call with a concise, concrete justification: state that this is writing a temporary test fixture (tempDir) containing non-secret, deterministic test data (.devcontainer.json) to a local ephemeral directory used only by the test run, so relaxed file permissions (0o644) are acceptable and not a security risk; keep the suppression scoped to the os.WriteFile call and include that the file is not persisted beyond the test lifecycle.pkg/platform/instance.go (1)
77-93: Reduce duplicate lookup logic between instance finders.This implementation is correct, but it now duplicates
FindInstancealmost line-for-line. Consider extracting a shared helper that takes namespace as a parameter to keep behavior aligned and reduce drift.♻️ Proposed refactor
+func findInstanceByUID(ctx context.Context, managementClient client.ManagementClient, namespace, uid string) (*managementv1.DevPodWorkspaceInstance, error) { + workspaceList, err := managementClient.Loft().ManagementV1().DevPodWorkspaceInstances(namespace).List(ctx, metav1.ListOptions{ + LabelSelector: storagev1.DevPodWorkspaceUIDLabel + "=" + uid, + }) + if err != nil { + return nil, err + } else if len(workspaceList.Items) == 0 { + return nil, nil + } + return &workspaceList.Items[0], nil +} + func FindInstance(ctx context.Context, baseClient client.Client, uid string) (*managementv1.DevPodWorkspaceInstance, error) { managementClient, err := baseClient.Management() if err != nil { return nil, fmt.Errorf("create management client: %w", err) } - - workspaceList, err := managementClient.Loft().ManagementV1().DevPodWorkspaceInstances("").List(ctx, metav1.ListOptions{ - LabelSelector: storagev1.DevPodWorkspaceUIDLabel + "=" + uid, - }) - if err != nil { - return nil, err - } else if len(workspaceList.Items) == 0 { - return nil, nil - } - - return &workspaceList.Items[0], nil + return findInstanceByUID(ctx, managementClient, "", uid) } func FindInstanceInProject(ctx context.Context, baseClient client.Client, uid, projectName string) (*managementv1.DevPodWorkspaceInstance, error) { managementClient, err := baseClient.Management() if err != nil { return nil, fmt.Errorf("create management client: %w", err) } - - workspaceList, err := managementClient.Loft().ManagementV1().DevPodWorkspaceInstances(project.ProjectNamespace(projectName)).List(ctx, metav1.ListOptions{ - LabelSelector: storagev1.DevPodWorkspaceUIDLabel + "=" + uid, - }) - if err != nil { - return nil, err - } else if len(workspaceList.Items) == 0 { - return nil, nil - } - - return &workspaceList.Items[0], nil + return findInstanceByUID(ctx, managementClient, project.ProjectNamespace(projectName), uid) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platform/instance.go` around lines 77 - 93, FindInstanceInProject duplicates the lookup logic in FindInstance; extract a shared helper (e.g., findInstanceByNamespace) that accepts ctx, baseClient client.Client, uid string, and namespace string, performs managementClient creation and the Loft DevPodWorkspaceInstance List with LabelSelector storagev1.DevPodWorkspaceUIDLabel+"="+uid, and returns (*managementv1.DevPodWorkspaceInstance, error); then modify both FindInstanceInProject and FindInstance to call this helper (passing project.ProjectNamespace(projectName) from FindInstanceInProject) to remove duplicated code and keep behavior aligned.cmd/agent/git_credentials.go (1)
97-106: Consider extracting duplicate error logging to a helper function.The error logging block (lines 97-106) is duplicated nearly verbatim in
getCredentialsFromLocalMachine(lines 114-123). Consider extracting this to a small helper to reduce duplication and centralize the log file path.Also, the
#nosec G302comment with "TODO" is somewhat contradictory—if there's a genuine concern about permissions, it should be addressed; otherwise, the TODO can be removed since0o644is a reasonable permission for an error log file.♻️ Proposed refactor to extract helper function
+const gitCredentialsErrorLog = "/tmp/git-credentials-error.log" + +func logCredentialError(source string, err error) { + // `#nosec` G302 -- Log file with standard read permissions + file, fileErr := os.OpenFile(gitCredentialsErrorLog, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + if fileErr != nil { + return + } + defer func() { _ = file.Close() }() + + _, _ = fmt.Fprintf(file, "get credentials from %s: %v\n", source, err) +}Then in both functions:
if credentialsErr != nil { - // `#nosec` G302 -- TODO Consider using a more secure permission setting and ownership if needed. - file, err := os.OpenFile("/tmp/git-credentials-error.log", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) - if err != nil { - return nil - } - defer func() { _ = file.Close() }() - - _, _ = fmt.Fprintf(file, "get credentials from workspace server: %v\n", credentialsErr) + logCredentialError("workspace server", credentialsErr) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agent/git_credentials.go` around lines 97 - 106, Duplicate error logging to /tmp/git-credentials-error.log appears in the getCredentialsFromWorkspaceServer and getCredentialsFromLocalMachine flows; extract that block into a small helper (e.g., writeGitCredentialsError(err error) or logGitCredentialsError(credentialsErr error)) that opens/appends the file, writes the formatted message, closes the file, and returns any internal errors as needed; centralize the log path in the helper (remove or clarify the `#nosec` TODO by either addressing permission concerns there or deleting the TODO) and replace the duplicated blocks in both functions with calls to the new helper to reduce duplication and keep file-permission logic in one place.e2e/tests/up/dockerfile_build.go (2)
111-112: Inconsistent security annotations.This
os.OpenFilecall follows the same pattern as line 70 (path constructed with a variable), yet it lacks the//nolint:gosecdirective. If security annotations are warranted for line 70, they should be applied consistently here as well to suppress the same G304 warning.Suggested fix for consistency
scriptFile, err := os.OpenFile(tempDir+"/scripts/install.sh", - os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) + os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) //nolint:gosec // G304: path is constructed from test fixtures🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/up/dockerfile_build.go` around lines 111 - 112, The os.OpenFile call that opens tempDir+"/scripts/install.sh" is missing the same //nolint:gosec annotation used on the earlier os.OpenFile that constructs a path from a variable; add the matching //nolint:gosec comment immediately above this os.OpenFile invocation to consistently suppress the G304 warning for this variable-based path construction (reference the os.OpenFile call that uses tempDir+"/scripts/install.sh" to locate the change).
69-70: Misleading comment and potentially redundant directives.The
G304rule flags potential file inclusion via variable (path traversal), not file permission issues. The comment text referencing "more secure permission setting" is misleading for this rule.Additionally, using both
//#nosecG304and//nolint:gosecmay be redundant—#nosecis for standalone gosec, while//nolint:gosecis for golangci-lint's gosec integration. Consider using only the directive that matches your linter toolchain.Suggested fix (if using golangci-lint)
- // `#nosec` G304 -- TODO Consider using a more secure permission setting and ownership if needed. - scriptFile, err := os.OpenFile(scriptPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) //nolint:gosec + scriptFile, err := os.OpenFile(scriptPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) //nolint:gosec // G304: path is constructed from test fixtures🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/up/dockerfile_build.go` around lines 69 - 70, The inline security comment for the os.OpenFile call is misleading and the suppression directives are redundant; update the comment to correctly justify suppressing G304 (e.g., explain why scriptPath is validated/sanitized) and remove one of the suppressors so only the one your toolchain uses remains (keep either "// `#nosec` G304" for gosec or "//nolint:gosec" for golangci-lint). Concretely, edit the os.OpenFile(scriptPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) site: replace the "TODO Consider using a more secure permission setting..." text with a short note that scriptPath is validated/controlled, and delete the redundant directive so only the appropriate linter suppression is present alongside the justification comment.pkg/gitsshsigning/client.go (1)
57-58: Replace TODO-based#nosecwith a concrete, permanent rationale (or tighten perms).The suppression currently ships with an open TODO. Please either use a stricter mode (for example
0o600) and drop suppression, or keep0o644with a clear justification that this signature artifact is intentionally non-sensitive.♻️ Suggested adjustment
- // `#nosec` G306 -- TODO Consider using a more secure permission setting and ownership if needed. + // `#nosec` G306 -- Detached signatures are non-secret artifacts; read access is intentional. if err := os.WriteFile(sigFile, signature, 0o644); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gitsshsigning/client.go` around lines 57 - 58, The current os.WriteFile call (os.WriteFile(sigFile, signature, 0o644)) is suppressed with a TODO `#nosec` G306; tighten the file mode to 0o600 and remove the `#nosec` suppression so the signature file is written with owner-read/write only. Locate the write in pkg/gitsshsigning (look for sigFile and the os.WriteFile call) and replace 0o644 with 0o600, delete the `#nosec` comment, and run any relevant tests; if you cannot tighten permissions for a specific reason instead retain 0o644 but replace the TODO `#nosec` with a short permanent rationale explaining why the signature artifact is non-sensitive and must be world-readable.pkg/image/image.go (1)
64-64: Add a doc comment for the new exported function.
CheckPushPermissionsis public, so a short Go doc comment will keepgolint/API docs clean and self-explanatory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/image/image.go` at line 64, Add a Go doc comment for the exported function CheckPushPermissions that begins with the function name and succinctly describes what the function does, the meaning of its input (image string) and what the returned error represents; update the comment immediately above the CheckPushPermissions declaration so it satisfies golint/API docs (e.g., "CheckPushPermissions checks whether the current user has permission to push the specified image and returns an error if not.").pkg/client/clientimplementation/daemonclient/up.go (1)
310-314: Wire new task-type constants into active-task handling paths.The constants
TaskTypeStopandTaskTypeDeleteare defined but unused. TheUpflow checks for active up tasks viafindActiveUpTask(line 316) before creating new ones, butStop(stop.go:14) andDelete(delete.go:16) skip this reconciliation entirely—calling their API methods directly without checking for existing active tasks. Consider refactoring to use a sharedfindActiveTaskByType(...)function and apply it to all three operations to ensure consistent behavior and avoid potential duplicate in-flight task creation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/clientimplementation/daemonclient/up.go` around lines 310 - 314, The new TaskTypeStop and TaskTypeDelete constants are defined but not used, causing Stop and Delete flows to bypass the active-task reconciliation that Up uses; create a shared function findActiveTaskByType(taskType string, ... ) (or refactor findActiveUpTask into findActiveTaskByType) and replace the direct API calls in the Stop and Delete handlers (e.g., the Stop and Delete functions that currently call API methods directly) to first call findActiveTaskByType(TaskTypeStop) and findActiveTaskByType(TaskTypeDelete) respectively, and only create a new task when no active task exists; update the Up flow to call the new shared function as well so all three operations use the same reconciliation logic.pkg/ssh/keys.go (1)
85-87: Harden key directory permissions.For directories storing private keys, prefer
0o700over0o755to reduce metadata exposure and traversal by other users.🔐 Proposed refactor
- err = os.MkdirAll(tempDir, 0o755) + err = os.MkdirAll(tempDir, 0o700)- err := os.MkdirAll(dir, 0o755) + err := os.MkdirAll(dir, 0o700)Also applies to: 124-126, 165-167, 199-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ssh/keys.go` around lines 85 - 87, Change directory creation for private key storage from world-readable 0o755 to restrictive 0o700: update the os.MkdirAll calls that create tempDir and the other key-storage directories in pkg/ssh/keys.go to use 0o700, and, to be safe, call os.Chmod(dirPath, 0o700) after creation to ensure permissions are applied; also adjust or remove the accompanying "#nosec G301" comment so it no longer suppresses the security warning for these hardening changes.pkg/devcontainer/setup/setup.go (1)
173-174: Consider tightening/homedirectory permissions.Creating
/homewith0o777is overly permissive. Standard Unix systems typically use0o755for/home. While the#noseccomment and TODO acknowledge this, consider changing to0o755now since world-writable directories can be a security risk (e.g., symlink attacks).♻️ Proposed fix
- // `#nosec` G301 -- TODO Consider using a more secure permission setting and ownership if needed. - err = os.MkdirAll("/home", 0o777) + // `#nosec` G301 -- Standard /home directory permissions + err = os.MkdirAll("/home", 0o755)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainer/setup/setup.go` around lines 173 - 174, Change the overly-permissive directory creation call that uses os.MkdirAll("/home", 0o777) to create /home with tighter permissions (use 0o755) and update or remove the `#nosec` G301 TODO if no longer needed; locate the os.MkdirAll call in setup.go (the call creating "/home") and replace the mode argument with 0o755, and if appropriate set ownership afterward or leave a clear TODO explaining why ownership changes are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/agent/container/daemon.go`:
- Around line 181-186: The activity file is being created/chmod'ed with
world-readable/writable/executable permissions (0o777); update the os.WriteFile
and os.Chmod calls that operate on agent.ContainerActivityFile to use
least-privilege permissions instead (e.g., 0o600 for single-user or 0o666 if
multiple users must write) and remove the execute bit; ensure the chosen
permission is applied in both the os.WriteFile and os.Chmod calls and consider
setting proper ownership if multi-user access is required.
In `@e2e/tests/provider/provider.go`:
- Around line 111-112: The test creates a directory named ".DS_Store" using
os.Mkdir but the intent is to add a macOS .DS_Store file; change the code that
calls os.Mkdir(tempDir+"/.DS_Store", 0o755) to create a regular file instead
(e.g., use os.Create or os.WriteFile against tempDir+"/.DS_Store" with
appropriate file mode and error handling) and update or remove the misleading
`#nosec` G301 comment; ensure the code writes an empty or stubbed .DS_Store
content and properly closes/handles the file error.
In `@pkg/agent/workspace.go`:
- Around line 146-147: Change the overly permissive os.MkdirAll(dir, 0o777) call
in workspace.go to create the probe directory with least-privilege permissions
(e.g., 0o755) and remove the `#nosec` suppression; update the os.MkdirAll
invocation that uses the variable dir to use 0o755, and if you need to adjust
ownership or tighten permissions after creation use os.Chmod or chown on that
same dir variable so security is enforced without suppressing the scanner.
In `@pkg/client/clientimplementation/proxy_client.go`:
- Around line 170-171: The mkdir call creates workspaceLocksDir with
overly-permissive 0o777 and ignores errors; change the permission to a
restrictive mode (e.g., 0o750 or 0o700) and handle the returned error from
os.MkdirAll(workspaceLocksDir, ...): if it fails, return or log the error and
abort initialization so lock integrity is preserved; update the code that
references workspaceLocksDir and os.MkdirAll to check the error and
remove/comment the // `#nosec` G301 only if you have justified the permission
change.
In `@pkg/client/clientimplementation/workspace_client.go`:
- Line 250: The call to os.MkdirAll(workspaceLocksDir, 0o777) currently discards
its error; change it to capture and handle/propagate the error (e.g., if err :=
os.MkdirAll(...); err != nil { return err } or panic consistent with the
existing GetLocksDir panic) so failures creating workspaceLocksDir surface
immediately; update the surrounding function (the code that calls GetLocksDir
and uses workspaceLocksDir / lock file ops) to return or propagate that error
instead of proceeding when MkdirAll fails.
- Around line 249-250: The workspace locks directory is created with overly
permissive 0o777 via os.MkdirAll(workspaceLocksDir, 0o777); change the mode to a
stricter permission such as 0o700 (or 0o755 if group read/execute is required)
and handle the returned error instead of discarding it—call os.MkdirAll(...),
check the error, and propagate or log it appropriately from the surrounding
function (identify the os.MkdirAll(workspaceLocksDir, ...) call and update it),
and remove or update the `#nosec` comment once fixed.
In `@pkg/envfile/envfile.go`:
- Around line 71-72: Change the permissive file mode used when writing the
envfile: replace os.WriteFile(location, out, 0o666) with a restrictive mode such
as 0o600 (owner read/write) to prevent unintended writes; if you need
group-readable access, consider 0o640 instead. Update the call that writes the
file (the os.WriteFile invocation using variables location and out in
pkg/envfile/envfile.go) to use the new mode, and if ownership/umask concerns
remain, open the file with os.OpenFile and explicitly set permissions with Chmod
after creation. Ensure the change preserves the same variables (location, out)
so callers remain unchanged.
In `@pkg/extract/extract.go`:
- Around line 158-159: The inline nolint on the os.Chmod call is using the wrong
gosec rule/guidance: update the comment above the os.Chmod(outFileName,
header.FileInfo().Mode()|0o600) call by replacing "nolint:gosec // G703: Path
traversal is handled by archive extraction context" with the correct rule and
justification—if you intend to suppress the path traversal warning use
"nolint:gosec // G305: Path traversal is handled by archive extraction context",
otherwise if you intended to suppress the ignored error return use "nolint:gosec
// G104: intentionally ignoring chmod error" so the rule ID matches the
justification.
In `@pkg/ide/fleet/fleet.go`:
- Around line 98-100: The guard currently checks the full path stored in
fleetBinary with strings.HasPrefix, which fails for absolute paths; change the
check to operate on the basename: use filepath.Base(fleetBinary) (e.g.
strings.HasPrefix(filepath.Base(fleetBinary), "fleet") or compare equality to
"fleet") in the same condition that returns the fmt.Errorf, and add the filepath
import if missing so the validation works for absolute and relative paths.
- Around line 181-182: The code writes a fixed file name (FleetURLFile) in /tmp
via os.WriteFile which is vulnerable to symlink/clobber attacks; replace this
with a safe temporary-file-then-rename pattern or use a per-user
non-world-writable directory: use os.CreateTemp (or CreateTemp with
os.UserCacheDir/os.UserConfigDir) to create a uniquely-named temp file, write
the trimmed content to that temp file (ensure you fsync and set 0o600 perms via
file.Chmod), close it, then atomically os.Rename the temp file to FleetURLFile
(or better: choose a user-specific directory instead of /tmp) to avoid
TOCTOU/symlink issues; update the code around FleetURLFile and the current
os.WriteFile call accordingly.
In `@pkg/provider/dir.go`:
- Around line 267-268: The workspace config is being written with insecure 0o644
permissions; update the os.WriteFile call that writes workspaceConfigFile using
workspaceConfigBytes to use 0o600 (owner read/write only) to match the other
config files (provider, pro instance, result, machine) and remove or update the
TODO/nosec comment accordingly (or if you intentionally need world-readable, add
a clear inline comment explaining why and leave 0o644).
---
Outside diff comments:
In `@pkg/daemon/platform/daemon.go`:
- Around line 75-88: The errChan in Start is created with capacity 1 but has
three senders (the goroutines calling d.localServer.ListenAndServe, d.Listen,
and d.watchNetmap) which can block after the first receiver returns; change the
channel to have a buffer equal to the number of senders (e.g., errChan :=
make(chan error, 3) in this function) so leftover sends never block, and apply
the same fix in the other function where two goroutines send to errChan
(make(chan error, 2)); locate the channel creation and the goroutines by the
calls to d.localServer.ListenAndServe, d.Listen(d.socketListener), and
d.watchNetmap(ctx) to update the capacities accordingly.
In `@pkg/daemon/platform/local_server.go`:
- Around line 377-380: The handler branch that checks len(clusterList.Clusters)
== 0 currently returns http.StatusInternalServerError; change this to a non-500
status such as http.StatusNotFound (or another appropriate 4xx) so "no clusters
allowed in project" is treated as a client/resource condition, i.e., replace the
http.Error call that uses http.StatusInternalServerError with
http.StatusNotFound while keeping the same error message and variables
(clusterList.Clusters, projectName).
In `@pkg/image/image.go`:
- Around line 64-71: Change CheckPushPermissions to accept a context (e.g., func
CheckPushPermissions(ctx context.Context, image string) error), use
GetKeychain(ctx) to obtain the keychain (instead of authn.DefaultKeychain),
propagate any error from GetKeychain, and pass that keychain into
remote.CheckPushPermission(ref, keychain, http.DefaultTransport); update all
callers to provide a context. This ensures consistency with the other image
functions that call GetKeychain(ctx) (see GetKeychain and other uses at lines
~27 and ~46) and prevents using the hardcoded authn.DefaultKeychain.
---
Nitpick comments:
In `@cmd/agent/git_credentials.go`:
- Around line 97-106: Duplicate error logging to /tmp/git-credentials-error.log
appears in the getCredentialsFromWorkspaceServer and
getCredentialsFromLocalMachine flows; extract that block into a small helper
(e.g., writeGitCredentialsError(err error) or
logGitCredentialsError(credentialsErr error)) that opens/appends the file,
writes the formatted message, closes the file, and returns any internal errors
as needed; centralize the log path in the helper (remove or clarify the `#nosec`
TODO by either addressing permission concerns there or deleting the TODO) and
replace the duplicated blocks in both functions with calls to the new helper to
reduce duplication and keep file-permission logic in one place.
In `@e2e/tests/up-features/up_features.go`:
- Around line 72-73: Replace the vague TODO in the `#nosec` comment above the
os.WriteFile call with a concise, concrete justification: state that this is
writing a temporary test fixture (tempDir) containing non-secret, deterministic
test data (.devcontainer.json) to a local ephemeral directory used only by the
test run, so relaxed file permissions (0o644) are acceptable and not a security
risk; keep the suppression scoped to the os.WriteFile call and include that the
file is not persisted beyond the test lifecycle.
In `@e2e/tests/up/dockerfile_build.go`:
- Around line 111-112: The os.OpenFile call that opens
tempDir+"/scripts/install.sh" is missing the same //nolint:gosec annotation used
on the earlier os.OpenFile that constructs a path from a variable; add the
matching //nolint:gosec comment immediately above this os.OpenFile invocation to
consistently suppress the G304 warning for this variable-based path construction
(reference the os.OpenFile call that uses tempDir+"/scripts/install.sh" to
locate the change).
- Around line 69-70: The inline security comment for the os.OpenFile call is
misleading and the suppression directives are redundant; update the comment to
correctly justify suppressing G304 (e.g., explain why scriptPath is
validated/sanitized) and remove one of the suppressors so only the one your
toolchain uses remains (keep either "// `#nosec` G304" for gosec or
"//nolint:gosec" for golangci-lint). Concretely, edit the
os.OpenFile(scriptPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) site:
replace the "TODO Consider using a more secure permission setting..." text with
a short note that scriptPath is validated/controlled, and delete the redundant
directive so only the appropriate linter suppression is present alongside the
justification comment.
In `@pkg/client/clientimplementation/daemonclient/up.go`:
- Around line 310-314: The new TaskTypeStop and TaskTypeDelete constants are
defined but not used, causing Stop and Delete flows to bypass the active-task
reconciliation that Up uses; create a shared function
findActiveTaskByType(taskType string, ... ) (or refactor findActiveUpTask into
findActiveTaskByType) and replace the direct API calls in the Stop and Delete
handlers (e.g., the Stop and Delete functions that currently call API methods
directly) to first call findActiveTaskByType(TaskTypeStop) and
findActiveTaskByType(TaskTypeDelete) respectively, and only create a new task
when no active task exists; update the Up flow to call the new shared function
as well so all three operations use the same reconciliation logic.
In `@pkg/devcontainer/setup/setup.go`:
- Around line 173-174: Change the overly-permissive directory creation call that
uses os.MkdirAll("/home", 0o777) to create /home with tighter permissions (use
0o755) and update or remove the `#nosec` G301 TODO if no longer needed; locate the
os.MkdirAll call in setup.go (the call creating "/home") and replace the mode
argument with 0o755, and if appropriate set ownership afterward or leave a clear
TODO explaining why ownership changes are required.
In `@pkg/gitsshsigning/client.go`:
- Around line 57-58: The current os.WriteFile call (os.WriteFile(sigFile,
signature, 0o644)) is suppressed with a TODO `#nosec` G306; tighten the file mode
to 0o600 and remove the `#nosec` suppression so the signature file is written with
owner-read/write only. Locate the write in pkg/gitsshsigning (look for sigFile
and the os.WriteFile call) and replace 0o644 with 0o600, delete the `#nosec`
comment, and run any relevant tests; if you cannot tighten permissions for a
specific reason instead retain 0o644 but replace the TODO `#nosec` with a short
permanent rationale explaining why the signature artifact is non-sensitive and
must be world-readable.
In `@pkg/image/image.go`:
- Line 64: Add a Go doc comment for the exported function CheckPushPermissions
that begins with the function name and succinctly describes what the function
does, the meaning of its input (image string) and what the returned error
represents; update the comment immediately above the CheckPushPermissions
declaration so it satisfies golint/API docs (e.g., "CheckPushPermissions checks
whether the current user has permission to push the specified image and returns
an error if not.").
In `@pkg/platform/instance.go`:
- Around line 77-93: FindInstanceInProject duplicates the lookup logic in
FindInstance; extract a shared helper (e.g., findInstanceByNamespace) that
accepts ctx, baseClient client.Client, uid string, and namespace string,
performs managementClient creation and the Loft DevPodWorkspaceInstance List
with LabelSelector storagev1.DevPodWorkspaceUIDLabel+"="+uid, and returns
(*managementv1.DevPodWorkspaceInstance, error); then modify both
FindInstanceInProject and FindInstance to call this helper (passing
project.ProjectNamespace(projectName) from FindInstanceInProject) to remove
duplicated code and keep behavior aligned.
In `@pkg/ssh/keys.go`:
- Around line 85-87: Change directory creation for private key storage from
world-readable 0o755 to restrictive 0o700: update the os.MkdirAll calls that
create tempDir and the other key-storage directories in pkg/ssh/keys.go to use
0o700, and, to be safe, call os.Chmod(dirPath, 0o700) after creation to ensure
permissions are applied; also adjust or remove the accompanying "#nosec G301"
comment so it no longer suppresses the security warning for these hardening
changes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (88)
.golangci.yamlcmd/agent/container/daemon.gocmd/agent/git_credentials.gocmd/agent/workspace/up.gocmd/helper/check_provider_update.gocmd/helper/fleet_helper.gocmd/import.gocmd/pro/login.gocmd/pro/start.gocmd/profile.gocmd/ssh.gocmd/version.goe2e/framework/util.goe2e/tests/context/context.goe2e/tests/integration/integration.goe2e/tests/provider/provider.goe2e/tests/up-docker-compose/build.goe2e/tests/up-docker-compose/config.goe2e/tests/up-features/up_features.goe2e/tests/up-features/wsl.goe2e/tests/up/dockerfile_build.goe2e/tests/up/provider_podman.gohack/pro/main.gopkg/agent/agent.gopkg/agent/binary.gopkg/agent/inject.gopkg/agent/tunnelserver/tunnelserver.gopkg/agent/workspace.gopkg/client/clientimplementation/daemonclient/form.gopkg/client/clientimplementation/daemonclient/up.gopkg/client/clientimplementation/proxy_client.gopkg/client/clientimplementation/workspace_client.gopkg/config/config.gopkg/copy/copy.gopkg/credentials/server.gopkg/daemon/platform/daemon.gopkg/daemon/platform/error.gopkg/daemon/platform/local_server.gopkg/devcontainer/build.gopkg/devcontainer/build/options.gopkg/devcontainer/compose.gopkg/devcontainer/config/parse.gopkg/devcontainer/config/parse_test.gopkg/devcontainer/config/prebuild_test.gopkg/devcontainer/crane/run.gopkg/devcontainer/feature/extend.gopkg/devcontainer/feature/features.gopkg/devcontainer/graph/graph_test.gopkg/devcontainer/setup/setup.gopkg/devcontainer/sshtunnel/sshtunnel.gopkg/dockercredentials/dockercredentials.gopkg/dockercredentials/helper.gopkg/driver/docker/docker.gopkg/driver/kubernetes/daemonsecret.gopkg/driver/kubernetes/pullsecrets.gopkg/driver/kubernetes/run.gopkg/envfile/envfile.gopkg/extract/compress.gopkg/extract/extract.gopkg/gitcredentials/gitcredentials.gopkg/gitsshsigning/client.gopkg/gitsshsigning/helper.gopkg/http/http.gopkg/ide/fleet/fleet.gopkg/ide/openvscode/openvscode.gopkg/ide/vscode/vscode.gopkg/image/image.gopkg/inject/download_urls.gopkg/options/resolve_test.gopkg/options/resolver/resolve_test.gopkg/platform/form/form.gopkg/platform/instance.gopkg/platform/project/project.gopkg/port/port.gopkg/provider/dir.gopkg/provider/download.gopkg/ssh/config.gopkg/ssh/keys.gopkg/ssh/server/agent.gopkg/ssh/server/exec.gopkg/ssh/server/port/port.gopkg/ssh/server/pty_unsupported.gopkg/stdio/listener.gopkg/template/fill.gopkg/types/types.gopkg/util/hash/hash.gopkg/util/hash/hash_test.gopkg/workspace/provider.go
💤 Files with no reviewable changes (8)
- e2e/tests/context/context.go
- pkg/port/port.go
- pkg/ssh/server/exec.go
- e2e/tests/up-docker-compose/config.go
- pkg/ssh/server/pty_unsupported.go
- pkg/ssh/server/port/port.go
- pkg/stdio/listener.go
- pkg/agent/inject.go
Comment on lines
+146
to
+147
| // #nosec G301,G703 -- TODO Consider using a more secure permission setting and ownership if needed. | ||
| err := os.MkdirAll(dir, 0o777) |
There was a problem hiding this comment.
Avoid world-writable permissions for probe directory creation.
Line 147 uses 0o777, which is overly permissive for this check and weakens security posture. Prefer least-privilege mode (for example 0o755) and avoid suppressing this with #nosec.
Proposed change
- // `#nosec` G301,G703 -- TODO Consider using a more secure permission setting and ownership if needed.
- err := os.MkdirAll(dir, 0o777)
+ err := os.MkdirAll(dir, 0o755)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // #nosec G301,G703 -- TODO Consider using a more secure permission setting and ownership if needed. | |
| err := os.MkdirAll(dir, 0o777) | |
| err := os.MkdirAll(dir, 0o755) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/agent/workspace.go` around lines 146 - 147, Change the overly permissive
os.MkdirAll(dir, 0o777) call in workspace.go to create the probe directory with
least-privilege permissions (e.g., 0o755) and remove the `#nosec` suppression;
update the os.MkdirAll invocation that uses the variable dir to use 0o755, and
if you need to adjust ownership or tighten permissions after creation use
os.Chmod or chown on that same dir variable so security is enforced without
suppressing the scanner.
Comment on lines
+170
to
+171
| // #nosec G301 -- TODO Consider using a more secure permission setting and ownership if needed. | ||
| _ = os.MkdirAll(workspaceLocksDir, 0o777) |
There was a problem hiding this comment.
Lock directory permissions are too broad and mkdir failure is ignored.
Line 171 uses 0o777 and discards the error, which weakens lock integrity and can hide initialization failures.
Suggested change
- // `#nosec` G301 -- TODO Consider using a more secure permission setting and ownership if needed.
- _ = os.MkdirAll(workspaceLocksDir, 0o777)
+ if err := os.MkdirAll(workspaceLocksDir, 0o700); err != nil {
+ panic(fmt.Errorf("create workspace locks dir: %w", err))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // #nosec G301 -- TODO Consider using a more secure permission setting and ownership if needed. | |
| _ = os.MkdirAll(workspaceLocksDir, 0o777) | |
| if err := os.MkdirAll(workspaceLocksDir, 0o700); err != nil { | |
| panic(fmt.Errorf("create workspace locks dir: %w", err)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client/clientimplementation/proxy_client.go` around lines 170 - 171, The
mkdir call creates workspaceLocksDir with overly-permissive 0o777 and ignores
errors; change the permission to a restrictive mode (e.g., 0o750 or 0o700) and
handle the returned error from os.MkdirAll(workspaceLocksDir, ...): if it fails,
return or log the error and abort initialization so lock integrity is preserved;
update the code that references workspaceLocksDir and os.MkdirAll to check the
error and remove/comment the // `#nosec` G301 only if you have justified the
permission change.
Comment on lines
+71
to
+72
| // #nosec G306 -- TODO Consider using a more secure permission setting and ownership if needed. | ||
| err = os.WriteFile(location, out, 0o666) |
There was a problem hiding this comment.
Avoid permissive write mode for /etc/envfile.json.
0o666 can allow unintended write access depending on runtime umask and ownership. This file can influence process environment and should be restricted.
🔒 Proposed fix
- // `#nosec` G306 -- TODO Consider using a more secure permission setting and ownership if needed.
- err = os.WriteFile(location, out, 0o666)
+ err = os.WriteFile(location, out, 0o600)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // #nosec G306 -- TODO Consider using a more secure permission setting and ownership if needed. | |
| err = os.WriteFile(location, out, 0o666) | |
| err = os.WriteFile(location, out, 0o600) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/envfile/envfile.go` around lines 71 - 72, Change the permissive file mode
used when writing the envfile: replace os.WriteFile(location, out, 0o666) with a
restrictive mode such as 0o600 (owner read/write) to prevent unintended writes;
if you need group-readable access, consider 0o640 instead. Update the call that
writes the file (the os.WriteFile invocation using variables location and out in
pkg/envfile/envfile.go) to use the new mode, and if ownership/umask concerns
remain, open the file with os.OpenFile and explicitly set permissions with Chmod
after creation. Ensure the change preserves the same variables (location, out)
so callers remain unchanged.
Comment on lines
+158
to
+159
| //nolint:gosec // G703: Path traversal is handled by archive extraction context | ||
| _ = os.Chmod(outFileName, header.FileInfo().Mode()|0o600) |
There was a problem hiding this comment.
Mismatched gosec rule number and justification.
The nolint comment references G703 (deferred error handling) but the justification mentions "Path traversal" which corresponds to G305. For suppressing path traversal warnings, the rule should be G305. If the intent is to suppress the ignored error return from os.Chmod, that would typically be G104.
Proposed fix (if suppressing path traversal)
- //nolint:gosec // G703: Path traversal is handled by archive extraction context
+ //nolint:gosec // G305: Path traversal is handled by archive extraction context📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| //nolint:gosec // G703: Path traversal is handled by archive extraction context | |
| _ = os.Chmod(outFileName, header.FileInfo().Mode()|0o600) | |
| //nolint:gosec // G305: Path traversal is handled by archive extraction context | |
| _ = os.Chmod(outFileName, header.FileInfo().Mode()|0o600) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/extract/extract.go` around lines 158 - 159, The inline nolint on the
os.Chmod call is using the wrong gosec rule/guidance: update the comment above
the os.Chmod(outFileName, header.FileInfo().Mode()|0o600) call by replacing
"nolint:gosec // G703: Path traversal is handled by archive extraction context"
with the correct rule and justification—if you intend to suppress the path
traversal warning use "nolint:gosec // G305: Path traversal is handled by
archive extraction context", otherwise if you intended to suppress the ignored
error return use "nolint:gosec // G104: intentionally ignoring chmod error" so
the rule ID matches the justification.
Comment on lines
+98
to
+100
| if !strings.HasPrefix(fleetBinary, "fleet") { | ||
| return fmt.Errorf("unexpected fleet binary name: %s", fleetBinary) | ||
| } |
There was a problem hiding this comment.
Use filepath.Base for this guard; current check breaks installs.
At Line 98, strings.HasPrefix(fleetBinary, "fleet") checks the full path (e.g. /home/user/.fleet-server/fleet), so it fails for normal absolute paths and returns an error on every install.
🐛 Proposed fix
- if !strings.HasPrefix(fleetBinary, "fleet") {
- return fmt.Errorf("unexpected fleet binary name: %s", fleetBinary)
- }
+ binaryName := filepath.Base(fleetBinary)
+ if !strings.HasPrefix(binaryName, "fleet") {
+ return fmt.Errorf("unexpected fleet binary name: %s", binaryName)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ide/fleet/fleet.go` around lines 98 - 100, The guard currently checks the
full path stored in fleetBinary with strings.HasPrefix, which fails for absolute
paths; change the check to operate on the basename: use
filepath.Base(fleetBinary) (e.g. strings.HasPrefix(filepath.Base(fleetBinary),
"fleet") or compare equality to "fleet") in the same condition that returns the
fmt.Errorf, and add the filepath import if missing so the validation works for
absolute and relative paths.
Comment on lines
+181
to
+182
| //nolint:gosec // G703: FleetURLFile path is controlled by the application | ||
| err = os.WriteFile(FleetURLFile, []byte(strings.TrimSpace(text[index:])), 0o600) |
There was a problem hiding this comment.
Avoid writing to a fixed /tmp filename; this is symlink/clobber-prone.
At Line 182, writing to /tmp/devpod-fleet.url.txt via os.WriteFile allows a local attacker to pre-create a symlink at that path. 0o600 does not mitigate that path-level risk.
🔐 Safer direction
-const (
- FleetURLFile = "/tmp/devpod-fleet.url.txt"
-)
+const (
+ FleetURLFileName = "devpod-fleet.url.txt"
+)
...
- err = os.WriteFile(FleetURLFile, []byte(strings.TrimSpace(text[index:])), 0o600)
+ fleetURLFile := filepath.Join(location, FleetURLFileName)
+ err = os.WriteFile(fleetURLFile, []byte(strings.TrimSpace(text[index:])), 0o600)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ide/fleet/fleet.go` around lines 181 - 182, The code writes a fixed file
name (FleetURLFile) in /tmp via os.WriteFile which is vulnerable to
symlink/clobber attacks; replace this with a safe temporary-file-then-rename
pattern or use a per-user non-world-writable directory: use os.CreateTemp (or
CreateTemp with os.UserCacheDir/os.UserConfigDir) to create a uniquely-named
temp file, write the trimmed content to that temp file (ensure you fsync and set
0o600 perms via file.Chmod), close it, then atomically os.Rename the temp file
to FleetURLFile (or better: choose a user-specific directory instead of /tmp) to
avoid TOCTOU/symlink issues; update the code around FleetURLFile and the current
os.WriteFile call accordingly.
Comment on lines
+267
to
+268
| // #nosec G306 -- TODO Consider using a more secure permission setting and ownership if needed. | ||
| err = os.WriteFile(workspaceConfigFile, workspaceConfigBytes, 0o644) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check permissions and comments around different config files in dir.go
rg -n -B5 -A5 'WriteFile.*0o[0-9]+' pkg/provider/dir.goRepository: skevetter/devpod
Length of output: 1460
🏁 Script executed:
#!/bin/bash
# Check for WorkspaceConfigFile definition and usage
rg -n -B3 -A3 'WorkspaceConfigFile|workspace\.json' pkg/provider/dir.goRepository: skevetter/devpod
Length of output: 787
🏁 Script executed:
#!/bin/bash
# Check all file permission modes used in the file
rg -n '0o[0-9]{3,4}' pkg/provider/dir.goRepository: skevetter/devpod
Length of output: 625
Consider using 0o600 for workspace config to match other config files.
All other config files in this file (provider, pro instance, result, machine) use 0o600 (owner read/write only), but workspace.json uses 0o644 (world-readable). The existing TODO comment in the code acknowledges this may not be sufficiently secure. Either document why workspace.json requires world-readable permissions, or change it to 0o600 for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/provider/dir.go` around lines 267 - 268, The workspace config is being
written with insecure 0o644 permissions; update the os.WriteFile call that
writes workspaceConfigFile using workspaceConfigBytes to use 0o600 (owner
read/write only) to match the other config files (provider, pro instance,
result, machine) and remove or update the TODO/nosec comment accordingly (or if
you intentionally need world-readable, add a clear inline comment explaining why
and leave 0o644).
Signed-off-by: GitHub <noreply@github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pkg/ide/fleet/fleet.go (1)
25-25:⚠️ Potential issue | 🟠 Major
FleetURLFileNamestill resolves to shared/tmp, so the symlink/clobber risk remains.At Line [174],
filepath.Join(location, FleetURLFileName)ignoreslocationbecauseFleetURLFileName(Line [25]) is absolute (/tmp/...). This keeps the write target in shared/tmpand reintroduces the same path-level overwrite risk previously flagged.🔍 Verification script (read-only)
#!/bin/bash set -euo pipefail # 1) Show definition + call sites rg -n 'FleetURLFileName|filepath.Join\(location,\s*FleetURLFileName\)|cat \+ fleet\.FleetURLFileName' pkg/ide/fleet/fleet.go cmd/up.go # 2) Demonstrate join semantics with absolute second path python - <<'PY' import os print(os.path.join("/home/user/.fleet-server", "/tmp/devpod-fleet.url.txt")) PYExpected: printed path is
/tmp/devpod-fleet.url.txt, confirminglocationis ignored.Also applies to: 174-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ide/fleet/fleet.go` at line 25, FleetURLFileName is defined as an absolute path which causes filepath.Join(location, FleetURLFileName) to ignore the provided location and always write to /tmp; change FleetURLFileName to a relative filename (e.g. "devpod-fleet.url.txt") or otherwise remove the leading slash, and update code paths that use filepath.Join(location, FleetURLFileName) to rely on the now-relative FleetURLFileName so the target file is created under the intended location; reference FleetURLFileName and the call site using filepath.Join(location, FleetURLFileName) to locate and fix the definition and usages.
🧹 Nitpick comments (3)
e2e/tests/provider/provider.go (1)
111-112: Preserve.DS_Storefile intent, but add a directory-prefixed case for branch coverage.This fix is good, but with Line 112 as a file write, the test only exercises the
!entry.IsDir()filter path and not the.DS_Storename-prefix path. Consider adding one hidden directory (e.g..DS_Store_dir) so this test also covers the directory-name filter branch.Proposed test coverage addition
// Add .DS_Store file to tempDir // `#nosec` G306 -- TODO Consider using a more secure permission setting and ownership if needed. err = os.WriteFile(tempDir+"/.DS_Store", []byte("test"), 0o644) framework.ExpectNoError(err) + // Add hidden directory with .DS_Store prefix to exercise name-based directory filtering. + // `#nosec` G301 -- test artifact in temp directory. + err = os.Mkdir(tempDir+"/.DS_Store_dir", 0o755) + framework.ExpectNoError(err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/provider/provider.go` around lines 111 - 112, The test currently only writes a file at tempDir+"/.DS_Store" which exercises the !entry.IsDir() branch but misses the directory-name prefix branch; add a second entry under the same tempDir that is a directory named ".DS_Store_dir" (create the dir via os.Mkdir or equivalent) so the code path checking entry.IsDir() and the name-prefix filter for ".DS_Store" is exercised as well; update any cleanup to remove the created directory and ensure the test asserts both cases are handled by the same logic that reads tempDir entries (refer to tempDir, os.WriteFile, os.Mkdir and the entry.IsDir() filtering logic).pkg/agent/workspace.go (2)
152-155: Use owner-only permission for the temporary probe script.
Line 155sets0o755, but this transient file only needs owner access/execution. Tightening to0o700reduces unnecessary local exposure.Proposed change
- if err := os.WriteFile(testFile, []byte(`#!/bin/sh + if err := os.WriteFile(testFile, []byte(`#!/bin/sh echo DevPod -`), 0o755); err != nil { +`), 0o700); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/agent/workspace.go` around lines 152 - 155, The temporary probe script created with os.WriteFile currently uses mode 0o755; change this to owner-only mode 0o700 to restrict access to the file (update the os.WriteFile call that writes testFile in workspace.go to use 0o700). Keep or update the existing `#nosec` G306,G703 comment as appropriate to reflect the intentional owner-only permission decision for the transient file.
234-237: Optional: centralize permission modes to avoid drift.
Line 236is correct, but this file repeats permission literals and TODO/nosec policy text in multiple spots. Consider extracting shared constants (e.g.,workspaceDirPerm,probeScriptPerm) to keep policy consistent and easier to audit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/agent/workspace.go` around lines 234 - 237, Extract repeated permission literals and related TODO/nosec comments into shared constants (e.g., define workspaceDirPerm and probeScriptPerm) and replace inline octal modes in calls like os.MkdirAll(workspaceDir, 0o755) and any file permission uses in this file with those constants; update related comments to reference the constants and remove duplicated TODO/nosec text so permission policy is centralized and easier to audit, ensuring symbols referenced include workspaceDir, os.MkdirAll, workspaceDirPerm, and probeScriptPerm.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/agent/container/daemon.go`:
- Around line 71-74: The activity file is created with
os.WriteFile(agent.ContainerActivityFile, nil, 0o666) but that mode can be
masked by umask and won't change existing file permissions; after the WriteFile
call add an explicit os.Chmod(agent.ContainerActivityFile, 0o666) and
handle/return any error (wrap it like the existing fmt.Errorf pattern) so the
ContainerActivityFile has consistent permissions across writers; reference the
existing os.WriteFile call and add the os.Chmod call immediately after it.
In `@pkg/client/clientimplementation/proxy_client.go`:
- Around line 170-173: The snippet calling os.MkdirAll(workspaceLocksDir, 0o755)
must not panic on failure; change the initialization to return the error up the
call chain so Lock(ctx)/Unlock methods can propagate it. Replace the
panic(fmt.Errorf(...)) with an error return from the initializer (or store the
init error on the proxy client instance) and update the Lock(ctx) and Unlock
callers to check and return that init error when present; refer to the
workspaceLocksDir os.MkdirAll call and the Lock(ctx)/Unlock methods to locate
and wire the error propagation.
In `@pkg/client/clientimplementation/workspace_client.go`:
- Around line 249-252: The Lock(ctx) flow should not panic on MkdirAll failure;
replace the panic in the workspace locks init with returning an error so callers
can handle it. In the code around Lock(ctx) where os.MkdirAll(workspaceLocksDir,
0o755) currently triggers panic(fmt.Errorf(...)), change that to propagate the
error (e.g., return fmt.Errorf("create workspace locks dir: %w", err)) from the
Lock function, preserving the existing error message text and ensuring Lock(ctx)
continues to return an error instead of crashing the process.
In `@pkg/extract/extract.go`:
- Around line 158-159: Remove the G305 suppression and add proper
canonical-boundary validation after building the path with path.Join(destFolder,
relativePath) (used in the extraction loop and by
getRelativeFromFullPath()/StripLevels) so that any .. sequences or symlink
targets cannot escape the destination: canonicalize both destFolder and the
computed outFileName (e.g., via filepath.Abs and optionally
filepath.EvalSymlinks), compute a relative path with filepath.Rel, and ensure
the resulting relative path does not start with ".." before performing any file
operations (os.OpenFile, os.Chmod, os.Chtimes); fail the extraction of that
entry if the check does not pass.
---
Duplicate comments:
In `@pkg/ide/fleet/fleet.go`:
- Line 25: FleetURLFileName is defined as an absolute path which causes
filepath.Join(location, FleetURLFileName) to ignore the provided location and
always write to /tmp; change FleetURLFileName to a relative filename (e.g.
"devpod-fleet.url.txt") or otherwise remove the leading slash, and update code
paths that use filepath.Join(location, FleetURLFileName) to rely on the
now-relative FleetURLFileName so the target file is created under the intended
location; reference FleetURLFileName and the call site using
filepath.Join(location, FleetURLFileName) to locate and fix the definition and
usages.
---
Nitpick comments:
In `@e2e/tests/provider/provider.go`:
- Around line 111-112: The test currently only writes a file at
tempDir+"/.DS_Store" which exercises the !entry.IsDir() branch but misses the
directory-name prefix branch; add a second entry under the same tempDir that is
a directory named ".DS_Store_dir" (create the dir via os.Mkdir or equivalent) so
the code path checking entry.IsDir() and the name-prefix filter for ".DS_Store"
is exercised as well; update any cleanup to remove the created directory and
ensure the test asserts both cases are handled by the same logic that reads
tempDir entries (refer to tempDir, os.WriteFile, os.Mkdir and the entry.IsDir()
filtering logic).
In `@pkg/agent/workspace.go`:
- Around line 152-155: The temporary probe script created with os.WriteFile
currently uses mode 0o755; change this to owner-only mode 0o700 to restrict
access to the file (update the os.WriteFile call that writes testFile in
workspace.go to use 0o700). Keep or update the existing `#nosec` G306,G703 comment
as appropriate to reflect the intentional owner-only permission decision for the
transient file.
- Around line 234-237: Extract repeated permission literals and related
TODO/nosec comments into shared constants (e.g., define workspaceDirPerm and
probeScriptPerm) and replace inline octal modes in calls like
os.MkdirAll(workspaceDir, 0o755) and any file permission uses in this file with
those constants; update related comments to reference the constants and remove
duplicated TODO/nosec text so permission policy is centralized and easier to
audit, ensuring symbols referenced include workspaceDir, os.MkdirAll,
workspaceDirPerm, and probeScriptPerm.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9e9d9b4-0767-4399-af87-f5bd41f2c195
📒 Files selected for processing (8)
cmd/agent/container/daemon.gocmd/up.goe2e/tests/provider/provider.gopkg/agent/workspace.gopkg/client/clientimplementation/proxy_client.gopkg/client/clientimplementation/workspace_client.gopkg/extract/extract.gopkg/ide/fleet/fleet.go
Comment on lines
+170
to
+173
| // #nosec G301 -- TODO Consider using a more secure permission setting and ownership if needed. | ||
| if err = os.MkdirAll(workspaceLocksDir, 0o755); err != nil { | ||
| panic(fmt.Errorf("create workspace locks dir: %w", err)) | ||
| } |
There was a problem hiding this comment.
Avoid panic on proxy lock-dir setup failure; return an error instead.
On Line 171, panic on MkdirAll failure introduces a process-abort path where Lock(ctx) otherwise communicates failures via error. Please propagate init failures through Lock/Unlock handling instead of panicking.
Suggested change
type proxyClient struct {
m sync.Mutex
workspaceLockOnce sync.Once
workspaceLock *flock.Flock
+ workspaceLockErr error
@@
-func (s *proxyClient) initLock() {
+func (s *proxyClient) initLock() error {
s.workspaceLockOnce.Do(func() {
s.m.Lock()
defer s.m.Unlock()
@@
workspaceLocksDir, err := provider.GetLocksDir(s.workspace.Context)
if err != nil {
- panic(fmt.Errorf("get workspaces dir: %w", err))
+ s.workspaceLockErr = fmt.Errorf("get workspaces dir: %w", err)
+ return
}
// `#nosec` G301 -- TODO Consider using a more secure permission setting and ownership if needed.
if err = os.MkdirAll(workspaceLocksDir, 0o755); err != nil {
- panic(fmt.Errorf("create workspace locks dir: %w", err))
+ s.workspaceLockErr = fmt.Errorf("create workspace locks dir: %w", err)
+ return
}
@@
})
+ return s.workspaceLockErr
}
func (s *proxyClient) Lock(ctx context.Context) error {
- s.initLock()
+ if err := s.initLock(); err != nil {
+ return err
+ }
@@
func (s *proxyClient) Unlock() {
- s.initLock()
+ if err := s.initLock(); err != nil {
+ s.log.Warnf("Skip unlock: lock init failed: %v", err)
+ return
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client/clientimplementation/proxy_client.go` around lines 170 - 173, The
snippet calling os.MkdirAll(workspaceLocksDir, 0o755) must not panic on failure;
change the initialization to return the error up the call chain so
Lock(ctx)/Unlock methods can propagate it. Replace the panic(fmt.Errorf(...))
with an error return from the initializer (or store the init error on the proxy
client instance) and update the Lock(ctx) and Unlock callers to check and return
that init error when present; refer to the workspaceLocksDir os.MkdirAll call
and the Lock(ctx)/Unlock methods to locate and wire the error propagation.
Comment on lines
+158
to
+159
| // #nosec G305 -- Path traversal is handled by archive extraction context | ||
| _ = os.Chmod(outFileName, header.FileInfo().Mode()|0o600) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Path construction and confinement checks =="
rg -n -C3 'outFileName := (path|filepath)\.Join\(destFolder' pkg/extract/extract.go || echo "Pattern not found with exact spacing"
rg -n -C3 'outFileName.*destFolder' pkg/extract/extract.go || echo "outFileName assignment context not found"
rg -n -C3 '(filepath\.Clean|strings\.HasPrefix.*destFolder|path escapes destination|invalid path in archive)' pkg/extract/extract.go || echo "No confinement checks found"
echo
echo "== gosec suppressions =="
rg -n -C2 '#nosec G305|nolint:gosec' pkg/extract/extract.go || echo "No G305 suppressions found"
echo
echo "== Full extract function context =="
rg -n -B 30 -A 30 'os\.Chmod' pkg/extract/extract.go | head -100Repository: skevetter/devpod
Length of output: 2185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full function containing outFileName assignment =="
# Find function signature and read down to the outFileName line
rg -n 'func.*outFileName|relativePath.*=' pkg/extract/extract.go | head -20
echo
echo "== Get full context of extract function (starting from function def) =="
sed -n '1,180p' pkg/extract/extract.go | tail -120Repository: skevetter/devpod
Length of output: 3361
Path traversal vulnerability: G305 suppression lacks boundary validation.
The suppression at line 158 claims traversal is "handled by archive extraction context," but path.Join(destFolder, relativePath) at line 94 lacks explicit canonical-path boundary checking. The getRelativeFromFullPath() function and StripLevels logic do not prevent .. sequences; a crafted tar entry like ../../etc/passwd would traverse outside destFolder via path.Join(), affecting all downstream operations (os.OpenFile at 138, os.Chmod at 159, os.Chtimes at 163).
Remove the suppression and add validation to ensure outFileName is canonicalized and verifiably under destFolder before file operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/extract/extract.go` around lines 158 - 159, Remove the G305 suppression
and add proper canonical-boundary validation after building the path with
path.Join(destFolder, relativePath) (used in the extraction loop and by
getRelativeFromFullPath()/StripLevels) so that any .. sequences or symlink
targets cannot escape the destination: canonicalize both destFolder and the
computed outFileName (e.g., via filepath.Abs and optionally
filepath.EvalSymlinks), compute a relative path with filepath.Rel, and ensure
the resulting relative path does not start with ".." before performing any file
operations (os.OpenFile, os.Chmod, os.Chtimes); fail the extraction of that
entry if the check does not pass.
Signed-off-by: GitHub <noreply@github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/pro/wakeup.go (1)
78-79:⚠️ Potential issue | 🟡 MinorLogging format string mismatch.
Infofis called with two arguments (targetWorkspaceandworkspaceInstance.Name) but the format string"Workspace %s is not sleeping"only has one%splaceholder. The second argument will be silently ignored.🐛 Proposed fix
- cmd.Log.Infof("Workspace %s is not sleeping", targetWorkspace, workspaceInstance.Name) + cmd.Log.Infof("Workspace %s is not sleeping", workspaceInstance.Name)Or if both values are needed:
- cmd.Log.Infof("Workspace %s is not sleeping", targetWorkspace, workspaceInstance.Name) + cmd.Log.Infof("Workspace %s (%s) is not sleeping", targetWorkspace, workspaceInstance.Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pro/wakeup.go` around lines 78 - 79, The Infof call on cmd.Log.Infof has a format-string/argument mismatch: it supplies two values (targetWorkspace and workspaceInstance.Name) but the format only has one %s. Fix by either using a single argument with Info/Infof consistently (e.g., log only targetWorkspace or only workspaceInstance.Name), or update the format string to include two placeholders and pass both values; locate the call to cmd.Log.Infof in wakeup.go and adjust the format or logging method accordingly.cmd/pro/provider/update/workspace.go (1)
65-71:⚠️ Potential issue | 🔴 CriticalGuard
oldInstancenil before update flows.Both paths assume
platform.FindInstancealways returns a non-nil instance. If not found returns(nil, nil), Line 71 and Line 102 can fail downstream with nil dereference behavior.💡 Proposed fix
projectName := project.ProjectFromNamespace(newInstance.GetNamespace()) opts := platform.FindInstanceOptions{Name: newInstance.GetName(), ProjectName: projectName} oldInstance, err := platform.FindInstance(ctx, baseClient, opts) if err != nil { return err } + if oldInstance == nil { + return fmt.Errorf("workspace instance %q not found in project %q", newInstance.GetName(), projectName) + } @@ opts := platform.FindInstanceOptions{UID: workspaceUID, ProjectName: project} oldInstance, err := platform.FindInstance(ctx, baseClient, opts) if err != nil { return err } + if oldInstance == nil { + return fmt.Errorf("workspace instance with UID %s not found in project %s", workspaceUID, project) + }Also applies to: 96-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pro/provider/update/workspace.go` around lines 65 - 71, platform.FindInstance may return (nil, nil) so guard against oldInstance being nil before calling updateInstance; in the code around opts := platform.FindInstanceOptions{Name: newInstance.GetName(), ProjectName: projectName} and the subsequent oldInstance, err := platform.FindInstance(...) check for err as you do and then if oldInstance == nil return a descriptive error (or wrap with fmt.Errorf) indicating the instance was not found, and apply the same nil-check before the later update flow (the block that calls updateInstance(ctx, baseClient, oldInstance, newInstance, cmd.Log)) to avoid nil dereference.
♻️ Duplicate comments (2)
pkg/client/clientimplementation/workspace_client.go (1)
555-580:⚠️ Potential issue | 🔴 Critical
initLock()error path is still unsafe forUnlock()After this signature change,
Unlock()still ignores theinitLock()error (Line 267) and then callss.workspaceLock.Unlock()(Line 278). If init fails, this can panic on a nil lock pointer.🔧 Minimal fix
func (s *workspaceClient) Unlock() { - s.initLock() + if err := s.initLock(); err != nil { + s.log.WithFields(logrus.Fields{"error": err}).Warn("skip unlock: lock init failed") + return + } // try to unlock machine if s.machineLock != nil { err := s.machineLock.Unlock() if err != nil { s.log.WithFields(logrus.Fields{"error": err}).Warn("error unlocking machine") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/clientimplementation/workspace_client.go` around lines 555 - 580, initLock() can fail leaving workspaceLock/machineLock nil, but Unlock() currently ignores initLock errors and calls s.workspaceLock.Unlock(), causing a panic; update the Unlock method to call initLock() and immediately return any error (s.workspaceLockErr) or otherwise check that s.workspaceLock and s.machineLock are non-nil before calling Unlock on them, and only call Unlock if the lock pointer is non-nil; ensure you reference the same fields (workspaceLock, machineLock) and reuse initLock()/workspaceLockErr for error propagation.pkg/ide/fleet/fleet.go (1)
173-174:⚠️ Potential issue | 🟠 MajorStill symlink/clobber-prone when writing a fixed filename.
This no longer targets
/tmp, but it still writes to a deterministic path. If this code runs with elevated privileges whilelocationis user-writable, a precreated symlink can still cause unintended overwrite. Use temp-file-in-dir + atomic rename.🔐 Safer write pattern
index := strings.Index(text, "https://fleet.jetbrains.com/") fleetURLFile := filepath.Join(location, FleetURLFileName) - err = os.WriteFile(fleetURLFile, []byte(strings.TrimSpace(text[index:])), 0o600) // `#nosec` G703 + tmpFile, err := os.CreateTemp(location, FleetURLFileName+".tmp-*") + if err != nil { + return err + } + tmpName := tmpFile.Name() + defer func() { _ = os.Remove(tmpName) }() + if err = tmpFile.Chmod(0o600); err != nil { + _ = tmpFile.Close() + return err + } + if _, err = tmpFile.WriteString(strings.TrimSpace(text[index:])); err != nil { + _ = tmpFile.Close() + return err + } + if err = tmpFile.Sync(); err != nil { + _ = tmpFile.Close() + return err + } + if err = tmpFile.Close(); err != nil { + return err + } + if err = os.Rename(tmpName, fleetURLFile); err != nil { + return err + } if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ide/fleet/fleet.go` around lines 173 - 174, The write to a deterministic path using fleetURLFile := filepath.Join(location, FleetURLFileName) followed by os.WriteFile(...) is still symlink/clobber-prone; fix by writing to a securely created temp file in the same directory (use location), fsync the file, and then atomically rename it to FleetURLFileName to replace the target; ensure the temp file is created with O_EXCL (or ioutil.TempFile in that dir), permissions 0600, handle errors from write/sync/close, and perform os.Rename to atomically move the temp file to fleetURLFile to avoid symlink attacks.
🧹 Nitpick comments (7)
cmd/build.go (1)
58-60: Preserve the underlying permission-check failure in returned error.The current return message loses
err, so root-cause diagnostics are dropped.Proposed fix
- err = image.CheckPushPermissions(ctx, cmd.Repository) - if err != nil { - return fmt.Errorf("cannot push to %s, please make sure you have push permissions to repository %s", cmd.Repository, cmd.Repository) - } + err = image.CheckPushPermissions(ctx, cmd.Repository) + if err != nil { + return fmt.Errorf( + "cannot push to repository %s, please make sure you have push permissions: %w", + cmd.Repository, + err, + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/build.go` around lines 58 - 60, The error returned after calling image.CheckPushPermissions(ctx, cmd.Repository) discards the underlying error; update the error return to wrap or include the original err (from image.CheckPushPermissions) so callers see the root cause (e.g., return fmt.Errorf("cannot push to %s, please make sure you have push permissions to repository %s: %w", cmd.Repository, cmd.Repository, err) or otherwise wrap err) while keeping the existing message and references to cmd.Repository and err.pkg/image/image.go (1)
74-77: Wrap push-permission check errors with image context.Returning
errdirectly here makes troubleshooting harder when multiple images are processed.Proposed fix
- err = remote.CheckPushPermission(ref, keychain, http.DefaultTransport) - if err != nil { - return err - } + err = remote.CheckPushPermission(ref, keychain, http.DefaultTransport) + if err != nil { + return fmt.Errorf("check push permission for %q: %w", image, err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/image/image.go` around lines 74 - 77, The remote.CheckPushPermission error should be wrapped with the image reference so failures identify which image failed; replace the direct return of err in the block calling remote.CheckPushPermission(ref, keychain, http.DefaultTransport) with a wrapped error that includes ref (e.g., fmt.Errorf("checking push permission for %s: %w", ref, err) or using errors.Wrap) so the error message contains the image context.pkg/devcontainer/setup/setup.go (1)
358-358: Use a private mode for~/.kubedirectory.Consider tightening this to
0o700since the directory holds credential-related config and should be user-private.Suggested diff
- if err := os.MkdirAll(kubeDir, 0o755); err != nil { // `#nosec` G301 -- Standard directory permissions + if err := os.MkdirAll(kubeDir, 0o700); err != nil { // `#nosec` G301 -- kube directory should be user-private return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainer/setup/setup.go` at line 358, The os.MkdirAll call creating the kubeDir currently uses permissive 0o755; change it to user-private 0o700 to protect credential-related files. Locate the os.MkdirAll(kubeDir, 0o755) invocation (referencing the kubeDir variable and the surrounding setup code in setup.go) and replace the mode with 0o700, keeping the existing `#nosec` comment or updating it if needed.pkg/ide/fleet/fleet.go (1)
222-223: Consider tightening fleet directory permissions and removing TODO debt.
0o755is broader than needed for this per-user state directory.0o700is safer by default and avoids relying on a lingering TODO with#nosec.♻️ Suggested hardening
- // `#nosec` G301 -- TODO Consider using a more secure permission setting and ownership if needed. - err = os.MkdirAll(folder, 0o755) + err = os.MkdirAll(folder, 0o700)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ide/fleet/fleet.go` around lines 222 - 223, Change the directory creation in fleet.go to use stricter permissions: replace the os.MkdirAll call that currently uses 0o755 with 0o700 for the per-user state directory (the call where err = os.MkdirAll(folder, 0o755) and the variable folder is used). Remove the trailing TODO and the inline `#nosec G301` suppression since the tighter permission removes the justification for the suppression, and ensure error handling around the err variable remains unchanged.pkg/platform/instance.go (1)
104-111: Name-based lookup doesn't handle "not found" gracefully.The UID-based lookup (lines 98-101) returns
(nil, nil)when no workspace is found, but the Name-based lookup returns the raw API error. This inconsistency means callers need different error handling for UID vs Name lookups.Consider wrapping the error check to return
(nil, nil)forNotFounderrors to maintain consistency:♻️ Suggested fix for consistent not-found handling
if opts.Name != "" { workspace, err := managementClient.Loft().ManagementV1(). DevPodWorkspaceInstances(namespace).Get(ctx, opts.Name, metav1.GetOptions{}) if err != nil { + if kerrors.IsNotFound(err) { + return nil, nil + } return nil, err } return workspace, nil }Note: This would require importing
kerrors "k8s.io/apimachinery/pkg/api/errors".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platform/instance.go` around lines 104 - 111, The Name-based branch in the lookup (when opts.Name != "") currently returns the raw API error from managementClient.Loft().ManagementV1().DevPodWorkspaceInstances(...).Get; update the error handling in that branch to detect kerrors.IsNotFound(err) and return (nil, nil) for not-found errors to match the UID-based path, otherwise return the error; import kerrors "k8s.io/apimachinery/pkg/api/errors" and apply this check around the Get call referencing opts.Name and DevPodWorkspaceInstances(...).Get.pkg/client/clientimplementation/daemonclient/delete.go (1)
25-25: Same namespace resolution concern, but graceful nil handling mitigates impact.This call also uses only
UIDwithoutProjectName. However, the subsequent nil check (lines 28-41) gracefully handles the case where the workspace isn't found by proceeding to delete the local workspace folder, which prevents a hard failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/clientimplementation/daemonclient/delete.go` at line 25, The call to platform.FindInstance uses only UID which can mis-resolve when namespaces/projects differ; update the FindInstance invocation (the workspace retrieval where workspace, err := platform.FindInstance(ctx, baseClient, platform.FindInstanceOptions{UID: c.workspace.UID})) to also set ProjectName (e.g., ProjectName: c.workspace.ProjectName) in the platform.FindInstanceOptions to ensure correct namespace resolution, while keeping the existing nil/error handling logic that falls back to deleting the local workspace folder.pkg/client/clientimplementation/daemonclient/up.go (1)
32-32: Same namespace resolution concern as other UID-only lookups.See comment on
stop.go- this call also uses onlyUIDwithoutProjectName, which may limit the search scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/clientimplementation/daemonclient/up.go` at line 32, The platform.FindInstance call currently passes only UID (platform.FindInstance(ctx, baseClient, platform.FindInstanceOptions{UID: c.workspace.UID})), which mirrors the same namespace-resolution issue noted in stop.go; update the FindInstanceOptions to include the workspace/project scope (e.g., add ProjectName: c.workspace.ProjectName or the appropriate project field on c.workspace) so the lookup is constrained to the correct namespace; modify the call where platform.FindInstanceOptions is constructed in up.go to include both UID and ProjectName to match the resolution used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/helper/ssh_server.go`:
- Around line 105-111: The activity marker file is being created with executable
bits and the chmod error is ignored; change
os.WriteFile(agent.ContainerActivityFile, nil, 0o755) to create the file with
non-executable permissions (e.g., 0o644) and remove execute bits, then call
os.Chmod(agent.ContainerActivityFile, 0o644) and check its returned error
instead of discarding it; if Chmod fails, write a clear error to stderr and
return (use the same error handling pattern as the preceding os.WriteFile
error), referencing agent.ContainerActivityFile, os.WriteFile and os.Chmod to
locate and update the code.
In `@cmd/pro/provider/rebuild.go`:
- Around line 61-62: After calling platform.FindInstance(ctx, baseClient,
instanceOpts) assign to workspace, check if workspace == nil immediately and
return a clear error (or wrap and return the existing err) before proceeding;
update the logic around the FindInstance call in rebuild.go (where instanceOpts
and workspace are set) so that if workspace is nil you handle it (e.g., return
fmt.Errorf("workspace %s not found", targetWorkspace) or similar) instead of
allowing later code to dereference a nil workspace.
In `@cmd/pro/rebuild.go`:
- Around line 69-70: After calling platform.FindInstance with instanceOpts
(where you set instanceOpts := platform.FindInstanceOptions{Name:
targetWorkspace, ProjectName: cmd.Project} and receive workspace, err :=
platform.FindInstance(...)), add an explicit nil check for workspace before
proceeding to platform.DialInstance; if workspace == nil (and err == nil) return
or surface a clear error (e.g., "workspace not found" including targetWorkspace
and ProjectName) so you don't pass a nil workspace into platform.DialInstance
and produce a confusing failure path. Ensure this nil-check occurs immediately
after the FindInstance call and handles both err != nil and workspace == nil
cases.
In `@pkg/platform/instance.go`:
- Around line 84-87: The current logic sets namespace =
project.ProjectNamespace(opts.ProjectName) even when both opts.Namespace and
opts.ProjectName are empty, causing prefix-only behavior; change it so when both
are empty you set namespace = metav1.NamespaceAll (import
k8s.io/apimachinery/pkg/apis/meta/v1) to enable true cluster-wide lookups,
otherwise keep the existing project.ProjectNamespace(opts.ProjectName) fallback;
update any related docstring to state that an empty ProjectName+Namespace means
cluster-wide search and ensure callers that pass only UID will now work as
intended (references: opts.Namespace, opts.ProjectName,
project.ProjectNamespace).
In `@pkg/ts/workspace_server.go`:
- Line 230: The socket permission currently set with os.Chmod(runnerProxySocket,
0o755) prevents non-owner processes from writing to the Unix socket; change the
mode to allow read/write for owner/group/others (e.g., 0o666) so non-owner
clients can connect, and update the surrounding comment to accurately reflect
that the socket is world-writable/accessible; locate the os.Chmod call
referencing runnerProxySocket and replace 0o755 with 0o666.
---
Outside diff comments:
In `@cmd/pro/provider/update/workspace.go`:
- Around line 65-71: platform.FindInstance may return (nil, nil) so guard
against oldInstance being nil before calling updateInstance; in the code around
opts := platform.FindInstanceOptions{Name: newInstance.GetName(), ProjectName:
projectName} and the subsequent oldInstance, err := platform.FindInstance(...)
check for err as you do and then if oldInstance == nil return a descriptive
error (or wrap with fmt.Errorf) indicating the instance was not found, and apply
the same nil-check before the later update flow (the block that calls
updateInstance(ctx, baseClient, oldInstance, newInstance, cmd.Log)) to avoid nil
dereference.
In `@cmd/pro/wakeup.go`:
- Around line 78-79: The Infof call on cmd.Log.Infof has a
format-string/argument mismatch: it supplies two values (targetWorkspace and
workspaceInstance.Name) but the format only has one %s. Fix by either using a
single argument with Info/Infof consistently (e.g., log only targetWorkspace or
only workspaceInstance.Name), or update the format string to include two
placeholders and pass both values; locate the call to cmd.Log.Infof in wakeup.go
and adjust the format or logging method accordingly.
---
Duplicate comments:
In `@pkg/client/clientimplementation/workspace_client.go`:
- Around line 555-580: initLock() can fail leaving workspaceLock/machineLock
nil, but Unlock() currently ignores initLock errors and calls
s.workspaceLock.Unlock(), causing a panic; update the Unlock method to call
initLock() and immediately return any error (s.workspaceLockErr) or otherwise
check that s.workspaceLock and s.machineLock are non-nil before calling Unlock
on them, and only call Unlock if the lock pointer is non-nil; ensure you
reference the same fields (workspaceLock, machineLock) and reuse
initLock()/workspaceLockErr for error propagation.
In `@pkg/ide/fleet/fleet.go`:
- Around line 173-174: The write to a deterministic path using fleetURLFile :=
filepath.Join(location, FleetURLFileName) followed by os.WriteFile(...) is still
symlink/clobber-prone; fix by writing to a securely created temp file in the
same directory (use location), fsync the file, and then atomically rename it to
FleetURLFileName to replace the target; ensure the temp file is created with
O_EXCL (or ioutil.TempFile in that dir), permissions 0600, handle errors from
write/sync/close, and perform os.Rename to atomically move the temp file to
fleetURLFile to avoid symlink attacks.
---
Nitpick comments:
In `@cmd/build.go`:
- Around line 58-60: The error returned after calling
image.CheckPushPermissions(ctx, cmd.Repository) discards the underlying error;
update the error return to wrap or include the original err (from
image.CheckPushPermissions) so callers see the root cause (e.g., return
fmt.Errorf("cannot push to %s, please make sure you have push permissions to
repository %s: %w", cmd.Repository, cmd.Repository, err) or otherwise wrap err)
while keeping the existing message and references to cmd.Repository and err.
In `@pkg/client/clientimplementation/daemonclient/delete.go`:
- Line 25: The call to platform.FindInstance uses only UID which can mis-resolve
when namespaces/projects differ; update the FindInstance invocation (the
workspace retrieval where workspace, err := platform.FindInstance(ctx,
baseClient, platform.FindInstanceOptions{UID: c.workspace.UID})) to also set
ProjectName (e.g., ProjectName: c.workspace.ProjectName) in the
platform.FindInstanceOptions to ensure correct namespace resolution, while
keeping the existing nil/error handling logic that falls back to deleting the
local workspace folder.
In `@pkg/client/clientimplementation/daemonclient/up.go`:
- Line 32: The platform.FindInstance call currently passes only UID
(platform.FindInstance(ctx, baseClient, platform.FindInstanceOptions{UID:
c.workspace.UID})), which mirrors the same namespace-resolution issue noted in
stop.go; update the FindInstanceOptions to include the workspace/project scope
(e.g., add ProjectName: c.workspace.ProjectName or the appropriate project field
on c.workspace) so the lookup is constrained to the correct namespace; modify
the call where platform.FindInstanceOptions is constructed in up.go to include
both UID and ProjectName to match the resolution used elsewhere.
In `@pkg/devcontainer/setup/setup.go`:
- Line 358: The os.MkdirAll call creating the kubeDir currently uses permissive
0o755; change it to user-private 0o700 to protect credential-related files.
Locate the os.MkdirAll(kubeDir, 0o755) invocation (referencing the kubeDir
variable and the surrounding setup code in setup.go) and replace the mode with
0o700, keeping the existing `#nosec` comment or updating it if needed.
In `@pkg/ide/fleet/fleet.go`:
- Around line 222-223: Change the directory creation in fleet.go to use stricter
permissions: replace the os.MkdirAll call that currently uses 0o755 with 0o700
for the per-user state directory (the call where err = os.MkdirAll(folder,
0o755) and the variable folder is used). Remove the trailing TODO and the inline
`#nosec G301` suppression since the tighter permission removes the justification
for the suppression, and ensure error handling around the err variable remains
unchanged.
In `@pkg/image/image.go`:
- Around line 74-77: The remote.CheckPushPermission error should be wrapped with
the image reference so failures identify which image failed; replace the direct
return of err in the block calling remote.CheckPushPermission(ref, keychain,
http.DefaultTransport) with a wrapped error that includes ref (e.g.,
fmt.Errorf("checking push permission for %s: %w", ref, err) or using
errors.Wrap) so the error message contains the image context.
In `@pkg/platform/instance.go`:
- Around line 104-111: The Name-based branch in the lookup (when opts.Name !=
"") currently returns the raw API error from
managementClient.Loft().ManagementV1().DevPodWorkspaceInstances(...).Get; update
the error handling in that branch to detect kerrors.IsNotFound(err) and return
(nil, nil) for not-found errors to match the UID-based path, otherwise return
the error; import kerrors "k8s.io/apimachinery/pkg/api/errors" and apply this
check around the Get call referencing opts.Name and
DevPodWorkspaceInstances(...).Get.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1de00e70-9c3c-4988-a964-009447e3841f
📒 Files selected for processing (34)
cmd/agent/container/daemon.gocmd/build.gocmd/helper/ssh_server.gocmd/pro/import_workspace.gocmd/pro/provider/create/workspace.gocmd/pro/provider/delete.gocmd/pro/provider/get/workspace.gocmd/pro/provider/rebuild.gocmd/pro/provider/ssh.gocmd/pro/provider/status.gocmd/pro/provider/stop.gocmd/pro/provider/up.gocmd/pro/provider/update/workspace.gocmd/pro/rebuild.gocmd/pro/sleep.gocmd/pro/wakeup.gocmd/troubleshoot.goe2e/tests/up/dockerfile_build.gopkg/client/clientimplementation/daemonclient/delete.gopkg/client/clientimplementation/daemonclient/status.gopkg/client/clientimplementation/daemonclient/stop.gopkg/client/clientimplementation/daemonclient/up.gopkg/client/clientimplementation/workspace_client.gopkg/daemon/platform/local_server.gopkg/devcontainer/prebuild.gopkg/devcontainer/setup/setup.gopkg/extract/extract.gopkg/ide/fleet/fleet.gopkg/image/image.gopkg/platform/instance.gopkg/ssh/server/agent.gopkg/ssh/server/ssh.gopkg/ssh/server/ssh_container.gopkg/ts/workspace_server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/extract/extract.go
Comment on lines
+61
to
+62
| instanceOpts := platform.FindInstanceOptions{Name: targetWorkspace, ProjectName: cmd.Project} | ||
| workspace, err := platform.FindInstance(ctx, baseClient, instanceOpts) |
There was a problem hiding this comment.
Handle workspace == nil immediately after FindInstance.
Without this check, Line 75 can receive a nil workspace and fail later with a weaker error signal.
Suggested fix
instanceOpts := platform.FindInstanceOptions{Name: targetWorkspace, ProjectName: cmd.Project}
workspace, err := platform.FindInstance(ctx, baseClient, instanceOpts)
if err != nil {
return err
}
+if workspace == nil {
+ return fmt.Errorf("workspace %q not found in project %q", targetWorkspace, cmd.Project)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| instanceOpts := platform.FindInstanceOptions{Name: targetWorkspace, ProjectName: cmd.Project} | |
| workspace, err := platform.FindInstance(ctx, baseClient, instanceOpts) | |
| instanceOpts := platform.FindInstanceOptions{Name: targetWorkspace, ProjectName: cmd.Project} | |
| workspace, err := platform.FindInstance(ctx, baseClient, instanceOpts) | |
| if err != nil { | |
| return err | |
| } | |
| if workspace == nil { | |
| return fmt.Errorf("workspace %q not found in project %q", targetWorkspace, cmd.Project) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/pro/provider/rebuild.go` around lines 61 - 62, After calling
platform.FindInstance(ctx, baseClient, instanceOpts) assign to workspace, check
if workspace == nil immediately and return a clear error (or wrap and return the
existing err) before proceeding; update the logic around the FindInstance call
in rebuild.go (where instanceOpts and workspace are set) so that if workspace is
nil you handle it (e.g., return fmt.Errorf("workspace %s not found",
targetWorkspace) or similar) instead of allowing later code to dereference a nil
workspace.
Comment on lines
84
to
87
| namespace := opts.Namespace | ||
| if namespace == "" { | ||
| namespace = project.ProjectNamespace(opts.ProjectName) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the docstring and implementation in pkg/platform/instance.go
cat -n pkg/platform/instance.go | head -100Repository: skevetter/devpod
Length of output: 3634
🏁 Script executed:
# Check the ProjectNamespace helper function
cat -n pkg/platform/project/project.go | grep -A 10 "func ProjectNamespace"Repository: skevetter/devpod
Length of output: 436
🏁 Script executed:
# Find all FindInstance calls to understand usage patterns
rg -n "FindInstance\(" --type=go -A 3Repository: skevetter/devpod
Length of output: 6162
🏁 Script executed:
# Specifically check daemonclient/delete.go and daemonclient/status.go
echo "=== daemonclient/delete.go ===" && cat -n daemonclient/delete.go && echo "" && echo "=== daemonclient/status.go ===" && cat -n daemonclient/status.goRepository: skevetter/devpod
Length of output: 162
🏁 Script executed:
# Read the complete FindInstance implementation
cat -n pkg/platform/instance.go | sed -n '72,125p'Repository: skevetter/devpod
Length of output: 1873
🏁 Script executed:
# Check the daemonclient files mentioned in the issue
cat -n pkg/client/clientimplementation/daemonclient/delete.goRepository: skevetter/devpod
Length of output: 4097
🏁 Script executed:
# Check status.go as well
cat -n pkg/client/clientimplementation/daemonclient/status.goRepository: skevetter/devpod
Length of output: 992
🏁 Script executed:
# Look for how metav1.NamespaceAll is used in this codebase
rg -n "NamespaceAll" --type=goRepository: skevetter/devpod
Length of output: 42
Fix docstring or implementation to accurately reflect namespace search scope.
The docstring claims "If both are empty, searches all namespaces," but the code calls project.ProjectNamespace(opts.ProjectName) with an empty string, which returns the prefix (e.g., "loft-p-") rather than enabling cluster-wide search. This causes UID-only lookups to fail for workspaces in project namespaces like "loft-p-myproject".
Multiple callers including daemonclient/delete.go:25, daemonclient/status.go:21, daemonclient/up.go:32, daemon/platform/local_server.go:441, and cmd/pro/provider/create/workspace.go:88 pass only UID without ProjectName or Namespace, relying on the documented all-namespaces behavior that doesn't actually exist.
Either implement true cluster-wide search (e.g., using metav1.NamespaceAll) when both are empty, or update the docstring to reflect the actual prefix-only behavior and require callers to specify the correct namespace.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/platform/instance.go` around lines 84 - 87, The current logic sets
namespace = project.ProjectNamespace(opts.ProjectName) even when both
opts.Namespace and opts.ProjectName are empty, causing prefix-only behavior;
change it so when both are empty you set namespace = metav1.NamespaceAll (import
k8s.io/apimachinery/pkg/apis/meta/v1) to enable true cluster-wide lookups,
otherwise keep the existing project.ProjectNamespace(opts.ProjectName) fallback;
update any related docstring to state that an empty ProjectName+Namespace means
cluster-wide search and ensure callers that pass only UID will now work as
intended (references: opts.Namespace, opts.ProjectName,
project.ProjectNamespace).
|
|
||
| // make sure all users can access the socket | ||
| _ = os.Chmod(runnerProxySocket, 0o777) | ||
| _ = os.Chmod(runnerProxySocket, 0o755) // #nosec G302 |
There was a problem hiding this comment.
Socket mode 0o755 can block non-owner clients
At Line 230, the code comment says “all users can access the socket”, but 0o755 removes write for group/others. For unix sockets, that can prevent non-owner processes from connecting.
🔧 Proposed fix
- _ = os.Chmod(runnerProxySocket, 0o755) // `#nosec` G302
+ _ = os.Chmod(runnerProxySocket, 0o777) // `#nosec` G302 -- required so non-owner users can connect to the unix socket📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _ = os.Chmod(runnerProxySocket, 0o755) // #nosec G302 | |
| _ = os.Chmod(runnerProxySocket, 0o777) // `#nosec` G302 -- required so non-owner users can connect to the unix socket |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ts/workspace_server.go` at line 230, The socket permission currently set
with os.Chmod(runnerProxySocket, 0o755) prevents non-owner processes from
writing to the Unix socket; change the mode to allow read/write for
owner/group/others (e.g., 0o666) so non-owner clients can connect, and update
the surrounding comment to accurately reflect that the socket is
world-writable/accessible; locate the os.Chmod call referencing
runnerProxySocket and replace 0o755 with 0o666.
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/build.go (1)
69-69:⚠️ Potential issue | 🟡 MinorTypo in error message: extra comma before the colon.
The error message has a stray comma that should be removed.
🔧 Proposed fix
- return fmt.Errorf("cannot build image,: %w", err) + return fmt.Errorf("cannot build image: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/build.go` at line 69, Fix the typo in the error string by removing the stray comma in the fmt.Errorf call so it reads "cannot build image: %w" instead of "cannot build image,: %w"; locate the fmt.Errorf invocation in cmd/build.go (the return fmt.Errorf(...) statement) and update the message literal accordingly.pkg/devcontainer/setup/setup.go (1)
494-539:⚠️ Potential issue | 🟠 MajorStale managed SSH keys are not removed when key count decreases.
The cleanup loop skips files with indexes outside the new key list, which leaves obsolete private keys on disk. That keeps credentials that should be removed.
🔧 Proposed fix
- for _, file := range files { - if !strings.HasPrefix(file.Name(), "platform_git_ssh_") { - continue - } - - fileName := strings.TrimPrefix(file.Name(), "platform_git_ssh_") - index, err := strconv.Atoi(fileName) - if err != nil { - continue - } - if index >= len(platformOptions.UserCredentials.GitSsh) { - continue - } - - err = os.Remove(filepath.Join(sshFolder, file.Name())) + for _, file := range files { + if !strings.HasPrefix(file.Name(), "platform_git_ssh_") { + continue + } + + err = os.Remove(filepath.Join(sshFolder, file.Name())) if err != nil { log.Warnf("Error removing previous platform git ssh key: %v", err) } }- "strconv"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainer/setup/setup.go` around lines 494 - 539, The cleanup loop in setup.go currently skips files whose parsed index is outside the new platformOptions.UserCredentials.GitSsh slice, leaving stale keys on disk; update the loop that iterates files in sshFolder (the block using os.ReadDir, strings.HasPrefix("platform_git_ssh_"), strconv.Atoi, and os.Remove) so that if the index cannot be parsed or if index >= len(platformOptions.UserCredentials.GitSsh) you remove the file (and log errors via log.Warnf on removal failures) instead of continuing, ensuring obsolete "platform_git_ssh_*" files are deleted.
♻️ Duplicate comments (1)
pkg/ide/fleet/fleet.go (1)
173-174:⚠️ Potential issue | 🟠 MajorAvoid direct write to predictable path; still symlink/clobber-prone in user-owned dir
This is still a fixed destination filename write. Since
locationcan be chowned to another user, a privileged writer can be tricked via symlink replacement. Use temp-file + atomic rename in the same directory.🔐 Safer write pattern
index := strings.Index(text, "https://fleet.jetbrains.com/") fleetURLFile := filepath.Join(location, FleetURLFileName) - err = os.WriteFile(fleetURLFile, []byte(strings.TrimSpace(text[index:])), 0o600) // `#nosec` G703 + tmpFile, err := os.CreateTemp(location, FleetURLFileName+".*.tmp") + if err != nil { + return err + } + defer func() { + _ = os.Remove(tmpFile.Name()) + _ = tmpFile.Close() + }() + if err = tmpFile.Chmod(0o600); err != nil { + return err + } + if _, err = tmpFile.Write([]byte(strings.TrimSpace(text[index:]))); err != nil { + return err + } + if err = tmpFile.Sync(); err != nil { + return err + } + if err = tmpFile.Close(); err != nil { + return err + } + if err = os.Rename(tmpFile.Name(), fleetURLFile); err != nil { + return err + } if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ide/fleet/fleet.go` around lines 173 - 174, The current direct write to fleetURLFile (constructed via filepath.Join(location, FleetURLFileName)) is vulnerable to symlink races; replace the os.WriteFile call with a safer pattern: create a temp file in the same directory (use os.CreateTemp or ioutil.TempFile with dir=location), write the trimmed data to that temp file, call Sync/Close to flush, set file mode to 0o600, then atomically rename the temp file to fleetURLFile with os.Rename; ensure error handling on CreateTemp/Write/Sync/Close/Rename and remove the temp file on failure to avoid leftover files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/helper/ssh_server.go`:
- Around line 105-106: The current os.WriteFile call that creates
agent.ContainerActivityFile uses permissions 0o644 which prevents non-owner
helper processes from writing and breaks shared heartbeat updates; change the
creation permission to 0o666 (matching the shared-file contract used in
cmd/agent/container/daemon.go) so the file is world-writable by helpers, and
ensure any existing Chmod flow (if present) remains compatible with
agent.ContainerActivityFile and its writers.
---
Outside diff comments:
In `@cmd/build.go`:
- Line 69: Fix the typo in the error string by removing the stray comma in the
fmt.Errorf call so it reads "cannot build image: %w" instead of "cannot build
image,: %w"; locate the fmt.Errorf invocation in cmd/build.go (the return
fmt.Errorf(...) statement) and update the message literal accordingly.
In `@pkg/devcontainer/setup/setup.go`:
- Around line 494-539: The cleanup loop in setup.go currently skips files whose
parsed index is outside the new platformOptions.UserCredentials.GitSsh slice,
leaving stale keys on disk; update the loop that iterates files in sshFolder
(the block using os.ReadDir, strings.HasPrefix("platform_git_ssh_"),
strconv.Atoi, and os.Remove) so that if the index cannot be parsed or if index
>= len(platformOptions.UserCredentials.GitSsh) you remove the file (and log
errors via log.Warnf on removal failures) instead of continuing, ensuring
obsolete "platform_git_ssh_*" files are deleted.
---
Duplicate comments:
In `@pkg/ide/fleet/fleet.go`:
- Around line 173-174: The current direct write to fleetURLFile (constructed via
filepath.Join(location, FleetURLFileName)) is vulnerable to symlink races;
replace the os.WriteFile call with a safer pattern: create a temp file in the
same directory (use os.CreateTemp or ioutil.TempFile with dir=location), write
the trimmed data to that temp file, call Sync/Close to flush, set file mode to
0o600, then atomically rename the temp file to fleetURLFile with os.Rename;
ensure error handling on CreateTemp/Write/Sync/Close/Rename and remove the temp
file on failure to avoid leftover files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a8a7712-48d6-4a90-9f18-d90cb4166e06
📒 Files selected for processing (13)
cmd/build.gocmd/helper/ssh_server.gocmd/pro/provider/update/workspace.gocmd/pro/rebuild.gocmd/pro/wakeup.goe2e/tests/up/dockerfile_build.gopkg/client/clientimplementation/workspace_client.gopkg/devcontainer/setup/setup.gopkg/ide/fleet/fleet.gopkg/image/image.gopkg/platform/instance.gopkg/ts/workspace_server.gopkg/upgrade/upgrade.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/pro/wakeup.go
- pkg/ts/workspace_server.go
- pkg/image/image.go
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/platform/instance.go (1)
67-69:⚠️ Potential issue | 🟠 MajorName-based lookup does not match the documented all-namespace behavior.
Line 67 says empty
Namespace+ProjectNamesearches all namespaces, but Line 114 usesGet, which is namespace-scoped. Withopts.Nameand empty scope, this path can silently returnnilvia Line 117 instead of performing a real cross-namespace lookup.Suggested fix
type FindInstanceOptions struct { // UID searches for an instance by its UID label. Takes precedence over Name. UID string // Name searches for an instance by its name. Only used if UID is empty. Name string - // Namespace specifies the namespace to search in. If empty and ProjectName is set, - // the project namespace is used. If both are empty, searches all namespaces. + // Namespace specifies the namespace to search in. If empty and ProjectName is set, + // the project namespace is used. If both are empty, UID lookup searches all namespaces. + // Name lookup requires Namespace or ProjectName to avoid ambiguous cross-namespace matches. Namespace string // ProjectName converts to a project namespace. Ignored if Namespace is explicitly set. ProjectName string } @@ if opts.Name != "" { + if namespace == metav1.NamespaceAll { + return nil, fmt.Errorf("Namespace or ProjectName must be specified when searching by Name") + } return findInstanceByName(ctx, managementClient, opts.Name, namespace) }In Kubernetes/client-go style APIs for namespaced resources, does calling Get with namespace="" perform an all-namespaces lookup, or is all-namespaces supported only for List/Watch? Please provide authoritative docs/examples.Also applies to: 113-118, 142-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/platform/instance.go` around lines 67 - 69, The code documents that an empty Namespace + ProjectName should search all namespaces but the name-based lookup path calls Get (which is namespace-scoped) and will silently fail when namespace is empty; change the logic that handles opts.Name so that when Namespace == "" you perform a cross-namespace List (using metav1.NamespaceAll) and match by name (and ProjectName/labels as needed) instead of calling Get, handle the case of zero or multiple matches explicitly, and only call Get when a specific Namespace is provided; refer to the Namespace, ProjectName, opts.Name, Get and List usage when locating the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/build.go`:
- Around line 58-62: The permission check currently calls
image.CheckPushPermissions(ctx, ...) but the underlying pkg/image/image.go uses
remote.CheckPushPermission with http.DefaultTransport and no context, so
cancellations/timeouts aren't propagated; update the implementation referenced
by image.CheckPushPermissions (or replace its call site) to use a context-aware
approach: either inline the go-containerregistry permission-check logic and swap
any context.TODO()/context.Background() uses for the passed ctx, or perform a
real push-style request configured with remote.WithContext(ctx) so the ctx is
honored during registry operations; reference the image.CheckPushPermissions
function and the remote.CheckPushPermission call site when making the change.
---
Duplicate comments:
In `@pkg/platform/instance.go`:
- Around line 67-69: The code documents that an empty Namespace + ProjectName
should search all namespaces but the name-based lookup path calls Get (which is
namespace-scoped) and will silently fail when namespace is empty; change the
logic that handles opts.Name so that when Namespace == "" you perform a
cross-namespace List (using metav1.NamespaceAll) and match by name (and
ProjectName/labels as needed) instead of calling Get, handle the case of zero or
multiple matches explicitly, and only call Get when a specific Namespace is
provided; refer to the Namespace, ProjectName, opts.Name, Get and List usage
when locating the code to modify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01b4223a-4635-457b-9b73-4814bed9dc6e
📒 Files selected for processing (3)
cmd/build.gocmd/helper/ssh_server.gopkg/platform/instance.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/helper/ssh_server.go
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/image/image.go (1)
87-96: Context-in-struct pattern is necessary here.Storing
context.Contextin a struct field is typically flagged by linters likecontainedctx. However, this approach is justified because:
remote.CheckPushPermissiondoes not accept a context parameter in its public API (go-containerregistry v0.21.2)- The custom transport ensures context is properly attached to HTTP requests via
req.WithContext(t.ctx)- The transport is created fresh per
CheckPushPermissionscall, so context staleness isn't a concernNote: The context propagation is limited to the HTTP transport level. The go-containerregistry library's
CheckPushPermissionfunction internally hard-codescontext.TODO()andcontext.Background(), so cancellations won't propagate to all internal operations—only HTTP-level requests respect the context.If linter warnings become an issue, you can add a
//nolint:containedctxdirective explaining the rationale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/image/image.go` around lines 87 - 96, The linter will flag storing context.Context in contextAwareTransport, but this is acceptable here because CheckPushPermission (from go-containerregistry) has no context param and we only need to inject the context at HTTP request time via contextAwareTransport.RoundTrip; update the code by adding a clear comment above the contextAwareTransport type explaining the rationale (that transport is created per CheckPushPermission call, only HTTP requests observe the context, and go-containerregistry internals use TODO/Background) and add a //nolint:containedctx directive with that explanation to suppress the linter warning; ensure the transport creation remains per-call so context staleness is not possible and keep req = req.WithContext(t.ctx) in RoundTrip.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/image/image.go`:
- Around line 87-96: The linter will flag storing context.Context in
contextAwareTransport, but this is acceptable here because CheckPushPermission
(from go-containerregistry) has no context param and we only need to inject the
context at HTTP request time via contextAwareTransport.RoundTrip; update the
code by adding a clear comment above the contextAwareTransport type explaining
the rationale (that transport is created per CheckPushPermission call, only HTTP
requests observe the context, and go-containerregistry internals use
TODO/Background) and add a //nolint:containedctx directive with that explanation
to suppress the linter warning; ensure the transport creation remains per-call
so context staleness is not possible and keep req = req.WithContext(t.ctx) in
RoundTrip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15e13882-5ad8-433b-a91e-1e0137ea56e1
📒 Files selected for processing (2)
pkg/image/image.gopkg/platform/instance.go
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.
Signed-off-by: Samuel K skevetter@pm.me
Summary by CodeRabbit
New Features
Bug Fixes
Refactoring