Windows VM: create launcher after start; clear setgid on harden

[Chessing234]

Summary

• Folded Create the Windows launcher entry only once the VM is running #7985
• Folded Clear setgid when hardening Windows VM storage directories #9414

Same changes as #7985 #9414 , folded so review is one place.

Test plan

• Spot-check each folded PR's test plan still applies

Fixes #7943

[llstrk]

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Verified: both changes do what the PR describes. No behavior defect was found in the inspected and tested scope. One small cleanup and one optional test case are noted below.

Launcher entry now written only after a successful start

Verified: windows-vm.desktop is the marker the Omarchy menu reads to disable Install and show Remove. It is now written only after priv up (the docker compose up -d step) succeeds, so every earlier exit leaves the menu offering Install.

install_windows            base                      head
-------------------------  ------------------------  ------------------------
after omarchy-pkg-add      write windows-vm.desktop  -
prompts / confirm (exit)   marker already present    no marker
write_compose (exit)       marker already present    no marker
priv up fails (exit)       marker already present    no marker
priv up succeeds           -                         create_launcher_entry

The desktop entry content is unchanged. Re-running Install after a failed start reuses the existing mounts and overwrites the compose file, so that path recovers cleanly (source-traced, not executed).

Setgid bit cleared when hardening storage directories

Verified: GNU chmod 0700 keeps an existing set-group-ID bit on a directory (gnulib mode_adjust only clears it when the mode mentions it), leaving 2700. That fails the exact-700 checks in prepare_caller_mounts and mounted_leaf_matches. a-s,u=rwx,go= yields exactly 700. Sandboxed runs of the real prepare_user_mount_sources from each revision, with a synthetic $HOME:

$HOME mode        base .windows / Windows   head .windows / Windows
----------------  ------------------------  ------------------------
2755 (setgid)     2700 / 2700               700 / 700
755 (control)     700  / 700                700 / 700

The root-side chmod-and-check step was emulated as a non-root user with each revision's exact mode string (same 2700 vs 700 outcome); prepare_caller_mounts itself was not run as root.

Verified: the new test/shell.d/windows-vm-launcher-test.sh is picked up by test/shell and passes on the head. It fails against the base script and against two reordered variants (call moved before the prompts, and between the confirmation and priv up).

Re-added mkdir -p "$HOME/.windows"

The fold adds mkdir -p "$HOME/.windows" after omarchy-pkg-add in install_windows (head line 1189). It arrives with #7985. Upstream commit c34d20c removed this line when prepare_user_mount_sources, the first step of install_windows, took over creating and validating ~/.windows.

Impact: none at runtime, since the directory always exists by that point. It is an unmentioned change unrelated to either commit's purpose.

Suggested change: drop the line.

Optional test improvement: nothing covers the setgid change. Setting chmod 2755 "$HOME" before calling prepare_user_mount_sources (for example in windows-vm-compose-test.sh, which already calls it) and asserting both directories end at 700 fails on the base (2700) and passes on the head, without privileges.

---

Review information

Test scope: Source review of bin/omarchy-windows-vm, the menu entries that read windows-vm.desktop, the shell test runner, and GNU coreutils chmod / gnulib modechange.c (coreutils 9.7 and 9.12 sources). Sandboxed runs used coreutils 9.11 for prepare_user_mount_sources on base and head with synthetic setgid and plain home directories (identity lookup stubbed), and the new test on the head, against the base script and against two reordered variants. The full install, launch and remove flows (which need /dev/kvm, Docker and privileges) and the existing Windows VM compose and mount-boundary tests were not run.

AI process: Opus 5.5 Medium coordination and synthesis, independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

[Chessing234]

dropped the redundant mkdir — prepare_user_mount_sources already creates ~/.windows.

[AlexR1712]

I reproduced the Remove > Windows failure on Omarchy 4.0.4-1 and tested this PR's fix against removal.

