fix: Directories created via panel are owned by root:root#183
fix: Directories created via panel are owned by root:root#183QuintenQVD0 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesMkdirAll chown integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/filesystem/filesystem.go (1)
120-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute the remaining parent-creating server paths through the chowning helper.
Writefilestill letsunixFS.Touchcreate missing parents, andRenamedelegates tounixFS.Rename, whose UFS implementation creates missing destination parents withMkdirAll. Both paths bypassFilesystem.mkdirAll, so nested paths created through these APIs can still leave directories owned by the Wings/root process user. PreserveRename’s existing validation/no-op behavior, but make the server layer chown any missing parents before or after the UFS rename path.Also applies to: 207-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/filesystem/filesystem.go` around lines 120 - 123, The Writefile function (around line 120-123) and the Rename function (around line 207-209) both create missing parent directories but bypass the Filesystem.mkdirAll chowning helper, leaving directories owned by the root/Wings process user. For Writefile, remove the parent-directory creation from unixFS.Touch by using a flag that prevents auto-creation, then ensure missing parents are created through Filesystem.mkdirAll before calling Touch. For Rename, preserve the existing validation and no-op behavior, but add a server-layer chowning step that calls Filesystem.mkdirAll on any missing destination parents before or after delegating to unixFS.Rename. This ensures all parent directories created through these APIs are properly chowned to the intended user.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/filesystem/filesystem.go`:
- Around line 229-232: The MkdirAll function returns a partial list of created
directories when an error occurs, but the current code returns immediately on
error before chowning those partially created paths. To fix this, capture the
created directories list from the unixFS.MkdirAll call, and ensure all
directories in that created list are chowned before returning any error. This
way, even if MkdirAll fails partway through, the directories it did create will
have the correct ownership, preventing ownership-related failures on retries.
---
Outside diff comments:
In `@server/filesystem/filesystem.go`:
- Around line 120-123: The Writefile function (around line 120-123) and the
Rename function (around line 207-209) both create missing parent directories but
bypass the Filesystem.mkdirAll chowning helper, leaving directories owned by the
root/Wings process user. For Writefile, remove the parent-directory creation
from unixFS.Touch by using a flag that prevents auto-creation, then ensure
missing parents are created through Filesystem.mkdirAll before calling Touch.
For Rename, preserve the existing validation and no-op behavior, but add a
server-layer chowning step that calls Filesystem.mkdirAll on any missing
destination parents before or after delegating to unixFS.Rename. This ensures
all parent directories created through these APIs are properly chowned to the
intended user.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e426b59b-9437-482f-9060-01414450a0da
📒 Files selected for processing (6)
internal/ufs/filesystem.gointernal/ufs/fs_unix.gointernal/ufs/fs_unix_test.gointernal/ufs/mkdir_unix.goserver/filesystem/compress.goserver/filesystem/filesystem.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.7, linux, arm64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.26.0, linux, arm64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.26.0, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.7, linux, amd64)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-02T13:53:08.995Z
Learnt from: parkervcp
Repo: pelican-dev/wings PR: 171
File: server/power.go:190-203
Timestamp: 2026-03-02T13:53:08.995Z
Learning: In the server package, when quotas are enabled via config.Get().System.Quotas.Enabled, the disk space check using used >= s.DiskSpace() does not require a special guard for unlimited-disk scenarios (DiskSpace() <= 0). The filesystem handles such cases, so the existing check is sufficient. Apply this pattern to similar quota-related disk checks in the server package and ensure tests/docs reflect that unlimited-disk behavior is governed by the filesystem, not by an extra guard in code.
Applied to files:
server/filesystem/filesystem.goserver/filesystem/compress.go
🔇 Additional comments (6)
internal/ufs/filesystem.go (1)
76-84: LGTM!internal/ufs/mkdir_unix.go (1)
13-18: LGTM!Also applies to: 27-33, 47-67
internal/ufs/fs_unix.go (1)
209-222: LGTM!Also applies to: 474-475, 626-627
internal/ufs/fs_unix_test.go (1)
167-168: LGTM!Also applies to: 177-178, 327-378
server/filesystem/filesystem.go (1)
163-168: LGTM!Also applies to: 203-204
server/filesystem/compress.go (1)
372-384: LGTM!
| created, err := fs.unixFS.MkdirAll(p, mode) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Chown partially created directories before returning MkdirAll errors.
UnixFS.MkdirAll returns the partial created list when a later mkdir fails, but this wrapper returns on err before chowning those paths. A failed or raced nested create can therefore leave newly created parents owned by the process user, making retries hit the same ownership problem.
Proposed fix
func (fs *Filesystem) mkdirAll(p string, mode ufs.FileMode) error {
- created, err := fs.unixFS.MkdirAll(p, mode)
- if err != nil {
- return err
- }
+ created, mkdirErr := fs.unixFS.MkdirAll(p, mode)
for _, dir := range created {
if err := fs.chownFile(dir); err != nil {
+ if mkdirErr != nil {
+ return fmt.Errorf("error creating directories: %w; additionally failed to chown %q: %v", mkdirErr, dir, err)
+ }
return err
}
}
- return nil
+ return mkdirErr
}📝 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.
| created, err := fs.unixFS.MkdirAll(p, mode) | |
| if err != nil { | |
| return err | |
| } | |
| func (fs *Filesystem) mkdirAll(p string, mode ufs.FileMode) error { | |
| created, mkdirErr := fs.unixFS.MkdirAll(p, mode) | |
| for _, dir := range created { | |
| if err := fs.chownFile(dir); err != nil { | |
| if mkdirErr != nil { | |
| return fmt.Errorf("error creating directories: %w; additionally failed to chown %q: %v", mkdirErr, dir, err) | |
| } | |
| return err | |
| } | |
| } | |
| return mkdirErr | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/filesystem/filesystem.go` around lines 229 - 232, The MkdirAll
function returns a partial list of created directories when an error occurs, but
the current code returns immediately on error before chowning those partially
created paths. To fix this, capture the created directories list from the
unixFS.MkdirAll call, and ensure all directories in that created list are
chowned before returning any error. This way, even if MkdirAll fails partway
through, the directories it did create will have the correct ownership,
preventing ownership-related failures on retries.
1 participant
Changes:
see : pterodactyl/wings#328
Summary by CodeRabbit
Bug Fixes
Tests