Chown dir when creating

[QuintenQVD0]

Changes

• Chown dir when using create directory button
• Closes Created Folders have wrong User and Group Ownership (root:root) #150

Summary by CodeRabbit

• Bug Fixes
  • Directory creation functionality has been enhanced to properly manage file ownership and permissions on newly created directories, ensuring they are created with correct access control settings and remain accessible. Directory operations now fail gracefully if ownership cannot be configured.

[coderabbitai]

📝 Walkthrough

Walkthrough

The handler for creating server directories now invokes Chown immediately after successful directory creation to establish proper file ownership. Requests abort if the ownership operation fails; otherwise, HTTP 204 No Content is returned.

Changes

Cohort / File(s)	Summary
Ownership Fix router/router_server_files.go	Added Chown call after CreateDirectory succeeds to ensure newly created directories have correct user/group ownership; request aborts with error if Chown fails.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 A folder's born, but ownership's at stake,
A simple Chown is all it takes,
No more root-owned directories gray,
Just pelican:pelican, hip-hip-hooray! 🎉

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Chown dir when creating' directly describes the main change: adding chown operation to directory creation logic.
Linked Issues check	✅ Passed	The PR adds chown call to directory creation [#150], matching the identified root cause that directory creation lacked chown present in file write paths.
Out of Scope Changes check	✅ Passed	Changes are limited to adding chown operation in postServerCreateDirectory handler, directly addressing the ownership issue in #150.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@router/router_server_files.go`:
- Around line 427-430: The handler currently calls
s.Filesystem().Chown(filepath.Join(data.Path, data.Name)) without validating
data.Name, allowing empty string or "." to resolve to data.Path and recurse over
an unintended tree; before calling s.Filesystem().Chown (and before any
create/chown logic) validate data.Name to reject empty strings, "." or other
path-traversal/invalid names (e.g., trim and check for len==0 or name==".", and
ensure it does not contain path separators), and return an error via
middleware.CaptureAndAbort(c, err) if invalid so the recursive chown only runs
for explicit valid folder names.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c00dc900-233e-4acb-b1dd-ff1e2a695e75

📥 Commits

Reviewing files that changed from the base of the PR and between 27f9dac and 86566f3.

📒 Files selected for processing (1)

• router/router_server_files.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). (3)

• GitHub Check: Build and Test (ubuntu-22.04, 1.25.7, linux, amd64)
• GitHub Check: Build and Test (ubuntu-22.04, 1.26.0, linux, amd64)
• GitHub Check: Analyze (go)

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Guard against empty/dot directory names before recursive Chown.

With the new recursive Chown, a request like name="" (or ".") will resolve to data.Path, so this endpoint can recursively chown an existing tree instead of only the new folder. Please reject invalid names before create/chown.

🔧 Proposed fix

 func postServerCreateDirectory(c *gin.Context) {
 	s := ExtractServer(c)

 	var data struct {
 		Name string `json:"name"`
 		Path string `json:"path"`
 	}
 	// BindJSON sends 400 if the request fails, all we need to do is return
 	if err := c.BindJSON(&data); err != nil {
 		return
 	}
+	name := strings.TrimSpace(data.Name)
+	if name == "" || name == "." || name == ".." {
+		c.AbortWithStatusJSON(http.StatusUnprocessableEntity, gin.H{
+			"error": "Invalid directory name.",
+		})
+		return
+	}
+	target := filepath.Join(data.Path, name)

-	if err := s.Filesystem().CreateDirectory(data.Name, data.Path); err != nil {
+	if err := s.Filesystem().CreateDirectory(name, data.Path); err != nil {
 		...
 	}
-	if err := s.Filesystem().Chown(filepath.Join(data.Path, data.Name)); err != nil {
+	if err := s.Filesystem().Chown(target); err != nil {
 		middleware.CaptureAndAbort(c, err)
 		return
 	}
 	c.Status(http.StatusNoContent)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@router/router_server_files.go` around lines 427 - 430, The handler currently
calls s.Filesystem().Chown(filepath.Join(data.Path, data.Name)) without
validating data.Name, allowing empty string or "." to resolve to data.Path and
recurse over an unintended tree; before calling s.Filesystem().Chown (and before
any create/chown logic) validate data.Name to reject empty strings, "." or other
path-traversal/invalid names (e.g., trim and check for len==0 or name==".", and
ensure it does not contain path separators), and return an error via
middleware.CaptureAndAbort(c, err) if invalid so the recursive chown only runs
for explicit valid folder names.

[C0D3-M4513R]

Correct me if I'm wrong, but this only seems to try to chown a directory, when explicitly creating a directory.

This doesn't seem to affect any directories being created, as a result of creating a file at all (e.g. creating test/testfile.txt).
Additionally this only seems to target the deepest created directory, when creating multiple directories at once (e.g. test/test1)

Please reopen #150, since this is insufficient in fixing that bug.

[parkervcp]

In theory this should chown the entire path. I am about to start looking at integrating the ptero wings FS changes and that may change all of this anyways.