Create the service unit directory owner-only instead of refusing it

[alexeyzimarev]

Closes #934 — AI-2772

What & why

kcap daemon service install creates the unit directory with Directory.CreateDirectory, which applies 0777 & ~umask, and then refuses it for being group-writable. On Debian and Ubuntu pam_umask with USERGROUPS_ENAB yes sets umask 002 for a user whose primary group is their own name, so the directory lands 0775 and no install can succeed. The mode bits alone do not say another account is involved — a user-private group has one member, and a directory we can chmod is one no other account owns. So the directory is now created 0700, a shared-writable one is tightened, and the refusal is kept for the case where the bits survive.

Where to look

ServiceFiles.CreateDirectory walks one level at a time on purpose: the mode-taking Directory.CreateDirectory overload applies the mode to the leaf only, so a single call would still leave every ancestor it created at 0777 & ~umask — and a writable parent is a rename away from replacing the unit directory whole. Ancestors that already exist are left alone; ~/.config is not this code's to tighten.

The refusal branch needs a directory the current user does not own, so it is driven by a seam rather than provoked — the only directories a test could rely on finding are shared system ones that a run as root would then really chmod.

Verification

Reproduced on a stock Ubuntu box, where the directory is created and rejected within the same second:

/home/alexey/.config/systemd       mode=775  birth=2026-09-14 11:58:57
/home/alexey/.config/systemd/user  mode=775  birth=2026-09-14 11:58:57   (empty)

WriteOwnerOnly_creates_a_usable_unit_directory_under_any_umask sets umask 002 and 000 and fails on main with exactly the reported InvalidOperationException. The parent-mode assertion in it also failed against a first attempt that used one CreateDirectory call, which is how the leaf-only behaviour above was found.

Capacitor.Cli.Tests.Unit: 4098 total, 0 failed, 19 skipped. dotnet build Capacitor.slnx: 16 projects, 0 warnings. dotnet publish -c Release: no IL2026/IL3050.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-14T14:05:10.234085Z	9233459	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[qodo-code-review]

PR Summary by Qodo

Create service unit directories with owner-only permissions

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Creates missing Unix service-directory ancestors owner-only regardless of process umask.
• Repairs shared-write bits, refusing installation only when insecure permissions persist.
• Centralizes directory handling and tests permissive umasks, repairs, and refusal paths.

Diagram

