[Security] Keep screen-recording state out of world-writable /tmp

[Chessing234]

Fixes #8372.

Recording state, region state, and the optional recorder debug log now use the per-user runtime directory. Without XDG_RUNTIME_DIR, they use ${XDG_STATE_HOME:-$HOME/.local/state}/omarchy; the webcam resizer uses that same fallback so region anchoring remains consistent.

The fallback directory is explicitly mode 0700, including when it already exists with broader permissions. This protects recording paths, geometry and recorder stderr even when XDG_STATE_HOME is traversable by other users. Directory creation and permission repair happen only on recording start, so an unusable directory does not prevent the stop path from signalling an active recorder.

Both troubleshooting documents now give the runtime and fallback debug-log locations.

Validation

All 26 assertions in test/shell.d/screenrecording-test.sh pass, including actual start-path runs with stubbed recorder/compositor commands, runtime and fallback paths, webcam region anchoring, and an existing 0755 fallback repaired to 0700. Tests ran on a local macOS copy with only Bash shebangs adjusted to the installed Bash 5 interpreter; native Arch recording and GUI validation remain outstanding. Bash syntax and git diff checks pass.

[Copilot]

Pull request overview

Moves screen-recording state and diagnostics into the per-user runtime directory to prevent /tmp path manipulation.

Changes:

• Relocates recording state, region, and debug files.
• Updates diagnostic documentation.
• Adds regression coverage for recording state placement.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File	Description
bin/omarchy-capture-screenrecording	Moves runtime files out of fixed /tmp paths.
test/shell.d/screenrecording-test.sh	Tests secure state-file placement.
default/agents/skills/omarchy/capture.md	Updates debug-log location.
default/agents/skills/omarchy/contributing.md	Updates troubleshooting guidance.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[omarchybot]

Reviewed at 8fd7576, with an independent second opinion from Codex at xhigh reasoning. The change is correct and complete for what it claims: I grepped every reader and writer of omarchy-screenrecord-filename, omarchy-screenrecord.log, omarchy-screenrecord-region and every hardcoded /tmp/omarchy-screenrecord* across bin/, shell/, default/, migrations/, docs/ and manual/, and nothing else reads the old location. The bar indicator (shell/plugins/bar/indicators/ScreenRecording.qml:18) and the Stop Screenrecording menu guard (default/omarchy/omarchy-menu.jsonc:60) both go through pgrep -f '^gpu-screen-recorder', so the indicator and the stop path are unaffected. Both stop-path reads (lines 230 and 263) use RECORDING_FILE, and REGION_FILE resolves to the same place it did before, matching bin/omarchy-capture-webcam-resize:11. Codex agreed on all of that; its independence is not currently guaranteed, so treat the agreement as agreement rather than as a separate confirmation.

Tests ran on a disposable VM, not on the review machine. ./test/cli passes, and ./test/shell.d/screenrecording-test.sh passes 21 assertions. I also checked the new assertions have teeth by mutation: reverting line 45 to the old /tmp name fails screen recording keeps no state under a fixed /tmp name, as your description says.

Pushed one commit (72080c5), test-only:

That first assertion also failed on the fixed script whenever /tmp/omarchy-screenrecord-filename already existed — which is not hypothetical. A recording interrupted before the upgrade leaves one behind, and the mutation run above leaves one behind too, so the very next run of the suite red-lights for the wrong reason (I hit this on the worker). Worse, the file another local account creates is one you cannot delete, so the scenario this PR is about would permanently break the test that proves it fixed. The commit snapshots the name's mtime and size before the run and compares after, instead of demanding /tmp be bare. The unfixed script rewrites the file either way, so the regression is still caught — verified both directions on the worker, with a stale file present.

Two things left for the maintainer to decide, not defects I would change under you:

• RUNTIME_DIR="${XDG_RUNTIME_DIR:-/tmp}" (line 44) puts both files back at fixed, pre-creatable names in world-writable /tmp when the variable is unset. Under a normal uwsm/SDDM login pam_systemd always sets it, so this is narrow; but it is the same shape as the defect being closed. It matches the dominant idiom in the repo (16 sites, including the REGION_FILE line you deliberately lined up with), which is a real argument for leaving it. It does not match your own [Security] Stage diagnostics where only their owner can read them #8370, which falls back to $XDG_STATE_HOME/omarchy and pre-creates at mode 600 for the same class of problem. Whichever way it goes, the two PRs should probably agree.
• A recording already running when omarchy update lands wrote its filename to the old /tmp path; the new stop path reads the runtime dir, finds nothing, and skips finalization — the MP4 survives in the recordings directory unfinalized, but the toast opens an empty path. Codex raised this and suggested a legacy bridge. I do not think one should be added: reading the old /tmp name whenever the runtime file is absent would restore exactly the attacker-controlled read this PR removes, and absent is the normal state before any recording. A clean cutover looks like the right call, but it is worth the maintainer knowing the transition exists. Separately, the stop path's behaviour when RECORDING_FILE is missing (empty filename, ffmpeg -i "", a "saved" toast that opens nothing) is a pre-existing rough edge, not something this PR introduces — out of scope here.

