From fe6949687a148ab59a2934ff06ef0b86faabd9e9 Mon Sep 17 00:00:00 2001 From: Claude Code Bot Date: Mon, 14 Sep 2026 19:17:57 -0700 Subject: [PATCH 1/3] fix(credentials): fail closed when GH_TOKEN vault fetch fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `op read` failed, _load_gh_token left GH_TOKEN unset and logged a warning saying gh would "use keyring fallback". That fallback is the user's keyring OAuth token, which carries repo, workflow, admin:org, delete_repo and admin:public_key — far wider than the restricted CCCLI PAT this function exists to inject. The effect was silent privilege escalation: any transient op failure (locked vault, missing service account token, network blip, backoff exhausted) handed the agent full admin access, announced only as a launch-time warning. The degraded path was strictly more privileged than the healthy one, which inverts the intent of injecting a restricted token at all. Export a deliberately invalid sentinel instead, so gh fails with a clear auth error rather than quietly succeeding with more access than intended. The "already set, skip lookup" guard now treats that sentinel as unset, so a retry can still reach the vault instead of being short-circuited by a previous failure. Verified against the pre-change code: on a failing `op read`, old behavior left GH_TOKEN with length 0 (unset -> keyring fallback); new behavior exports a 38-char sentinel (-> gh fails closed). Adds three tests covering the failure path, which previously had none: the sentinel is exported, the warning is emitted, and a present sentinel does not suppress a later vault lookup. Also corrects the file header, which claimed these credentials are "not present in the interactive shell environment". OP_SERVICE_ACCOUNT_TOKEN is in fact inherited by child processes of the wrapper, including the agent's shell. Refs #120 Claude-Session: https://claude.ai/code/session_015WZskZun8am3TWVS5pMYii --- lib/credentials.sh | 29 ++++++++++++++++++++++---- tests/test-credentials.sh | 44 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/lib/credentials.sh b/lib/credentials.sh index fec44dd..13011ac 100755 --- a/lib/credentials.sh +++ b/lib/credentials.sh @@ -2,8 +2,13 @@ # credentials.sh — inject CCCLI credentials from 1Password at wrapper launch # # Fetches OP_SERVICE_ACCOUNT_TOKEN from macOS Keychain and GH_TOKEN from the -# Automation vault. Both are scoped to the wrapper process lifetime only — -# they are not present in the interactive shell environment. +# Automation vault. Both are exported into the wrapper process and inherited +# by everything it launches, including the agent's own shell — they are not +# present in the user's login shell, but they ARE visible to child processes. +# +# If the vault fetch fails, GH_TOKEN is set to a deliberately invalid sentinel +# rather than left unset, so gh fails closed instead of falling back to the +# keyring OAuth token (which holds admin:org). See _load_gh_token. # # Requires: lib/logging.sh must be sourced first. # Must be sourced before lib/secrets-loader.sh (which needs OP_SERVICE_ACCOUNT_TOKEN). @@ -18,6 +23,11 @@ readonly _CREDENTIALS_SH_LOADED=1 readonly _CREDS_KEYCHAIN_SERVICE="op-service-account-claude-automation" readonly _CREDS_GH_TOKEN_REF="op://Automation/GitHub - CCCLI/Token" +# Sentinel exported when the vault fetch fails, so gh fails closed instead of +# falling back to the keyring OAuth token. Must be non-empty (an empty value +# would re-enable the keyring fallback) and must not be a valid credential. +readonly _CREDS_GH_TOKEN_FETCH_FAILED="invalid-cccli-token-vault-fetch-failed" + # Computed once so both credential-fetch functions below don't each re-run # `command -v timeout` on every invocation. _CREDS_HAS_TIMEOUT=false @@ -74,7 +84,9 @@ _load_service_account_token() { # GH_TOKEN is the restricted-scope CCCLI PAT, separate from the # personal token in gh's keyring. _load_gh_token() { - if [[ -n "${GH_TOKEN:-}" ]]; then + # A previously-exported failure sentinel is not a real token: treat it as + # unset so a retry can still reach the vault rather than being skipped. + if [[ -n "${GH_TOKEN:-}" && "${GH_TOKEN}" != "${_CREDS_GH_TOKEN_FETCH_FAILED}" ]]; then debug_log "GH_TOKEN already set, skipping vault lookup" return 0 fi @@ -112,7 +124,16 @@ _load_gh_token() { export GH_TOKEN="${token}" debug_log "GH_TOKEN loaded from Automation vault" else - log_warn "Failed to fetch GH_TOKEN from 1Password — gh CLI will use keyring fallback" + # Fail closed. Leaving GH_TOKEN unset makes gh fall back to the user's + # keyring OAuth token, which carries repo/workflow/admin:org/delete_repo — + # far wider than the restricted CCCLI PAT this function exists to inject. + # A transient op failure (locked vault, missing service account token, + # network blip, backoff exhausted) must not silently upgrade the agent's + # GitHub privileges. Export a deliberately invalid sentinel instead: gh + # then fails with a clear auth error rather than quietly succeeding with + # more access than intended. + export GH_TOKEN="${_CREDS_GH_TOKEN_FETCH_FAILED}" + log_warn "Failed to fetch GH_TOKEN from 1Password — gh disabled for this session (keyring fallback deliberately blocked)" fi unset token } diff --git a/tests/test-credentials.sh b/tests/test-credentials.sh index f39315c..56bf3d5 100755 --- a/tests/test-credentials.sh +++ b/tests/test-credentials.sh @@ -190,6 +190,50 @@ assert_equals "vault-gh-token" "${result}" \ assert_equals "1" "$(wc -l <"${call_log}" | tr -d ' ')" \ "op read succeeds on first attempt -> called exactly once" +# op read fails (all attempts) -> GH_TOKEN exported as the invalid sentinel, +# NOT left unset. Leaving it unset would let gh fall back to the user's keyring +# OAuth token (repo/workflow/admin:org), silently widening the agent's access +# on any transient vault failure. Fail closed instead. +stub_dir="$(make_stub_dir env bash cat id security timeout)" +cat >"${stub_dir}/op" <<'EOF' +#!/usr/bin/env bash +exit 1 +EOF +chmod +x "${stub_dir}/op" +result="$( + PATH="${stub_dir}" \ + OP_SERVICE_ACCOUNT_TOKEN="dummy" \ + bash -c "unset GH_TOKEN; source '${LIB_DIR}/logging.sh'; source '${LIB_DIR}/credentials.sh'; echo \"\${GH_TOKEN:-unset}\"" 2>/dev/null +)" +assert_equals "invalid-cccli-token-vault-fetch-failed" "${result}" \ + "op read fails -> GH_TOKEN set to invalid sentinel (fails closed, no keyring fallback)" + +# The same failure must warn, so a degraded session is visible rather than silent. +warn_output="$( + PATH="${stub_dir}" \ + OP_SERVICE_ACCOUNT_TOKEN="dummy" \ + bash -c "unset GH_TOKEN; source '${LIB_DIR}/logging.sh'; source '${LIB_DIR}/credentials.sh'" 2>&1 1>/dev/null +)" +assert_contains "gh disabled for this session" "${warn_output}" \ + "op read fails -> warns that gh is disabled" + +# A previously-set sentinel must not short-circuit a later retry: it is a +# failure marker, not a real token. +stub_dir="$(make_stub_dir env bash cat id security timeout)" +cat >"${stub_dir}/op" <<'EOF' +#!/usr/bin/env bash +echo "recovered-gh-token" +EOF +chmod +x "${stub_dir}/op" +result="$( + PATH="${stub_dir}" \ + OP_SERVICE_ACCOUNT_TOKEN="dummy" \ + GH_TOKEN="invalid-cccli-token-vault-fetch-failed" \ + bash -c "source '${LIB_DIR}/logging.sh'; source '${LIB_DIR}/credentials.sh'; echo \"\${GH_TOKEN:-unset}\"" 2>/dev/null +)" +assert_equals "recovered-gh-token" "${result}" \ + "sentinel present -> vault lookup still runs (sentinel is not a valid preset)" + # --- Summary --- echo "" From d4c9d9cf23fa11ffbd35c57ffcaf48467e2d40df Mon Sep 17 00:00:00 2001 From: Claude Code Bot Date: Tue, 15 Sep 2026 08:09:11 -0700 Subject: [PATCH 2/3] fix(lint): add repo-level .shellcheckrc so CI resolves sourced files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit standards-check failed on this PR with SC1091 for tests/test-credentials.sh line 18, which sources tests/lib/op-guard.sh through a variable path. The `# shellcheck source=tests/lib/op-guard.sh` directive at that call site already names the real target, but shellcheck only follows a source directive when external sourcing is enabled. The fleet's run-standards.sh invokes `shellcheck -S info` with no `-x` and no `--rcfile`, so nothing enabled it. Local runs passed the whole time because, absent an rc in the repo, shellcheck falls back to the developer's ~/.shellcheckrc, which sets external-sources=true. CI has no such file. That is a local/CI divergence no amount of local linting could surface — the local check was structurally incapable of failing. Verified as a known-bad/known-good pair rather than assuming: shellcheck --norc -S info tests/test-credentials.sh -> SC1091, exit 1 shellcheck --rcfile=.shellcheckrc -S info -> exit 0 `--norc` is the only faithful local stand-in for CI here: overriding $HOME does not suppress the user-level rc, so an earlier attempt to neutralize it that way produced a false pass and proved nothing. Repo-wide re-lint with external sourcing enabled is clean, so following the sourced files surfaces no new findings: shellcheck --rcfile=.shellcheckrc -S info lib/*.sh tests/*.sh \ tests/lib/*.sh bin/claude-wrapper -> exit 0 Scoped to a repo-level lint config with no single owning module, so the full suite ran: test-credentials 14/14, test-launch-dir-check 6/6, test-remote-session 26/26, test-wrapper 64/64. This also fixes the same latent failure in tests/test-wrapper.sh:24, which carries an identical directive and passes today only because it was outside this PR's changed-file scope. Claude-Session: https://claude.ai/code/session_011r87TBgqE338FGJn5zpZ52 --- .shellcheckrc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 .shellcheckrc diff --git a/.shellcheckrc b/.shellcheckrc new file mode 100644 index 0000000..a6606c5 --- /dev/null +++ b/.shellcheckrc @@ -0,0 +1,15 @@ +# Repo-level shellcheck config. +# +# Every lib/*.sh sources its siblings through a variable path +# (`source "${WRAPPER_LIB}/logging.sh"`, `source "${TEST_DIR}/lib/op-guard.sh"`), +# which shellcheck cannot resolve on its own. The `# shellcheck source=...` +# directives at those call sites name the real target, but a directive is only +# followed when external sourcing is enabled — otherwise every one of them +# raises SC1091. +# +# Local runs passed anyway, because with no rc in the repo shellcheck fell +# back to the developer's ~/.shellcheckrc, which happens to set this. CI has +# no such file, so it saw the raw SC1091 and failed — a gap no local lint +# could catch. Setting it here makes both environments agree, and removes the +# need to remember `--external-sources` on every manual invocation. +external-sources=true From 70d75189f9097b326c160592ea7fdc90584cba08 Mon Sep 17 00:00:00 2001 From: Claude Code Bot Date: Tue, 15 Sep 2026 08:14:57 -0700 Subject: [PATCH 3/3] docs: drop --external-sources from lint guidance, now set by .shellcheckrc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repo-root .shellcheckrc added in the previous commit sets external-sources=true for every run, so the flag the Lint section told readers to pass is redundant. Both the Lint section and the Headroom ShellCheck notes still described the old workflow. Also removes the Headroom suggestion to suppress SC1091 via --exclude=SC1091. That advice hid the diagnostic rather than resolving the source path, and `# shellcheck disable` directives are disallowed in this repo's standards. Records why the config belongs in the repo instead of a personal ~/.shellcheckrc: CI runs bare `shellcheck -S info` with no user-level config, so a machine-local rc yields a passing local lint and a failing CI — the exact divergence that broke standards-check on PR #122. The Headroom block is marked auto-generated, but its header allows manual edits where accuracy requires them; this content was actively misleading. Verified the newly documented command runs clean exactly as written: shellcheck -S info lib/*.sh bin/claude-wrapper tests/*.sh tests/lib/*.sh -> exit 0 npx markdownlint-cli --config ~/.markdownlint.json CLAUDE.md -> exit 0 Claude-Session: https://claude.ai/code/session_011r87TBgqE338FGJn5zpZ52 --- CLAUDE.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9e533e1..e274b82 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -21,10 +21,12 @@ There is no test runner script — run individual test files directly. No `npm t ### Lint ```bash -shellcheck --external-sources lib/*.sh bin/claude-wrapper tests/*.sh +shellcheck -S info lib/*.sh bin/claude-wrapper tests/*.sh tests/lib/*.sh ``` -Use `--external-sources` because lib files source each other via variables (`${WRAPPER_LIB}/logging.sh`), and plain `shellcheck` emits false SC1091 warnings. +No `--external-sources` flag is needed: the repo-root `.shellcheckrc` sets `external-sources=true` for every run. Lib and test files source each other through variable paths (`${WRAPPER_LIB}/logging.sh`, `${TEST_DIR}/lib/op-guard.sh`), which shellcheck cannot resolve unless external sourcing is enabled — without it, the `# shellcheck source=...` directives at those call sites are ignored and each one raises a false SC1091. + +Keep that config in the repo rather than relying on a personal `~/.shellcheckrc`. CI (`standards-check`) runs bare `shellcheck -S info` with no flags and no user-level config, so a machine-local rc makes local lint pass while CI fails — see PR #122. ### Debug mode @@ -129,9 +131,9 @@ All secret files (`.op` files) must be owner-only permissions (no group/world). *~400 tokens/session saved* -- Run `shellcheck --external-sources ` (not plain `shellcheck`) for files that source other scripts via variables (e.g., `source "${WRAPPER_LIB}/logging.sh"`). Plain shellcheck produces SC1091 errors that are not real failures. -- `shellcheck` on `bin/claude-wrapper` produces SC1091 errors for sourced lib files; suppress with `--exclude=SC1091` or use `shellcheck --external-sources lib/file.sh` instead -- Always run `shellcheck` before committing shell script changes +- Run plain `shellcheck -S info `. The repo-root `.shellcheckrc` sets `external-sources=true`, so variable-path sources (`source "${WRAPPER_LIB}/logging.sh"`) resolve and the old `--external-sources` flag is redundant. +- Do not suppress SC1091 with `--exclude=SC1091` or a `# shellcheck disable` directive — the rc resolves it properly instead of hiding it. +- Always run `shellcheck` before committing shell script changes. ### Git Workflow