The menu runs omarchy-windows-vm install / remove. I started with no VM. install exited 0 and created the container, compose file, protected bind mounts, disk directory, and launcher. ~/Windows and its bind anchor were both 700. The Windows ISO download was still retrying, so I checked Dockurr's running /run/samba.sh. It sets an empty writable share to 2777. I ran its chmod 2777 /shared operation inside the container, and both the host directory and bind anchor changed to 2777.

remove then exited 1 with ❌ Windows VM removal stopped before user-side cleanup; inspect the VM data before retrying. It left ~/Windows at 2700, with the container, disk data, compose file, and launcher still present. The user's earlier full Windows installation had reached the same mode and removal error. After chmod g-s ~/Windows, the same remove command succeeded. I checked that the VM data, container, compose file, launcher, credentials, and bind mounts were gone.

I added a regression to the existing removal test and opened it against this branch as Chessing234/omarchy#2. It sets ~/Windows to 2777, runs prepare_caller_mounts (called by assert_mounts_safe in the privileged removal path), then runs __priv_remove. The test checks that removal deletes the disk data, keeps the shared files, and leaves the shared directory at 700. It exits 1 at prepare_caller_mounts on untouched quattro and passes on this PR's branch. These are the added test steps:

# Before the final dc() { :; } and __priv_remove in the removal case:
chmod 2777 "$HOME/Windows"
prepare_caller_mounts  # Production assert_mounts_safe calls this as root.

# After __priv_remove and the existing shared-content assertion:
[[ $(stat -c '%a' "$HOME/Windows") == 700 ]] || fail "removal left the shared directory setgid"

The PR branch's compose, launcher, and Windows VM tests passed. I put temporary test files in the project directory after the first compose-test run hit this host's /tmp quota.

[Chessing234]

pulled in the setgid removal regression test

[Chessing234]

thanks for reproducing and for the removal regression test — pulling that in

[llstrk]

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Follow-up to the earlier review, checked on head 799827f4 (rebased onto d201fb95), including the removal reproduction report and the regression test pulled in from it.

Verified: no behavior defect was found in the tested scope. The rebase changed nothing in bin/omarchy-windows-vm, and the only script change since the earlier review is the dropped mkdir.

Earlier finding                               Status on 799827f4
--------------------------------------------  ---------------------------------------
Launcher entry written only after priv up     still verified, unchanged
Setgid cleared when hardening directories     still verified, trigger now confirmed
New launcher test                             still verified, test file unchanged
Re-added mkdir -p "$HOME/.windows"            resolved (b5d6270 removes it)
Optional: no test for the setgid change       partly resolved (root side only)

Setgid fix: Dockur supplies the trigger

Verified in Dockur source (src/samba.sh L154-160): when the shared directory is empty and writable, each container start that reaches Dockur's Samba setup (after the image download on a first install) runs chmod 2777 on it. That has been 2777 since Dockur v5.08; earlier releases used 777, which the base's chmod 0700 handled. On the base, the next privileged prepare_caller_mounts (launch or remove) leaves 2700, fails the exact-700 check and returns 1, so removal stops with the generic "stopped before user-side cleanup" error. On the head the same step yields 700 and continues. This matches the reproduction report; the installed 4.0.4-1 package itself was not inspected.

start mode   chmod 0700 (base)   chmod a-s,u=rwx,go= (head)
----------   -----------------   --------------------------
755          700                 700
2777         2700                700
6777         6700                700

New removal regression test

Verified: in windows-vm-compose-test.sh, the test passes on the head (22 ok) and fails on the base script (exit 1). It also fails when only the root-side chmod (L585) is reverted to chmod 0700. That covers the root side of the earlier optional test note. Reverting only the user-side chmod (L1020) still passes, so that line remains uncovered; both callers go on to the privileged step, which re-hardens.

The test runs as non-root, and assert_mounts_safe calls prepare_caller_mounts only when EUID == 0. The explicit prepare_caller_mounts call in the test is therefore what clears the bit, as its comment says. Consequences:

