From fe56d68e905c8dc63a4c6a43727fef40833360d7 Mon Sep 17 00:00:00 2001 From: bastidotnet <233381911+bastidotnet@users.noreply.github.com> Date: Tue, 25 Aug 2026 13:24:00 +0200 Subject: [PATCH 1/4] Validate the cached Apple-display device path before use The cached device path was trusted for merely existing, not for being a hiddev node, and fell back to a predictable /tmp path when XDG_RUNTIME_DIR was unset. Validate the cache shape (hiddev char device) and cache only under the user-private runtime dir; asdcontrol already gates non-Apple devices downstream, so this is defense-in-depth in the layer Omarchy owns. --- bin/omarchy-brightness-display-apple | 21 ++- .../brightness-display-apple-cache-test.sh | 120 ++++++++++++++++++ 2 files changed, 137 insertions(+), 4 deletions(-) create mode 100755 test/shell.d/brightness-display-apple-cache-test.sh diff --git a/bin/omarchy-brightness-display-apple b/bin/omarchy-brightness-display-apple index 81202b87..1027686d 100755 --- a/bin/omarchy-brightness-display-apple +++ b/bin/omarchy-brightness-display-apple @@ -4,7 +4,13 @@ # omarchy:args=[--no-osd] [+N%|N%-|N%] # omarchy:examples=omarchy brightness display apple | omarchy brightness display apple +5% | omarchy brightness display apple --no-osd 50% -device_cache="${XDG_RUNTIME_DIR:-/tmp}/omarchy-brightness-display-apple.device" +# Only cache under the user-private runtime dir. With no XDG_RUNTIME_DIR we skip +# caching (detect every run) rather than fall back to a predictable, world-writable +# /tmp path another user could pre-create. +device_cache="" +if [[ -n "${XDG_RUNTIME_DIR:-}" ]]; then + device_cache="$XDG_RUNTIME_DIR/omarchy-brightness-display-apple.device" +fi no_osd=0 if [[ ${1:-} == "--no-osd" ]]; then no_osd=1 @@ -28,9 +34,14 @@ find_apple_display_device() { local cached="" local device="" - if [[ -r $device_cache ]]; then + if [[ -n "$device_cache" && -r $device_cache ]]; then read -r cached <"$device_cache" || true - if [[ -n $cached && -e $cached ]]; then + # Trust a cached value only if it still names a hiddev character device. A + # stale or unexpected cache (a regular file, a non-hiddev node) is ignored and + # we re-detect instead of handing an arbitrary path to asdcontrol. The globs + # are left unquoted on purpose: [[ ]] pattern-matches an unquoted right side, + # and quoting them would turn the match into a literal string comparison. + if [[ ( $cached == /dev/hiddev* || $cached == /dev/usb/hiddev* ) && -c $cached ]]; then printf '%s\n' "$cached" return 0 fi @@ -39,7 +50,9 @@ find_apple_display_device() { device="$(detect_apple_display_device)" || return 1 [[ -n $device ]] || return 1 - printf '%s\n' "$device" >"$device_cache" + if [[ -n "$device_cache" ]]; then + printf '%s\n' "$device" >"$device_cache" + fi printf '%s\n' "$device" } diff --git a/test/shell.d/brightness-display-apple-cache-test.sh b/test/shell.d/brightness-display-apple-cache-test.sh new file mode 100755 index 00000000..2808f93f --- /dev/null +++ b/test/shell.d/brightness-display-apple-cache-test.sh @@ -0,0 +1,120 @@ +#!/bin/bash + +set -euo pipefail + +source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/base-test.sh" + +TMPDIR=$(mktemp -d) +trap 'rm -rf "$TMPDIR"' EXIT + +# Stubs on PATH: drop sudo so asdcontrol runs directly, record every asdcontrol +# invocation, make detection deterministic by having --detect report no device, +# and no-op the OSD. On a host without any /dev/*hiddev* node the wrapper's +# detect_apple_display_device returns before it ever runs asdcontrol, so the +# reject cases assert on the negative: a refused cache value is never handed to +# `asdcontrol -- `. Blind-trust validation would hand it over and be +# caught here. +stub_dir="$TMPDIR/stubs" +mkdir -p "$stub_dir" + +asd_log="$TMPDIR/asdcontrol.log" + +cat >"$stub_dir/sudo" <<'STUB' +#!/bin/bash +exec "$@" +STUB +chmod +x "$stub_dir/sudo" + +cat >"$stub_dir/asdcontrol" <>"$asd_log" +# --detect reports nothing, so detection never yields a device. +if [[ \$1 == "--detect" ]]; then + exit 0 +fi +# A brightness read (a lone device arg) returns a plausible value; a set +# ( -- ) just succeeds. +if [[ \$# -eq 1 ]]; then + printf '%s: BRIGHTNESS=30000\n' "\$1" +fi +exit 0 +STUB +chmod +x "$stub_dir/asdcontrol" + +cat >"$stub_dir/omarchy-osd" <<'STUB' +#!/bin/bash +exit 0 +STUB +chmod +x "$stub_dir/omarchy-osd" + +run_wrapper() { + # $1: value for XDG_RUNTIME_DIR ("" means unset); remaining args go to the wrapper. + local xdg="$1" + shift + : >"$asd_log" + if [[ -n $xdg ]]; then + XDG_RUNTIME_DIR="$xdg" PATH="$stub_dir:$ROOT/bin:$PATH" \ + omarchy-brightness-display-apple "$@" 2>&1 || true + else + env -u XDG_RUNTIME_DIR PATH="$stub_dir:$ROOT/bin:$PATH" \ + omarchy-brightness-display-apple "$@" 2>&1 || true + fi +} + +# --- A cache value that is not a hiddev character device is rejected ---------- +xdg_dir="$TMPDIR/xdg" +mkdir -p "$xdg_dir" +cache_file="$xdg_dir/omarchy-brightness-display-apple.device" + +regular_file="$TMPDIR/not-a-device" +: >"$regular_file" + +for poison in "/dev/null" "$regular_file" "/tmp/omarchy-evil"; do + printf '%s\n' "$poison" >"$cache_file" + output=$(run_wrapper "$xdg_dir" "+5%") + if grep -qF -- "$poison -- +5%" "$asd_log"; then + fail "wrapper handed a non-hiddev cache value to asdcontrol: $poison" "$output" + fi +done +pass "wrapper rejects a cached path that is not a hiddev character device" + +# NOTE: the complementary arm (a cache value that DOES match /dev/hiddev* but is +# not a character device) cannot be built without root -- only real device nodes +# live under /dev. It is covered by the -c test and exercised below only when a +# real hiddev node happens to be present. + +# --- A legitimate cached hiddev node is trusted (only where HW is present) ---- +real_hiddev="" +for candidate in /dev/usb/hiddev* /dev/hiddev*; do + if [[ -c $candidate ]]; then + real_hiddev="$candidate" + break + fi +done +if [[ -n $real_hiddev ]]; then + printf '%s\n' "$real_hiddev" >"$cache_file" + run_wrapper "$xdg_dir" "+5%" >/dev/null + grep -qF -- "$real_hiddev -- +5%" "$asd_log" || + fail "wrapper did not trust a valid cached hiddev node: $real_hiddev" + pass "wrapper trusts a cached hiddev character device without re-detecting" +else + pass "no /dev/hiddev* character device present; skipping the valid-cache case" +fi + +# --- With no XDG_RUNTIME_DIR, the predictable /tmp cache is not consulted ------ +# Guard on the real path not pre-existing so we never clobber a live cache, and +# remove what we create. Old code read /tmp and would hand /dev/null to +# asdcontrol; new code has no cache path at all when XDG_RUNTIME_DIR is unset. +tmp_cache="/tmp/omarchy-brightness-display-apple.device" +if [[ -e $tmp_cache ]]; then + pass "$tmp_cache already exists on this host; skipping the /tmp-fallback case" +else + printf '%s\n' "/dev/null" >"$tmp_cache" + output=$(run_wrapper "" "+5%") + used=1 + grep -qF -- "/dev/null -- +5%" "$asd_log" || used=0 + rm -f "$tmp_cache" + (( used == 0 )) || + fail "wrapper consulted the world-writable /tmp cache with no XDG_RUNTIME_DIR" "$output" + pass "wrapper ignores the /tmp cache path when XDG_RUNTIME_DIR is unset" +fi From e53548fae28ecd6b08dc2f7f5facf7f71f05621b Mon Sep 17 00:00:00 2001 From: bastidotnet <233381911+bastidotnet@users.noreply.github.com> Date: Tue, 25 Aug 2026 13:51:39 +0200 Subject: [PATCH 2/4] Harden the test's temp-file handling against a symlink race The /tmp-fallback case did check-then-create on a fixed /tmp name, a TOCTOU/symlink race, and the EXIT trap only cleaned $TMPDIR. Create the decoy atomically with noclobber (O_EXCL) so it refuses to overwrite an existing file or follow a symlink at that path, and remove it on exit only when this test created it. The fixed path is required (it is exactly the path the old code would form), so a random mktemp name cannot replace it. Addresses the Copilot review on #8198; the wrapper fix is unchanged. --- .../brightness-display-apple-cache-test.sh | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/test/shell.d/brightness-display-apple-cache-test.sh b/test/shell.d/brightness-display-apple-cache-test.sh index 2808f93f..c3ac8d53 100755 --- a/test/shell.d/brightness-display-apple-cache-test.sh +++ b/test/shell.d/brightness-display-apple-cache-test.sh @@ -5,7 +5,20 @@ set -euo pipefail source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/base-test.sh" TMPDIR=$(mktemp -d) -trap 'rm -rf "$TMPDIR"' EXIT +# The /tmp-fallback case (below) must place its decoy at exactly the fixed path the +# old wrapper would have formed, so it cannot use a random mktemp name. Track whether +# we created it and remove it on exit only then -- never touch a path we did not create. +tmp_cache="/tmp/omarchy-brightness-display-apple.device" +created_tmp_cache=0 + +cleanup() { + rm -rf "$TMPDIR" + # Remove the /tmp decoy only if this test is the one that created it. + if (( created_tmp_cache )); then + rm -f "$tmp_cache" + fi +} +trap cleanup EXIT # Stubs on PATH: drop sudo so asdcontrol runs directly, record every asdcontrol # invocation, make detection deterministic by having --detect report no device, @@ -102,14 +115,14 @@ else fi # --- With no XDG_RUNTIME_DIR, the predictable /tmp cache is not consulted ------ -# Guard on the real path not pre-existing so we never clobber a live cache, and -# remove what we create. Old code read /tmp and would hand /dev/null to -# asdcontrol; new code has no cache path at all when XDG_RUNTIME_DIR is unset. -tmp_cache="/tmp/omarchy-brightness-display-apple.device" -if [[ -e $tmp_cache ]]; then - pass "$tmp_cache already exists on this host; skipping the /tmp-fallback case" -else - printf '%s\n' "/dev/null" >"$tmp_cache" +# Create the decoy atomically with noclobber (O_EXCL) instead of check-then-create: +# this refuses to overwrite an existing file or follow a symlink at the fixed path, +# closing the TOCTOU/symlink race. The fixed path is required -- it is exactly the +# path the old code would have formed, so a decoy anywhere else would prove nothing. +# If the path is already taken, skip rather than touch it; the EXIT trap removes the +# decoy only when this test created it. +if ( set -C; printf '%s\n' "/dev/null" >"$tmp_cache" ) 2>/dev/null; then + created_tmp_cache=1 output=$(run_wrapper "" "+5%") used=1 grep -qF -- "/dev/null -- +5%" "$asd_log" || used=0 @@ -117,4 +130,6 @@ else (( used == 0 )) || fail "wrapper consulted the world-writable /tmp cache with no XDG_RUNTIME_DIR" "$output" pass "wrapper ignores the /tmp cache path when XDG_RUNTIME_DIR is unset" +else + pass "$tmp_cache already present or not safely creatable; skipping the /tmp-fallback case" fi From 9d8c0176d172a4397b560aae7b8fa751aac586dd Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Thu, 27 Aug 2026 21:40:31 +0200 Subject: [PATCH 3/4] Make the cache tests fail when either check is removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither half of the validation was covered. Dropping `&& -c $cached` from the wrapper left the whole file green: all three poison values fail on the pathname prefix, so none of them ever reached the character-device test. A path that matches the hiddev glob but is not a device now covers it, and it is the real case rather than a synthetic one -- the display replugs, the interface renumbers, and the cached node is gone. It is added only when the host has no such node, so a machine with the display attached cannot fail there spuriously. The no-XDG_RUNTIME_DIR assertion had the same problem for the opposite reason: its decoy held a path the validation rejects on its own, so restoring the `${XDG_RUNTIME_DIR:-/tmp}` fallback left it passing. It asserts on the open now instead of on the contents -- a FIFO with no writer blocks whoever opens it, so a wrapper that consults the path hangs and one that ignores it exits. mkfifo is atomic and fails outright if the path is taken, so it still neither overwrites a file nor follows a symlink at the fixed path. Clearing created_tmp_cache as soon as the decoy is removed keeps this run's EXIT trap from deleting a concurrent run's decoy at the same fixed path, which would have let that run pass against the old code. 🤖 Generated by Opus 5 in Claude Code. Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Codex XHigh --- .../brightness-display-apple-cache-test.sh | 51 ++++++++++++------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/test/shell.d/brightness-display-apple-cache-test.sh b/test/shell.d/brightness-display-apple-cache-test.sh index c3ac8d53..18b65cd0 100755 --- a/test/shell.d/brightness-display-apple-cache-test.sh +++ b/test/shell.d/brightness-display-apple-cache-test.sh @@ -82,7 +82,19 @@ cache_file="$xdg_dir/omarchy-brightness-display-apple.device" regular_file="$TMPDIR/not-a-device" : >"$regular_file" -for poison in "/dev/null" "$regular_file" "/tmp/omarchy-evil"; do +poisons=("/dev/null" "$regular_file" "/tmp/omarchy-evil") + +# The cases above all fail on the pathname prefix, so none of them reaches the -c +# test -- drop `&& -c $cached` from the wrapper and they all still pass. A path +# that matches the hiddev glob but is not a character device is what -c is for, +# and it is the realistic stale cache: the display replugs, the interface +# renumbers, and the cached node is simply gone. Add it only when the host really +# has no such node, so a machine with the display attached cannot fail here. +if [[ ! -e /dev/hiddev999 ]]; then + poisons+=("/dev/hiddev999") +fi + +for poison in "${poisons[@]}"; do printf '%s\n' "$poison" >"$cache_file" output=$(run_wrapper "$xdg_dir" "+5%") if grep -qF -- "$poison -- +5%" "$asd_log"; then @@ -91,10 +103,10 @@ for poison in "/dev/null" "$regular_file" "/tmp/omarchy-evil"; do done pass "wrapper rejects a cached path that is not a hiddev character device" -# NOTE: the complementary arm (a cache value that DOES match /dev/hiddev* but is -# not a character device) cannot be built without root -- only real device nodes -# live under /dev. It is covered by the -c test and exercised below only when a -# real hiddev node happens to be present. +# NOTE: the /dev/hiddev999 case above covers the -c test for a glob-matching path +# that does not exist. The remaining arm -- a path under /dev that exists, matches +# the glob, and is not a character device -- cannot be built without root, since +# only real device nodes live there. # --- A legitimate cached hiddev node is trusted (only where HW is present) ---- real_hiddev="" @@ -115,20 +127,25 @@ else fi # --- With no XDG_RUNTIME_DIR, the predictable /tmp cache is not consulted ------ -# Create the decoy atomically with noclobber (O_EXCL) instead of check-then-create: -# this refuses to overwrite an existing file or follow a symlink at the fixed path, -# closing the TOCTOU/symlink race. The fixed path is required -- it is exactly the -# path the old code would have formed, so a decoy anywhere else would prove nothing. -# If the path is already taken, skip rather than touch it; the EXIT trap removes the -# decoy only when this test created it. -if ( set -C; printf '%s\n' "/dev/null" >"$tmp_cache" ) 2>/dev/null; then +# Assert on the open, not on the contents. A decoy holding a rejectable path proves +# nothing: the validation above refuses it whether or not the /tmp fallback is still +# there, so that assertion passes against both wrappers. A FIFO with no writer blocks +# whoever opens it, so a wrapper that consults the path hangs and one that ignores it +# exits -- which separates the two. mkfifo is atomic and fails outright if the path is +# taken, so it neither overwrites a file nor follows a symlink; the fixed path is +# required, being exactly the path the old code would have formed. Clear the flag as +# soon as the decoy is gone, so a concurrent run's decoy cannot be removed by this +# run's EXIT trap. +if mkfifo "$tmp_cache" 2>/dev/null; then created_tmp_cache=1 - output=$(run_wrapper "" "+5%") - used=1 - grep -qF -- "/dev/null -- +5%" "$asd_log" || used=0 + status=0 + env -u XDG_RUNTIME_DIR PATH="$stub_dir:$ROOT/bin:$PATH" \ + timeout 5 omarchy-brightness-display-apple "+5%" >/dev/null 2>&1 || status=$? rm -f "$tmp_cache" - (( used == 0 )) || - fail "wrapper consulted the world-writable /tmp cache with no XDG_RUNTIME_DIR" "$output" + created_tmp_cache=0 + (( status != 124 )) || + fail "wrapper consulted the world-writable /tmp cache with no XDG_RUNTIME_DIR" \ + "it blocked reading the FIFO decoy at $tmp_cache" pass "wrapper ignores the /tmp cache path when XDG_RUNTIME_DIR is unset" else pass "$tmp_cache already present or not safely creatable; skipping the /tmp-fallback case" From d1845245d3c7441270f4da91e0032281486c2fd9 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Thu, 27 Aug 2026 21:40:31 +0200 Subject: [PATCH 4/4] Unquote the new variables inside [[ ]] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md asks for unquoted variables inside `[[ ]]`, with quotes reserved for string literals being compared. The three conditions added here quoted them. 🤖 Generated by Opus 5 in Claude Code. Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Codex XHigh --- bin/omarchy-brightness-display-apple | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bin/omarchy-brightness-display-apple b/bin/omarchy-brightness-display-apple index 1027686d..c9cce2ff 100755 --- a/bin/omarchy-brightness-display-apple +++ b/bin/omarchy-brightness-display-apple @@ -8,7 +8,7 @@ # caching (detect every run) rather than fall back to a predictable, world-writable # /tmp path another user could pre-create. device_cache="" -if [[ -n "${XDG_RUNTIME_DIR:-}" ]]; then +if [[ -n ${XDG_RUNTIME_DIR:-} ]]; then device_cache="$XDG_RUNTIME_DIR/omarchy-brightness-display-apple.device" fi no_osd=0 @@ -34,7 +34,7 @@ find_apple_display_device() { local cached="" local device="" - if [[ -n "$device_cache" && -r $device_cache ]]; then + if [[ -n $device_cache && -r $device_cache ]]; then read -r cached <"$device_cache" || true # Trust a cached value only if it still names a hiddev character device. A # stale or unexpected cache (a regular file, a non-hiddev node) is ignored and @@ -50,7 +50,7 @@ find_apple_display_device() { device="$(detect_apple_display_device)" || return 1 [[ -n $device ]] || return 1 - if [[ -n "$device_cache" ]]; then + if [[ -n $device_cache ]]; then printf '%s\n' "$device" >"$device_cache" fi printf '%s\n' "$device"