fix(execd): skip chmod/chown for pre-existing dirs in MakeDir#1044
fix(execd): skip chmod/chown for pre-existing dirs in MakeDir#1044ashishpatel26 wants to merge 2 commits into
Conversation
When createDirectories is called on a path whose parent components already exist (e.g. /tmp), MakeDir was unconditionally calling ChmodFile on the target after os.MkdirAll, even if the directory was not newly created. For system directories the process does not own this causes 'chmod: operation not permitted'. Fix: stat the target before MkdirAll; if it already exists, skip ChmodFile entirely. Only newly-created directories receive the requested permission. This matches POSIX mkdir -p --mode semantics. Fixes opensandbox-group#1024
ashishpatel26
requested review from
Pangjiping,
hittyt and
ninan-nn
as code owners
|
Changed directories: components. 📋 Recommended labels (based on changed files):
Other available labels:
💡 Tip: Use |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates MakeDir to avoid applying chmod/chown on directories that already exist, preventing failures when the process lacks ownership/permissions, and adds regression tests to validate idempotent behavior.
Changes:
- Detect whether the target directory exists before
os.MkdirAll, and skipChmodFileif it does. - Add tests covering new directory creation, idempotency on existing dirs, and nested directory creation with pre-existing parents.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| components/execd/pkg/web/controller/utils_windows.go | Skip chmod/chown for pre-existing directories in MakeDir (Windows variant). |
| components/execd/pkg/web/controller/utils.go | Skip chmod/chown for pre-existing directories in MakeDir (non-Windows/shared variant). |
| components/execd/pkg/web/controller/utils_test.go | Add tests validating MakeDir behavior for new/existing/nested directories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _, statErr := os.Stat(abs) | ||
| alreadyExisted := statErr == nil |
| if alreadyExisted { | ||
| return nil | ||
| } |
| _, statErr := os.Stat(abs) | ||
| alreadyExisted := statErr == nil |
| require.Equal(t, before.Mode(), after.Mode(), "MakeDir must not chmod a pre-existing directory") | ||
| } | ||
|
|
||
| func TestMakeDirCreatesNestedDirectoriesWithoutChmodingParents(t *testing.T) { |
| func TestMakeDirCreatesNewDirectory(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| newDir := filepath.Join(tmp, "newdir") | ||
|
|
||
| require.NoError(t, MakeDir(newDir, model.Permission{Mode: 755})) | ||
|
|
||
| info, err := os.Stat(newDir) | ||
| require.NoError(t, err) | ||
| require.True(t, info.IsDir()) | ||
| } |
|
Thanks for the reviews. Addressing feedback: Copilot – Use Copilot – Undocumented semantics change: Valid. The updated behavior (skip chmod on pre-existing dirs) is intentional to avoid "operation not permitted" when callers pass system directories like Copilot – Test doesn't assert applied permissions: Will add an assertion on the resulting permission bits on non-Windows platforms. Copilot – Typo in test identifier |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec49e55b8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if alreadyExisted { | ||
| return nil |
There was a problem hiding this comment.
When a single /directories request includes both a parent and a nested child (for example /workspace/project with mode 700 and /workspace/project/logs with mode 755), MakeDirs iterates the request map in unspecified order; if the child is handled first, os.MkdirAll creates the parent with default permissions, and the later explicit parent entry now returns here without applying its requested mode/owner. That regresses multi-directory requests from the public API's “map of directory paths to permission objects” behavior, leaving explicitly requested directories with default permissions depending on map iteration order.
Useful? React with 👍 / 👎.
|
Duplicate of #1025 |
Summary
MakeDircalledos.MkdirAllthen unconditionally calledChmodFileon the target path/tmp,/workspace) owned by a different user, thechmod/chownsyscall fails with "operation not permitted", breaking subsequentwriteFilecallsos.StatbeforeMkdirAll— if the path already exists, skip permission changes entirely; only newly-created directories getchmod/chownappliedTest plan
go test ./pkg/web/controller/...— 3 new unit tests: new dir created, pre-existing dir mode unchanged, nested create doesn't chmod parentsFixes #1024