• Deleting that root-branch call from assert_mounts_safe still passes the test, so the production link from removal to hardening is not exercised.
• The final assertion's message, "removal left the shared directory setgid", credits removal with the helper's work.
• On the base, the run aborts through set -e with no not ok line naming the cause.

Optional test improvement: assert the setup, gate the helper and reword the final assertion message, so the base reports a named failure (not run as written):

chmod 2777 "$HOME/Windows"
[[ $(stat -c '%a' "$HOME/Windows") == 2777 ]] || fail "test setup made the shared directory setgid"
prepare_caller_mounts || fail "prepare_caller_mounts hardens a setgid shared directory"

windows-vm-mount-boundary-test.sh already runs as EUID == 0 in a user namespace. Setting its shared target to 2777 before with_vm_lock assert_mounts_safe and asserting 700 afterwards would cover the root-only link (source-traced, not run: it needs subordinate IDs).

Shared directory reopens after each start (pre-existing, not introduced by this PR)

While ~/Windows stays empty, Dockur sets it back to 2777 on each container start that reaches its Samba setup. The protected bind anchor under /var/lib/omarchy/windows/mounts/users/<uid>/ has 0711/0755 parents and shares the directory's inode. Any local account can therefore list, create, delete or rename entries through that path, whatever the mode of $HOME. Guest-created files are forced to 0666 (samba.sh L231-240). The anchor route stays open through stop until the next privileged launch or remove, or until a reboot drops the bind. The 2777 mode itself survives reboot, so ~/Windows stays reachable wherever $HOME lets other accounts through.

               first Dockur start      next launch/remove              later starts (share still empty)
base           2777, open to others    2700, fails closed              blocked until chmod g-s
this PR        2777, open to others    700, succeeds                   2777 again each start

Impact: source-traced, not run. This PR neither creates nor widens the exposure, but it no longer ends in a failure. Between a start and the next privileged hardening, the manual's statement that other accounts "cannot browse their contents" does not hold.

Suggested change: not required for this PR's fix. This overlaps with open #9783, which changes the same two chmod lines and adds re-hardening after the guest reports ready and on stop (not reviewed here). Only one of the two needs to land.

---

Review information

Test scope: Source review of the current head, base and earlier reviewed head of bin/omarchy-windows-vm and its tests, Dockur dockur/windows source (1e99833), and gnulib modechange.c. Sandboxed runs (GNU coreutils 9.11, synthetic identity stubs) covered the chmod mode comparison, prepare_user_mount_sources with setgid and plain homes, the launcher test, and the compose test on the head and on the base. Diagnostic mutants reverted each chmod and the root-branch hardening call. Not run: the full install, launch and remove flows (Docker, root, /dev/kvm), the root-path mount-boundary test, and Samba itself.

AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

[omarchybot]

Thanks for folding these together. This pull request makes two independent fixes, and they stand differently.