Related, and it matters for merge order: this closes #8372. #7532 adds test/shell.d/screenrecording-portal-fallback-test.sh with a hardcoded RECORDING_FILE="/tmp/omarchy-screenrecord-filename" — that test will fail once this merges, whichever lands second. #7484 hardcodes a new /tmp/omarchy-screenrecord-paused in the script, the menu JSON and the QML indicator, citing "matching the existing /tmp/omarchy-screenrecord-filename convention" — a convention this PR removes. The competing pause/resume PR #6470 already uses ${XDG_RUNTIME_DIR:-/tmp} and needs nothing.

Waiting on the maintainer.

[itz4blitz]

The direction is right and the test design is a standout — comparing the /tmp name across the run instead of demanding absence, so a leftover from a pre-fix recording can't false-positive, is the correct call.

One substantive problem: the fallback re-opens the hole this PR exists to close.

RUNTIME_DIR="${XDG_RUNTIME_DIR:-/tmp}"
RECORDING_FILE="$RUNTIME_DIR/omarchy-screenrecord-filename"

When XDG_RUNTIME_DIR is unset — a recording started from a systemd user unit without the import, a cron/at job, an ssh session before systemd --user, or a stale env after logout — the state file lands right back at the predictable /tmp/omarchy-screenrecord-filename the PR's own comment calls out: "a name in world-writable /tmp is a name any other local account can create first and then point wherever it likes." The comment and the code disagree exactly in the branch where the attack still works.

Since the stop path reads the file back and feeds the path to ffmpeg (writes beside it), mv (replaces it) and rm -f (deletes its preview), a pre-planted symlink at the fallback name is a same-user arbitrary-file clobber or delete on stop — the same primitive #8372 describes.

Suggested resolution, matching the resolution #8370 and #8429 use:

RUNTIME_DIR="${XDG_RUNTIME_DIR:-${XDG_STATE_HOME:-$HOME/.local/state}/omarchy}"

…and mkdir -p "$RUNTIME_DIR" before use, or refuse to run when neither is usable. REGION_FILE carried the same :-/tmp fallback before this PR, so this is a chance to fix both rather than preserve one.

A test for the unset branch would pin it: run the start path with env -u XDG_RUNTIME_DIR and a fake HOME, then assert no omarchy-screenrecord-filename appears at a /tmp path and the state file lands under the state dir instead. Happy to contribute that if useful.

Everything else — the debug-log relocation, the docs updates, the stop-path assertions — checks out.

[Chessing234]

fell back to the state dir when there is no runtime dir, instead of /tmp.

[omarchybot]

Re-reviewed the new commit (a47bfff), with a second opinion from Codex at xhigh reasoning. The fallback is the right answer to the point raised last time, but the commit landed two integration problems, both now fixed on your branch.

The suite is red on any clean machine. ./test/shell.d/screenrecording-test.sh aborts at line 341 before the first assertion in that block: the file runs under set -euo pipefail, and tmp_state_before=$(stat -c '%y %s' "$tmp_state" 2>/dev/null) takes the script down when /tmp/omarchy-screenrecord-filename is absent — which is the normal case, and now the case on every machine, because the fixed script no longer creates it. On a fresh worker the file reports 18 passed, 0 failed, exit 1, so the three assertions you added never ran at all. This is my fault, not yours: it came in with the snapshot commit I pushed on the last pass, and it survived review only because the mutation runs that produced it had left the name behind on the machine I tested on. a8717fd adds || true to both snapshots. Codex found the same thing statically and traced it to the same line.

bin/omarchy-capture-webcam-resize:11 was left behind. It reads ${XDG_RUNTIME_DIR:-/tmp}/omarchy-screenrecord-region; the recorder now writes that region file to ${XDG_RUNTIME_DIR:-${XDG_STATE_HOME:-$HOME/.local/state}/omarchy}. Nothing changes when XDG_RUNTIME_DIR is set, but in exactly the case the new fallback exists for, a region recording with a webcam writes the region to one path and reads it from another: the overlay anchors to the whole monitor instead of the captured region, and the reader is still looking at a /tmp name anyone can plant. 94f59e8b gives the resizer the same expression and adds an assertion that a recording started without a runtime dir still anchors the camera to its region. Codex flagged this independently from a pinned copy of your tree; that is the one finding here I would call a genuine second pair of eyes rather than agreement.

