fix(files): block privilege escalation via set_permissions on sync folders

[martinmarsian]

Summary

• A user with wired.account.file.set_permissions could assign themselves as owner of a sync folder they do not own, granting new read access to that folder.
• Fix reads the existing owner from .wired/permissions via FilePrivilege(path:) and rejects the request with wired.error.permission_denied when a non-owner attempts to claim ownership.
• The check works regardless of whether .wired/type is present.

Test plan

• As non-owner user: open Get Info on a sync folder owned by another user, change owner to yourself, Save → server rejects with permission denied
• As current owner: change owner to another user → succeeds
• As current owner: change mode bits or group without changing owner → succeeds

🤖 Generated with Claude Code

[github-actions]

Claude finished @nark's task in 1m 57s —— View job

---

Code Review — PR #98: block privilege escalation via set_permissions

✨ Good instinct and well-scoped fix. The change reuses existing infrastructure (FilePrivilege(path:)), the conventional commit message is correct, and the inline comment explains the why clearly. The happy path (folder already has a permissions file) is correctly protected.

That said, there are two issues that need addressing before merge.

---

🔴 Blocking — Nil bypass: first-caller-wins on unowned folders

// FilesController.swift:633
if let existingPrivileges = FilePrivilege(path: realPath) {
    if existingPrivileges.owner != user.username && owner == user.username {
        // …deny…
    }
}

When .wired/permissions does not yet exist (folder was just created, or migrated from an older server build), FilePrivilege(path: realPath) returns nil, the outer if let fails silently, and the check is never entered. The first user with wired.account.file.set_permissions to call setPermissions on that folder can write themselves as owner — exactly the privilege escalation the PR aims to prevent.

The PR description says "The check works regardless of whether .wired/type is present" — that's true, but the guard is NOT immune to an absent .wired/permissions file.

Suggested fix: treat a missing permissions file as "no owner" and block any claim of ownership in that state too (unless the caller is an administrator), or at minimum allow only accounts with a broader admin-level privilege to set initial ownership:

guard let existingPrivileges = FilePrivilege(path: realPath) else {
    // No permissions file yet: only admins may set initial ownership.
    if owner == user.username && !user.hasPrivilege(name: "wired.account.settings.edit") {
        App.serverController.replyError(client: client, error: "wired.error.permission_denied", message: message)
        return
    }
    // Fall through to write initial permissions.
    // (insert the mode-building block here, or restructure to avoid duplication)
    return
}
if existingPrivileges.owner != user.username && owner == user.username {
    App.serverController.replyError(client: client, error: "wired.error.permission_denied", message: message)
    return
}

The exact privilege to gate on is your call — the important thing is that the nil case is explicitly handled rather than silently allowed. Fix this →

---

🟡 Should fix — Path mismatch between check and write

The ownership check reads from realPath (line 633), but privileges are ultimately written to finalPath = resolveAliasesAndSymlinks(in: realPath) (line 659). If realPath is a macOS alias that FileManager.fileExists(atPath:) does not transparently follow, then FilePrivilege(path: realPath) returns nil (no .wired/permissions found) even though the metadata exists at finalPath. This again hits the nil-bypass above.

The simplest fix is to perform the ownership check against finalPath instead of realPath — but finalPath is computed only after the message fields are parsed (line 648). Either hoist finalPath computation above the check, or load FilePrivilege from finalPath:

// hoist line 648 above the new check
let finalPath = resolveAliasesAndSymlinks(in: realPath)

if let existingPrivileges = FilePrivilege(path: finalPath) {   // <-- finalPath, not realPath
    if existingPrivileges.owner != user.username && owner == user.username {
        …
    }
}

---

💡 Suggestion — Flatten nested if

Two nested if statements can be a single compound condition, which is more idiomatic Swift:

if let existing = FilePrivilege(path: finalPath),
   existing.owner != user.username,
   owner == user.username {
    App.serverController.replyError(…)
    return
}

---

Great work spotting and isolating this escalation path — the fix is very close. Resolving the nil bypass and the path-mismatch will make it solid.

[martinmarsian]

Follow-up on the Claude review findings — both flagged items have been addressed in commits pushed after the review ran.

Issue	Status	Commit
🔴 Nil bypass — FilePrivilege(path:) returns nil for folders without .wired/permissions, skipping the ownership check	✅ Fixed — else if branch explicitly denies non-admin ownership claims when no permissions file exists	a9a222b
🟡 Path mismatch — check used realPath, write used finalPath	✅ Fixed — check now uses finalPath (alias/symlink-resolved) consistent with the write path	a9a222b
🟡 Privilege name — initial guard used wired.account.settings.edit (wrong name)	✅ Fixed — corrected to wired.account.account.edit_users	374dc99

The automated re-review can't fire (no CLAUDE_CODE_OAUTH_TOKEN on this repo), so leaving this note manually. Branch is ready for review.