From b836b5cdd6dfa0299b5ed70db58802683890d933 Mon Sep 17 00:00:00 2001 From: Joey Maffiola <7maffiolajoey@gmail.com> Date: Tue, 16 Jun 2026 01:23:36 +0000 Subject: [PATCH 1/3] fix: fail open instead of aborting under set -e on 1Password DB lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hook.sh runs under `set -euo pipefail`. find_1password_db and query_mounts return non-zero as a normal "not found / unavailable" signal, but assigning their output (db_path=$(...) / mount_hex_data=$(...)) makes set -e abort the whole script before the fail-open logic runs — exiting 1 with empty stdout instead of allowing. The runner masks this by treating empty output as allow, but the hook never reaches its own fail-open branches, its log warnings never fire, and configured-mode deny is short-circuited (a missing required mount cannot block). Guard both assignments with `|| true` so the existing emptiness checks run and the hook fails open as designed — matching the `|| true` idiom already used in extract_toml_array_items. - Modified: hooks/1password-validate-mounted-env-files/hook.sh (~line 415, 417) - Tests: tests/hooks/1password-validate-mounted-env-files.bats — added two regression tests (no DB present; DB present but unqueryable). Both fail before the fix and pass after. Full suite: 160/160 passing. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../hook.sh | 10 ++- .../1password-validate-mounted-env-files.bats | 66 +++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/hooks/1password-validate-mounted-env-files/hook.sh b/hooks/1password-validate-mounted-env-files/hook.sh index 0d25a5c..9fd2627 100755 --- a/hooks/1password-validate-mounted-env-files/hook.sh +++ b/hooks/1password-validate-mounted-env-files/hook.sh @@ -424,9 +424,15 @@ db_path="" mount_hex_data="" if [[ "$os_type" != "unknown" ]]; then - db_path=$(find_1password_db "$os_type") + # find_1password_db and query_mounts return non-zero as a normal "not found / + # unavailable" signal (e.g. no 1Password desktop database on disk, or sqlite3 + # not installed). Under `set -euo pipefail`, a non-zero command substitution in + # an assignment aborts the whole script — which would skip the fail-open logic + # below and crash instead of allowing. Guard with `|| true` so we fall through + # to the existing emptiness checks and fail open as designed. + db_path=$(find_1password_db "$os_type") || true if [[ -n "$db_path" ]]; then - mount_hex_data=$(query_mounts "$db_path") + mount_hex_data=$(query_mounts "$db_path") || true fi fi diff --git a/tests/hooks/1password-validate-mounted-env-files.bats b/tests/hooks/1password-validate-mounted-env-files.bats index 30fffdc..363543e 100644 --- a/tests/hooks/1password-validate-mounted-env-files.bats +++ b/tests/hooks/1password-validate-mounted-env-files.bats @@ -213,3 +213,69 @@ TOML assert_lines ".env" } +# ============================================================================ +# Fail-open regression tests for `set -e` aborts on the 1Password DB lookup +# ============================================================================ +# +# find_1password_db and query_mounts use a non-zero return as a normal +# "not found / unavailable" signal. Under `set -euo pipefail`, a non-zero +# command substitution in an assignment aborts the script before the +# fail-open logic runs — so the hook would exit 1 with empty stdout instead +# of allowing. These two tests pin the intended fail-open behavior and run +# without sqlite3 (the exact path the prior `deny` test skips). + +# Build a canonical-input payload for one workspace root. +_canonical_for_root() { + python3 -c "import json,sys; print(json.dumps({ + 'client': 'cursor', + 'event': 'before_shell_execution', + 'type': 'command', + 'workspace_roots': [sys.argv[1]], + 'cwd': sys.argv[1], + 'command': 'echo hi', + 'raw_payload': {}, + }))" "$1" +} + +@test "fails open (allow) when no 1Password database is present" { + # Regression for line ~415: db_path=$(find_1password_db ...) returns + # non-zero when no database exists on disk. + local home="${BATS_TEST_TMPDIR}/home" + mkdir -p "$home" # intentionally no 1Password sqlite database + + local ws="${BATS_TEST_TMPDIR}/workspace" + mkdir -p "$ws" # no .1password/environments.toml -> default mode + + run env HOME="$home" bash "$HOOK_SCRIPT" <<<"$(_canonical_for_root "$ws")" + + [[ $status -eq 0 ]] + [[ "$output" == '{"decision":"allow","message":""}' ]] +} + +@test "fails open (allow) when the 1Password database cannot be queried" { + # Regression for line ~417: mount_hex_data=$(query_mounts ...) returns + # non-zero when sqlite3 is missing or the database is invalid/unreadable. + # A present-but-invalid db file reaches query_mounts and forces the + # non-zero path regardless of whether sqlite3 is installed. + local home="${BATS_TEST_TMPDIR}/home" + local db_dir + case "$(uname -s)" in + Darwin*) + db_dir="${home}/Library/Group Containers/2BUA8C4S2C.com.1password/Library/Application Support/1Password/Data" + ;; + *) + db_dir="${home}/.config/1Password" + ;; + esac + mkdir -p "$db_dir" + printf 'not-a-valid-sqlite-database' > "${db_dir}/1Password.sqlite" + + local ws="${BATS_TEST_TMPDIR}/workspace" + mkdir -p "$ws" + + run env HOME="$home" bash "$HOOK_SCRIPT" <<<"$(_canonical_for_root "$ws")" + + [[ $status -eq 0 ]] + [[ "$output" == '{"decision":"allow","message":""}' ]] +} + From 3845ac72b216cce6952cd93c09323bb5933c43f7 Mon Sep 17 00:00:00 2001 From: Joey Maffiola <7maffiolajoey@gmail.com> Date: Sun, 5 Jul 2026 22:21:07 -0400 Subject: [PATCH 2/3] test: pin configured-mode deny on DB-less machines; dedupe test helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the PR surfaced three gaps in the new regression tests: 1. The headline behavior this fix enables — configured-mode (TOML) deny on machines without the 1Password desktop database — was not pinned by any test: a mutation that short-circuits to allow when db_path is empty kept the whole suite green. Added "denies TOML-required mounts even when no 1Password database is present" (fails before the fix with exit 1 / empty stdout, passes after). 2. The per-OS DB path logic existed twice in this file (fixture + new invalid-db test); if the copies drifted the invalid-db test would pass vacuously via the "no db" branch. Extracted _1password_data_dir and used it in both places. 3. _canonical_for_root duplicated the byte-identical inline python3 payload builder in the existing deny test; the deny test now calls the helper. Also dropped the "line ~415/~417" comment references (already stale at PR head — the guarded assignments sit at 421/423) in favor of the assignment text, and trimmed the section banner to point at the hook.sh comment instead of restating it. - Modified: tests/hooks/1password-validate-mounted-env-files.bats only - Tested: bats -r tests/ — 161/161 passing; new deny test verified to fail against the pre-fix hook.sh (merge-base) and pass at PR head --- .../1password-validate-mounted-env-files.bats | 109 ++++++++++-------- 1 file changed, 58 insertions(+), 51 deletions(-) diff --git a/tests/hooks/1password-validate-mounted-env-files.bats b/tests/hooks/1password-validate-mounted-env-files.bats index 363543e..6cc4a7c 100644 --- a/tests/hooks/1password-validate-mounted-env-files.bats +++ b/tests/hooks/1password-validate-mounted-env-files.bats @@ -4,22 +4,43 @@ load "../test_helper" HOOK_SCRIPT="${PROJECT_ROOT}/hooks/1password-validate-mounted-env-files/hook.sh" -# Minimal SQLite DB at the path find_1password_db expects; query_mounts requires objects_associated. -create_minimal_1password_sqlite_fixture() { - local fake_home="$1" - local db_path +# Per-OS 1Password data directory under a given home (mirrors the primary +# paths find_1password_db searches). +_1password_data_dir() { + local home="$1" case "$(uname -s)" in Darwin*) - db_path="${fake_home}/Library/Group Containers/2BUA8C4S2C.com.1password/Library/Application Support/1Password/Data/1Password.sqlite" + echo "${home}/Library/Group Containers/2BUA8C4S2C.com.1password/Library/Application Support/1Password/Data" ;; *) - db_path="${fake_home}/.config/1Password/1Password.sqlite" + echo "${home}/.config/1Password" ;; esac +} + +# Minimal SQLite DB at the path find_1password_db expects; query_mounts requires objects_associated. +create_minimal_1password_sqlite_fixture() { + local fake_home="$1" + local db_path + db_path="$(_1password_data_dir "$fake_home")/1Password.sqlite" mkdir -p "$(dirname "$db_path")" sqlite3 "$db_path" 'CREATE TABLE objects_associated (key_name TEXT, data BLOB);' } +# Build a canonical-input payload for one workspace root (python3 for +# JSON-safe escaping of tmpdir paths). +_canonical_for_root() { + python3 -c "import json,sys; print(json.dumps({ + 'client': 'cursor', + 'event': 'before_shell_execution', + 'type': 'command', + 'workspace_roots': [sys.argv[1]], + 'cwd': sys.argv[1], + 'command': 'echo hi', + 'raw_payload': {}, + }))" "$1" +} + canonical_empty_roots='{"client":"cursor","event":"before_shell_execution","type":"command","workspace_roots":[],"cwd":"","command":"echo hi","raw_payload":{}}' canonical_one_root='{"client":"cursor","event":"before_shell_execution","type":"command","workspace_roots":["/tmp"],"cwd":"/tmp","command":"echo hi","raw_payload":{}}' @@ -50,18 +71,7 @@ canonical_one_root='{"client":"cursor","event":"before_shell_execution","type":" mkdir -p "$ws/.1password" printf '%s\n' 'mount_paths = [".env.missing"]' > "$ws/.1password/environments.toml" - local payload - payload=$(python3 -c "import json,sys; print(json.dumps({ - 'client': 'cursor', - 'event': 'before_shell_execution', - 'type': 'command', - 'workspace_roots': [sys.argv[1]], - 'cwd': sys.argv[1], - 'command': 'echo hi', - 'raw_payload': {}, - }))" "$ws") - - run env HOME="$HOME" bash "$HOOK_SCRIPT" <<<"$payload" + run env HOME="$HOME" bash "$HOOK_SCRIPT" <<<"$(_canonical_for_root "$ws")" [[ $status -eq 1 ]] [[ $(printf '%s\n' "$output" | wc -l) -eq 1 ]] printf '%s' "$output" | python3 -c 'import json,sys; d=json.load(sys.stdin); assert d.get("decision")=="deny" and d.get("message"), d' @@ -214,32 +224,17 @@ TOML } # ============================================================================ -# Fail-open regression tests for `set -e` aborts on the 1Password DB lookup +# Regression tests for `set -e` aborts on the 1Password DB lookup # ============================================================================ # -# find_1password_db and query_mounts use a non-zero return as a normal -# "not found / unavailable" signal. Under `set -euo pipefail`, a non-zero -# command substitution in an assignment aborts the script before the -# fail-open logic runs — so the hook would exit 1 with empty stdout instead -# of allowing. These two tests pin the intended fail-open behavior and run -# without sqlite3 (the exact path the prior `deny` test skips). - -# Build a canonical-input payload for one workspace root. -_canonical_for_root() { - python3 -c "import json,sys; print(json.dumps({ - 'client': 'cursor', - 'event': 'before_shell_execution', - 'type': 'command', - 'workspace_roots': [sys.argv[1]], - 'cwd': sys.argv[1], - 'command': 'echo hi', - 'raw_payload': {}, - }))" "$1" -} +# find_1password_db and query_mounts return non-zero as a normal "not found / +# unavailable" signal; the hook must handle that and decide, not abort under +# `set -euo pipefail` — see the comment above the `|| true` guards in hook.sh. +# These tests run without sqlite3 (the exact path the `deny` test above skips). @test "fails open (allow) when no 1Password database is present" { - # Regression for line ~415: db_path=$(find_1password_db ...) returns - # non-zero when no database exists on disk. + # Regression: db_path=$(find_1password_db ...) returns non-zero when no + # database exists on disk. local home="${BATS_TEST_TMPDIR}/home" mkdir -p "$home" # intentionally no 1Password sqlite database @@ -253,20 +248,13 @@ _canonical_for_root() { } @test "fails open (allow) when the 1Password database cannot be queried" { - # Regression for line ~417: mount_hex_data=$(query_mounts ...) returns - # non-zero when sqlite3 is missing or the database is invalid/unreadable. - # A present-but-invalid db file reaches query_mounts and forces the + # Regression: mount_hex_data=$(query_mounts ...) returns non-zero when + # sqlite3 is missing or the database is invalid/unreadable. A + # present-but-invalid db file reaches query_mounts and forces the # non-zero path regardless of whether sqlite3 is installed. local home="${BATS_TEST_TMPDIR}/home" local db_dir - case "$(uname -s)" in - Darwin*) - db_dir="${home}/Library/Group Containers/2BUA8C4S2C.com.1password/Library/Application Support/1Password/Data" - ;; - *) - db_dir="${home}/.config/1Password" - ;; - esac + db_dir="$(_1password_data_dir "$home")" mkdir -p "$db_dir" printf 'not-a-valid-sqlite-database' > "${db_dir}/1Password.sqlite" @@ -279,3 +267,22 @@ _canonical_for_root() { [[ "$output" == '{"decision":"allow","message":""}' ]] } +@test "denies TOML-required mounts even when no 1Password database is present" { + # Pins configured-mode deny on machines without the desktop database — + # the behavior the `|| true` guards make reachable. Before the fix the + # hook aborted before validating, so a required-but-missing mount could + # never block on these machines. + local home="${BATS_TEST_TMPDIR}/home" + mkdir -p "$home" # intentionally no 1Password sqlite database + + local ws="${BATS_TEST_TMPDIR}/workspace" + mkdir -p "$ws/.1password" + printf '%s\n' 'mount_paths = [".env.missing"]' > "$ws/.1password/environments.toml" + + run env HOME="$home" bash "$HOOK_SCRIPT" <<<"$(_canonical_for_root "$ws")" + + [[ $status -eq 1 ]] + [[ $(printf '%s\n' "$output" | wc -l) -eq 1 ]] + printf '%s' "$output" | python3 -c 'import json,sys; d=json.load(sys.stdin); assert d.get("decision")=="deny" and d.get("message"), d' +} + From 84151fe57c478a900a7bd09b944221d3000e65d4 Mon Sep 17 00:00:00 2001 From: Joey Maffiola <7maffiolajoey@gmail.com> Date: Tue, 7 Jul 2026 02:27:18 +0000 Subject: [PATCH 3/3] test: match telemetry-aware allow output after rebase onto main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased fix/hook-set-e-fail-open onto current 1Password/main (was 13 commits behind). Upstream's telemetry work (f745d34, "Add Telemetry Reporting for Hooks") extended the allow decision output with the mode/mount_count/deny_reason fields, so the two fail-open regression tests — which pinned the pre-telemetry `{"decision":"allow","message":""}` string — needed updating to the current `{"decision":"allow","message":"","mode":"default","mount_count":0,"deny_reason":null}` shape, matching how the suite's other allow tests now assert. The fix itself (the `|| true` guards) is unchanged and still correct: the hook reaches its fail-open branch and exits 0. The `denies TOML-required mounts` regression test already passed against upstream's refactored deny logic. Full suite: 199/199 passing (npx bats -r tests/) on top of current main. --- tests/hooks/1password-validate-mounted-env-files.bats | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/hooks/1password-validate-mounted-env-files.bats b/tests/hooks/1password-validate-mounted-env-files.bats index 6cc4a7c..07ad7d3 100644 --- a/tests/hooks/1password-validate-mounted-env-files.bats +++ b/tests/hooks/1password-validate-mounted-env-files.bats @@ -244,7 +244,7 @@ TOML run env HOME="$home" bash "$HOOK_SCRIPT" <<<"$(_canonical_for_root "$ws")" [[ $status -eq 0 ]] - [[ "$output" == '{"decision":"allow","message":""}' ]] + [[ "$output" == '{"decision":"allow","message":"","mode":"default","mount_count":0,"deny_reason":null}' ]] } @test "fails open (allow) when the 1Password database cannot be queried" { @@ -264,7 +264,7 @@ TOML run env HOME="$home" bash "$HOOK_SCRIPT" <<<"$(_canonical_for_root "$ws")" [[ $status -eq 0 ]] - [[ "$output" == '{"decision":"allow","message":""}' ]] + [[ "$output" == '{"decision":"allow","message":"","mode":"default","mount_count":0,"deny_reason":null}' ]] } @test "denies TOML-required mounts even when no 1Password database is present" {