Launcher entry after start (#7943): correct, and nothing else does it. On head 799827f4, create_launcher_entry runs only after priv up succeeds (bin/omarchy-windows-vm:1346). Every earlier exit in install_windows now leaves no windows-vm.desktop behind: Ctrl+C, a failed source preparation, a missing KVM or too little disk, a cancelled RAM, CPU or disk prompt, declining "Proceed with this configuration?", a failed compose or credentials write, and a failed start. That file is the only thing omarchy-menu.jsonc checks to hide Install (line 215) and show Remove (line 296). remove still deletes it (line 1388), and launch checks the compose file, not the marker, so nothing depended on it being written early. One limit of the new test: it compares line numbers, so it would still pass if the call moved inside the if ! priv up failure branch. That is a gap in coverage, not a defect in the change.

The setgid half competes with twelve other open pull requests for the same bug. These all change the same chmod 0700 lines: #9605, #9783, #10113, #10338, #10411, #12019, #12046, #12155, #12323, #13154, #13213, #13750 and #13800. Only one of them should land. Your chmod a-s,u=rwx,go= is correct, and so is the removal regression you pulled in, but this isn't the one we're recommending. #12323 and #9605 make the same fix, and their tests cover more: #12323 tests the root path with live binds and a failed hardening, and #9605 also hardens the credentials directory. If either of those merged alongside this branch, the two chmod lines would conflict.

So the proportionate shape for this pull request is the launcher fix alone. Dropping the setgid commit (c5fd21e0) and the removal test that depends on it (799827f4) would let it land whichever setgid fix the maintainer picks. That's your call and the maintainer's; nothing was pushed to your branch.

This was checked by Claude Opus 5.5, which read both changes against quattro and compared all fourteen diffs. Codex at medium effort was asked separately which setgid fix should go forward, without being given an answer. It ranked #12323 first and #9605 second, and it independently traced every exit path above for the launcher change and found no defect. It may have been able to read this session, so its independence is not guaranteed. Nothing was run on a worker: the review only read the code, and it will be verified on a worker once a direction is chosen. This is waiting on the maintainer to choose between the setgid fixes.

[omarchybot]

Following the merge of #12323, The setgid half is now covered; moving creation of the Windows launcher until after successful startup is a separate change. Keep it open for that remaining work, rebased without the duplicate permission fix.

Scope checked by GPT-6 in Codex and Codex Medium against the discussion and landed change; review independence is not guaranteed. No tests were rerun for this cleanup.

Omabot on behalf of DHH

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Refactors Windows VM launcher setup to write after VM starts.

The PR appears safe to merge, although the existing removal-path test coverage concern remains.

Findings

1.  Removal test bypasses hardening path ▶

Summary

The PR creates the Windows VM launcher only after the VM starts and publishes it atomically. The latest change also prevents publication into a directory at the launcher path; launcher success and failure tests cover the helper.

_{Reviews (4) · Last reviewed commit: "Refuse to publish the Windows launcher i..." · Reviewed by Greptile}

[greptile-apps]

Removal test bypasses hardening path

The test calls prepare_caller_mounts before __priv_remove, so its permission assertion can pass even if privileged removal stops calling that helper. Exercise hardening through the privileged removal path; otherwise this test will miss a regression in the behavior it is meant to protect.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[omarchybot]

The launcher fix works, and it is now the whole of this pull request. I pushed two small commits to your branch, described below.

What was run. The real omarchy-windows-vm install ran on a worker built from the Omarchy ISO. Only the prompts were scripted, and Docker start-up was stubbed to succeed or fail on demand. On quattro (d9b970dd), declining "Proceed with this configuration?" leaves windows-vm.desktop behind, and so does a failed priv up. That is #7943: the menu then hides Install and offers Remove. On this branch the file is written only after a successful start, its contents are identical to quattro's, and if it cannot be saved the installer says so and exits 1. ./test/cli passes on a clean worker, along with windows-vm-launcher-test.sh, windows-vm-launcher-failure-test.sh, windows-vm-compose-test.sh and windows-vm-mount-boundary-test.sh. Both launcher tests fail against quattro's script.

What I pushed, at 84d3cbe1:

• 45cf502b drops the seven lines added to windows-vm-compose-test.sh. That setgid hardening landed with Clear setgid when hardening Windows VM directories #12323, and quattro's compose and mount-boundary tests already cover it through the root path. This test also called prepare_caller_mounts itself before removal, so it could not show that removal hardens the directory, which is what its comment and assertion say.
• 84d3cbe1 changes mv -f to mv -fT in create_launcher_entry. Without -T, a directory at windows-vm.desktop makes mv move the staged entry inside it and succeed: install reports success, but no launcher is created. On the worker, the version without -T did exactly that, and with -T it reports the failure.

This was reviewed by Claude Opus 5.5 and by Codex Medium as a second opinion. In its first round, Codex found the mv -T case and agreed the compose-test hunk was redundant. It found nothing open in its second round on 84d3cbe1. It can read this session, so its independence is not guaranteed. The pull request is now linked to #7943. Its title still mentions setgid, which no longer applies. This is now waiting on the maintainer.