graph TD
  M["Service Managers"] --> W["Secure Writer"] --> C["Create 0700 Ancestors"] --> S{"Shared Write?"}
  S -->|No| U["Write 0600 Unit"]
  S -->|Yes| T["Remove Write Bits"] --> R{"Bits Remain?"}
  R -->|No| U
  R -->|Yes| F["Refuse Install"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Temporarily change process umask

• ➕ Allows ordinary directory creation to produce restrictive permissions.
• ➕ Could reduce explicit per-directory creation logic.
• ➖ Umask is process-global and unsafe around concurrent operations.
• ➖ Requires careful restoration on every failure path.
• ➖ Implicitly affects unrelated file and directory creation.

2. Keep rejecting shared-writable directories

• ➕ Avoids modifying pre-existing directory permissions.
• ➕ Retains simpler fail-closed behavior.
• ➖ Breaks installation under common Debian and Ubuntu umask 002 defaults.
• ➖ Cannot distinguish user-private groups from genuinely shared ownership.
• ➖ Rejects directories created by the installer itself.

Recommendation: Keep the PR's explicit, level-by-level directory creation and repair strategy. It avoids process-global umask changes, secures every newly created ancestor without modifying unrelated existing parents, and preserves fail-closed behavior when shared-write permissions cannot be removed.

Files changed (6) +119 / -23

Bug fix (1) +51 / -11

ServiceFiles.csCreate and repair secure service unit directories +51/-11

Create and repair secure service unit directories

• Creates each missing Unix directory ancestor with mode 0700, bypassing permissive umask defaults. Existing target directories have group and world write removed when possible and are rejected only if those bits remain; Windows retains inherited ACL behavior.

src/Capacitor.Cli/Services/ServiceFiles.cs

Refactor (3) +2 / -7

LaunchdServiceManager.csDelegate launchd directory creation to the secure writer +1/-3

Delegate launchd directory creation to the secure writer

• Removes direct LaunchAgents directory creation so ServiceFiles owns both directory permission enforcement and unit writing.

src/Capacitor.Cli/Services/LaunchdServiceManager.cs

SystemdServiceManager.csDelegate systemd directory creation to the secure writer +1/-3

Delegate systemd directory creation to the secure writer

• Removes direct user-unit directory creation, ensuring systemd installations use the centralized owner-only directory workflow.

src/Capacitor.Cli/Services/SystemdServiceManager.cs

WindowsScheduledTaskServiceManager.csCentralize Windows task directory creation +0/-1

Centralize Windows task directory creation

• Removes per-file directory creation from the scheduled-task manager so the shared writer consistently owns directory preparation across platforms.

src/Capacitor.Cli/Services/WindowsScheduledTaskServiceManager.cs

Tests (1) +65 / -4

ServiceFilesTests.csCover secure directory creation and repair behavior +65/-4

Cover secure directory creation and repair behavior

• Adds serialized umask tests proving nested directories remain safe under permissive modes. Covers repairable shared-write directories and injected repair failure while preserving the no-unit-on-refusal guarantee.

test/Capacitor.Cli.Tests.Unit/Services/ServiceFilesTests.cs

Documentation (1) +1 / -1

README.mdDocument owner-only service directory guarantees +1/-1

Document owner-only service directory guarantees

• Updates the service credential security documentation to describe 0700 directory creation, shared-write repair, and refusal when unsafe permissions persist.

README.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Elevated installs alter foreign folders ✗ Dismissed 🐞 Bug ≡ Correctness

Description

RequireNoSharedWrite treats a successful SetUnixFileMode call as proof that the directory
belongs to the current account, although an elevated process can chmod directories owned by anyone.
When HOME points to another user's home or a shared location, installation strips that directory's
write permissions and creates a root-owned service file there rather than refusing the ownership
mismatch.

Code

src/Capacitor.Cli/Services/ServiceFiles.cs[R119-120]

+            if (tighten is not null) tighten(directory);
+            else File.SetUnixFileMode(directory, File.GetUnixFileMode(directory) & ~SharedWrite);

Relevance

●● Moderate

Ownership validation is a meaningful elevated-install concern, but no close historical precedent
confirms acceptance.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The repair checks only mode bits and never verifies ownership, while the service unit is created as
the current process user. Home resolution accepts any rooted HOME, and systemd and launchd derive
their service paths directly from that value, making an elevated process capable of repairing and
writing into another account's service directory.

src/Capacitor.Cli/Services/ServiceFiles.cs[109-123]
src/Capacitor.Cli/Services/ServiceFiles.cs[63-75]
src/Capacitor.Cli.Core/UserHome.cs[16-26]
src/Capacitor.Cli/Services/SystemdUnit.cs[12-15]
src/Capacitor.Cli/Services/LaunchdUnit.cs[13-18]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new permission repair can modify a pre-existing service directory owned by another account when installation runs with elevated privileges.

## Fix Focus Areas
- src/Capacitor.Cli/Services/ServiceFiles.cs[105-127]
- src/Capacitor.Cli.Core/UserHome.cs[16-26]

## Recommended Fix
Read the directory owner's Unix user ID without following symlinks and compare it with the effective user ID before changing its mode. Refuse installation when ownership differs, including when the process is root, and only tighten a pre-existing directory positively verified as belonging to the invoking account.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Writable parents can replace service units ✗ Dismissed 🐞 Bug ⛨ Security

Description

CreateDirectory stops walking at the first existing ancestor, while RequireNoSharedWrite
validates only the final unit directory. When an existing parent such as .config/systemd is group-
or world-writable, another account can rename and replace the protected user directory after
validation and control the unit loaded by the daemon.

Code

src/Capacitor.Cli/Services/ServiceFiles.cs[R99-102]

+        for (var d = directory; !string.IsNullOrEmpty(d) && !Directory.Exists(d); d = Path.GetDirectoryName(d)!)
+            missing.Push(d);
+
+        while (missing.Count > 0) Directory.CreateDirectory(missing.Pop(), OwnerOnlyDir);

Relevance

●● Moderate

Credible parent-directory replacement risk, but PR explicitly scopes existing ancestors outside its
ownership.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The creation loop explicitly stops as soon as it reaches any existing directory, but the permission
repair is called only for the final destination. Systemd units live below .config/systemd/user, so
a writable pre-existing .config or systemd directory retains the ability to replace the checked
user directory wholesale.

src/Capacitor.Cli/Services/ServiceFiles.cs[90-102]
src/Capacitor.Cli/Services/ServiceFiles.cs[113-127]
src/Capacitor.Cli/Services/SystemdUnit.cs[12-15]
src/Capacitor.Cli/Services/LaunchdUnit.cs[13-18]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`CreateDirectory` leaves existing ancestors unchecked, allowing a writable parent to replace the validated service directory and its unit files.

## Fix Focus Areas
- src/Capacitor.Cli/Services/ServiceFiles.cs[94-123]

## Recommended Fix
Walk every directory component between the trusted user home and the unit directory, validating that each existing component has no group or world write permission. Refuse installation when an existing parent remains shared-writable, and use no-follow or directory-handle-based operations where available so validated components cannot be exchanged through symlink or rename races.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. A test bypasses temporary path helpers ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

WriteOwnerOnly_creates_a_usable_unit_directory_under_any_umask builds the unit path with
Path.Combine(dir, ...) after dir was obtained from tmp.PathTo(...). Because the new path
remains under the temporary root, path handling provided by the fixture does not reach the
service-file path.

Code

test/Capacitor.Cli.Tests.Unit/Services/ServiceFilesTests.cs[R139-140]

+        var dir      = tmp.PathTo("systemd", "user");
+        var path     = Path.Combine(dir, "kcap-daemon-test.service");

Relevance

●●● Strong

Directly violates an explicit active rule requiring TempDir members for all paths under temporary
roots.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 2767472 requires all paths beneath a TempDir root to be constructed through its
members rather than with Path.Combine. Lines 139-140 derive dir through PathTo but then
manually combine the final child path.

Rule 2767472: Use TempDir helper for all temporary filesystem state in tests
test/Capacitor.Cli.Tests.Unit/Services/ServiceFilesTests.cs[139-140]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test constructs a path beneath its temporary root with `Path.Combine` instead of using the temporary-directory helper's path API.

## Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/Services/ServiceFilesTests.cs[139-140]

## Recommended Fix
Construct the complete unit path with `PathTo("systemd", "user", "kcap-daemon-test.service")`; derive the directory separately with `PathTo("systemd", "user")` only where its mode must be asserted.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Two tests manage temp folders manually ✗ Dismissed 📘 Rule violation ▣ Testability

Description

WriteOwnerOnly_creates_a_usable_unit_directory_under_any_umask and
WriteOwnerOnly_tightens_a_shared_writable_directory_it_can_repair instantiate TempDir inside the
test methods instead of using a public required [TempDir] property. These added lifecycles bypass
framework injection and leave the class mixing injected fixtures with ad hoc temporary-directory
ownership.

Code

test/Capacitor.Cli.Tests.Unit/Services/ServiceFilesTests.cs[138]

+        using var tmp = new TempDir();

Relevance

●●● Strong

Directly violates an explicit active repository rule requiring injected TempDir fixtures.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 2808173 prohibits new TempDir() calls in test classes and requires a single
injected [TempDir] property. The two added tests construct their own fixtures at lines 138 and
163.

Rule 2808173: Use injected [TempDir] public required property in test classes instead of manual fields
test/Capacitor.Cli.Tests.Unit/Services/ServiceFilesTests.cs[138-138]
test/Capacitor.Cli.Tests.Unit/Services/ServiceFilesTests.cs[163-163]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Two newly added tests manually construct and dispose `TempDir`, while test classes requiring temporary directories must use a framework-injected public required `[TempDir]` property.

## Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/Services/ServiceFilesTests.cs[13-16]
- test/Capacitor.Cli.Tests.Unit/Services/ServiceFilesTests.cs[138-138]
- test/Capacitor.Cli.Tests.Unit/Services/ServiceFilesTests.cs[163-163]

## Recommended Fix
Add one `[TempDir] public required TempDir Tmp { get; init; }` property to the test class, replace the added local `new TempDir()` instances with that injected fixture, and migrate the class's remaining manual `TempDir` constructions so it has a single lifecycle model.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 64 rules

✅ Cross-repo context — repo relationships

Review mode: ⚖️ Balanced: This changes security-sensitive service-directory permissions and installation behavior across multiple platforms, with meaningful filesystem and ownership edge cases, but is not broad or defect-dense enough to justify extended review.

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 923345912d

ℹ️ 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".

[chatgpt-codex-connector]

Repair owner bits after creating each directory

On Unix, the explicit 0700 mode passed to Directory.CreateDirectory is still filtered by the process umask. For example, under umask 0400, each new directory lands as 0300; the unit write can succeed because write and traverse remain, but later ListInstalled() calls cannot enumerate the unreadable unit directory and fail with an access error. The existing file path repairs its requested mode after creation, but this new directory path tests only shared-write bits, so it should likewise verify and repair every newly created directory to 0700 rather than relying on the requested creation mode.

Useful? React with 👍 / 👎.

[alexeyzimarev]

Confirmed and fixed in 954a51f.

The mechanism is right, and the test suite was blind to it: the assertions only checked the shared-write bits, so a umask stripping the owner's bits passed. Added umask 400 and 777 cases and tightened the assertion to an exact mode — both failed, then passed once each created directory is chmod'd after creation.

400 is the sharper case, as described: 0300 is still writable, so the install reports success and ListInstalled fails afterwards. 777 lands 0000 and fails at the write instead.