Both fixes are mutation-checked on a disposable VM, not on the review machine: reverting the || true puts the suite back to 18 passed, exit 1, and reverting the resizer's path fails the new anchor assertion (24 passed, 1 failed). With both in, ./test/shell.d/screenrecording-test.sh is 25 passed, 0 failed, bin-style-test.sh passes, and ./test/cli exits 0.

Two smaller things left for you, neither of them blocking:

• mkdir -p "$RUNTIME_DIR" || exit 1 (line 47) runs before the script decides whether this is a start or a stop. default/hypr/bindings/utilities.lua:38 binds Alt+Print to omarchy-capture-screenrecording --stop-recording || omarchy-menu toggle trigger.capture.screenrecord, so a failed mkdir is indistinguishable from "nothing was recording" and opens the record menu while a recording is still running. It needs a broken $HOME or a non-directory at the runtime path to fire, so it is close to unreachable — but creating the directory inside start_screenrecording, where the writes actually happen, costs nothing and keeps the stop path unconditional. Codex raised the ordering; I checked the keybinding and it is as described.
• The body still says the state moves "next to the region file under $XDG_RUNTIME_DIR (0700, per-user)" and lists three assertions. That is no longer the change: there is a state-directory fallback now, and the region file's reader only just caught up. Worth a rewrite before this is read at merge time.

One thing I checked and am not reporting, because it does not hold: the fallback directory is created 0755 under the default umask 022, and the files in it 0644, which does not match the 0700 the comment above it opens with. On a stock Omarchy install it makes no difference — /etc/login.defs sets HOME_MODE 0700 and $HOME measures 700 on a machine built from the ISO, so nothing under ~/.local/state is reachable by another local account whatever its own mode is. Codex rated it low on the assumption that the ancestors are searchable; they are not.

Related, unchanged from the last pass and still worth the maintainer's attention for merge order: this closes #8372 in full, including the debug log the issue names. #7532 hardcodes RECORDING_FILE="/tmp/omarchy-screenrecord-filename" in a new test file and #7484 hardcodes a new /tmp/omarchy-screenrecord-paused across the script, the menu JSON and the QML indicator, explicitly citing the convention this removes; #6470 needs nothing. #8430 does the same class of fix for the Battle.net installer log with a third shape again — mktemp under TMPDIR at 0600 — so there are now three different answers to "not a fixed name in /tmp" in flight at once.

Codex's independence is not currently guaranteed, so read its agreement as agreement. On this pass it earned more than that twice: it found the set -e abort by static tracing before my worker run reproduced it, and it found the resizer path from a pinned export of your tree that my own edits could not reach. Its first run stopped on its own when it noticed the worktree had moved under it mid-review, which is why it was re-run pinned.

Waiting on you for the two items above, then on the maintainer.

[Chessing234]

@dhh @ryanrhughes ready for review

[Copilot]

🔵 Needs a closer look

The fallback directory may expose sensitive debug data, and debug-log relocation needs coverage.

Review details

Suppressed comments (2)

default/agents/skills/omarchy/capture.md:37

• The implementation now falls back to the Omarchy state directory when XDG_RUNTIME_DIR is unset, so this instruction points affected users to a nonexistent path. Mention the fallback location as well.

If recording fails to start, rerun with `OMARCHY_SCREENRECORD_DEBUG=true` to
collect a log at `$XDG_RUNTIME_DIR/omarchy-screenrecord.log` worth attaching to a bug
report.

default/agents/skills/omarchy/contributing.md:41

• The no-runtime-directory fallback writes this log to the Omarchy state directory, not this path. Include that fallback so the troubleshooting procedure remains valid in the case the code explicitly supports.

`OMARCHY_SCREENRECORD_DEBUG=true` and attach `$XDG_RUNTIME_DIR/omarchy-screenrecord.log`.

• Files reviewed: 3/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[ErikMelton]

• [P2] Protect files created in the fallback directory. mkdir -p and the subsequent paths use ordinary umask permissions. Without XDG_RUNTIME_DIR, a traversable XDG_STATE_HOME can leave the directory 0755 and the state/debug files 0644, exposing recording paths, geometry, and recorder stderr to other users. The fallback directory or individual artifacts should be explicitly private (0700/0600).

• [P2] Document the fallback debug-log location. Both capture.md and contributing.md always direct users to $XDG_RUNTIME_DIR/omarchy-screenrecord.log. When that variable is unset, the implementation instead writes under ${XDG_STATE_HOME:-$HOME/.local/state}/omarchy/, so the troubleshooting instructions point to the wrong file.

[Chessing234]

@ErikMelton Addressed both findings in ce438cd: existing fallback directories are explicitly made 0700, and both docs include the fallback debug-log path. Directory creation also moved to recording start so it cannot block the stop path. All 26 focused assertions pass with Bash 5 compatibility on macOS; native Arch/GUI validation is still outstanding. Could you re-review the update?