Skip to content

fix: fail open instead of aborting under set -e on 1Password DB lookup#31

Open
J-MaFf wants to merge 1 commit into
1Password:mainfrom
J-MaFf:fix/hook-set-e-fail-open
Open

fix: fail open instead of aborting under set -e on 1Password DB lookup#31
J-MaFf wants to merge 1 commit into
1Password:mainfrom
J-MaFf:fix/hook-set-e-fail-open

Conversation

@J-MaFf

@J-MaFf J-MaFf commented Jun 16, 2026

Copy link
Copy Markdown

Problem

hooks/1password-validate-mounted-env-files/hook.sh runs under set -euo pipefail. find_1password_db and query_mounts return non-zero as a normal "not found / unavailable" signal, but their results are assigned via command substitution:

db_path=$(find_1password_db "$os_type")
mount_hex_data=$(query_mounts "$db_path")

Under set -e, a non-zero command substitution in an assignment aborts the whole script. So on any machine without the 1Password desktop database on disk (CLI-only, CI), or without sqlite3 installed, the hook exits 1 with empty stdout before reaching its fail-open logic.

bin/run-hook.sh masks this by treating empty hook output as "allow", so the end result is usually still safe — but the hook never reaches its own intended fail-open branches, its log warnings never fire, and configured-mode deny is short-circuited: a required-but-missing mount can't block, because the script dies before validating.

Reproduction

On a machine with no 1Password desktop database present:

echo '{"client":"cursor","event":"before_shell_execution","type":"command","workspace_roots":["/tmp"],"cwd":"/tmp","command":"echo hi","raw_payload":{}}' \
  | env HOME="$(mktemp -d)" bash hooks/1password-validate-mounted-env-files/hook.sh
echo "exit=$?"
  • Actual: exit=1, empty stdout (aborted by set -e).
  • Expected: exit=0, {"decision":"allow","message":""} (fail open as designed).

Fix

Guard both assignments with || true — the same idiom already used in extract_toml_array_items — so the existing emptiness checks (if [[ -n "$db_path" ]]) run and the hook falls through to its fail-open allow.

Tests

Added two regression tests to tests/hooks/1password-validate-mounted-env-files.bats:

  • fails open (allow) when no 1Password database is present (covers the find_1password_db path)
  • fails open (allow) when the 1Password database cannot be queried (covers the query_mounts path)

Both fail before this change and pass after, and run without sqlite3 — exactly the path the existing deny test skips when sqlite3 is unavailable, which is why the regression went uncaught.

Full suite: 160/160 passing (npx bats -r tests/).

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) <noreply@anthropic.com>
@J-MaFf J-MaFf marked this pull request as ready for review June 16, 2026 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant