From 8bdbed12bbcd3e539826eeae177f7f20c4afd0be Mon Sep 17 00:00:00 2001 From: Claude Code Bot Date: Thu, 17 Sep 2026 19:29:39 -0700 Subject: [PATCH 1/3] feat(credentials): load every owner's PAT at launch, failing closed per token A fine-grained PAT is bound to one resource owner at creation, so no single token covers the fleet. Reading an org's repos with the personal token fails 403 on branch protection and 404 on the repo itself -- whether that repo is public or private, because the boundary is ownership, not visibility. Selecting one token per session from the launch directory therefore guarantees the wrong credential for any cross-owner work, which fleet probes and rollouts are by definition. Load all three as GH_TOKEN_SWM/NOS/TWM at launch. Additive: GH_TOKEN keeps its launch-directory selection, so nothing that reads it changes. Fetched once at launch rather than per use. A 42-repo probe would otherwise mean dozens of vault reads, making the correct path slower than the keyring fallback it replaces -- which is how workarounds get entrenched. Also extracts _creds_read_token_ref() from _load_gh_token(): the retry, backoff and timeout handling were already correct and are now shared rather than duplicated. It prints to stdout, returns 1 on failure, and never exports or logs a value. _load_gh_token's behavior is unchanged. Each token fails closed independently. A failed fetch exports the invalid sentinel, never an empty string: an empty value would let gh fall through to the user's keyring OAuth token (repo/workflow/admin:org), silently widening the agent's access on a transient vault failure. A partial failure poisons only its own variable. Tests: 8 new cases in test-credentials.sh (32/32), covering the success path, the all-fail and partial-fail closed paths, the warning, and the already-set guard. A known-bad gate runs first. Every assertion compares against the fixed sentinel literal or a stub-supplied value -- none prints a credential, because a test that must echo a live token to prove itself is the test that leaks it. Verified by mutation, not just by passing: - fail open (empty instead of sentinel) -> 2 failures - drop the already-set guard -> 3 failures - point every var at one ref -> 3 failures Full suite: 133/133 across test-credentials, test-launch-dir-check, test-remote-session and test-wrapper. shellcheck -S info clean. test-gh-token-permissions.sh's "Token authentication failed (got: twistedmelonman)" is PRE-EXISTING -- it reproduces identically on a clean tree. That failure is #126 itself: GH_TOKEN resolves from the launch directory, so a session started in a smartwatermelon repo still authenticates as twistedmelonman. This commit supplies the per-owner tokens that fix needs; it does not change GH_TOKEN selection. Advances #126 Claude-Session: https://claude.ai/code/session_017s2qrmkQaV54fbcMzB2KRm --- lib/credentials.sh | 125 ++++++++++++++++++++++++++++-------- tests/test-credentials.sh | 130 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 228 insertions(+), 27 deletions(-) diff --git a/lib/credentials.sh b/lib/credentials.sh index ef46fb0..c207e8a 100755 --- a/lib/credentials.sh +++ b/lib/credentials.sh @@ -136,6 +136,41 @@ _creds_gh_token_ref_for_owner() { esac } +# Read one vault reference, with bounded retries. Prints the token on stdout +# and returns 1 if every attempt failed. Callers decide what a failure means; +# this function never exports anything and never logs the value. +_creds_read_token_ref() { + local token_ref="$1" + local token + local -a backoff=(2 4 8) + local wait_secs + + if "${_CREDS_HAS_TIMEOUT}"; then + for wait_secs in "${backoff[@]}"; do + token="$(timeout "${wait_secs}" op read "${token_ref}" 2>/dev/null || true)" + if [[ -n "${token}" ]]; then + printf '%s' "${token}" + return 0 + fi + debug_log "op read failed (timeout ${wait_secs}s)" + done + else + # No timeout command available to bound each attempt, so retrying would + # only multiply the hang risk (an unbounded op read blocks forever on + # the first try, making retries unreachable) with no upside. Attempt + # exactly once instead of the usual backoff loop. + debug_log "timeout command unavailable, skipping retries (single attempt only)" + token="$(op read "${token_ref}" 2>/dev/null || true)" + if [[ -n "${token}" ]]; then + printf '%s' "${token}" + return 0 + fi + debug_log "op read failed (no timeout available)" + fi + + return 1 +} + # Fetch GH_TOKEN from Automation vault via service account. # Only runs if OP_SERVICE_ACCOUNT_TOKEN is available. # GH_TOKEN is the restricted-scope CCCLI PAT, separate from the @@ -153,40 +188,16 @@ _load_gh_token() { return 0 fi - local token - local -a backoff=(2 4 8) - local wait_secs - local owner token_ref + local token owner token_ref # ${PWD} is the user's launch directory: bin/claude-wrapper never changes # directory before sourcing this file, so the cwd it inherits is the one the - # session was started from. One op read per launch, not three — the token is - # selected, not accumulated. + # session was started from. owner="$(_creds_github_owner_for_dir "${PWD}")" token_ref="$(_creds_gh_token_ref_for_owner "${owner}")" debug_log "GitHub owner: ${owner:-} -> token ref: ${token_ref}" - if "${_CREDS_HAS_TIMEOUT}"; then - for wait_secs in "${backoff[@]}"; do - token="$(timeout "${wait_secs}" op read "${token_ref}" 2>/dev/null || true)" - if [[ -n "${token}" ]]; then - break - fi - debug_log "op read failed (timeout ${wait_secs}s)" - done - else - # No timeout command available to bound each attempt, so retrying would - # only multiply the hang risk (an unbounded op read blocks forever on - # the first try, making retries unreachable) with no upside. Attempt - # exactly once instead of the usual backoff loop. - debug_log "timeout command unavailable, skipping retries (single attempt only)" - token="$(op read "${token_ref}" 2>/dev/null || true)" - if [[ -z "${token}" ]]; then - debug_log "op read failed (no timeout available)" - fi - fi - - if [[ -n "${token}" ]]; then + if token="$(_creds_read_token_ref "${token_ref}")"; then export GH_TOKEN="${token}" debug_log "GH_TOKEN loaded from Automation vault (${token_ref})" else @@ -204,8 +215,68 @@ _load_gh_token() { unset token } +# Load every owner's token as GH_TOKEN_, so a session can act for an +# owner other than the one its launch directory happens to name. +# +# A fine-grained PAT is bound to one resource owner at creation, so no single +# token can cover the fleet: reading an org's repos with the personal token +# fails 403 on protection and 404 on the repo itself, whether that repo is +# public or private — the boundary is ownership, not visibility. Selecting one +# token per session (above) therefore guarantees the wrong credential for any +# cross-owner work, which fleet probes and rollouts are by definition. +# +# Fetched once at launch rather than per use. A 42-repo probe would otherwise +# mean dozens of vault reads, making the correct path slower than the keyring +# fallback it replaces — which is how workarounds get entrenched. +# +# These are additive: GH_TOKEN keeps its launch-directory selection, so nothing +# that reads it changes. See claude-wrapper#126 for the eventual target, where +# gh-wrapper.sh selects among these per invocation from the target repo's owner. +_load_owner_gh_tokens() { + if [[ -z "${OP_SERVICE_ACCOUNT_TOKEN:-}" ]]; then + debug_log "Skipping per-owner token fetch: OP_SERVICE_ACCOUNT_TOKEN not available" + return 0 + fi + + local spec var token_ref token loaded=0 + # var:ref pairs. The refs are the same constants _load_gh_token selects from. + local -a specs=( + "GH_TOKEN_SWM:${_CREDS_GH_TOKEN_REF_SMARTWATERMELON}" + "GH_TOKEN_NOS:${_CREDS_GH_TOKEN_REF_NIGHTOWLSTUDIOLLC}" + "GH_TOKEN_TWM:${_CREDS_GH_TOKEN_REF_PERSONAL}" + ) + + for spec in "${specs[@]}"; do + var="${spec%%:*}" + token_ref="${spec#*:}" + + # Respect a value already in the environment, matching _load_gh_token. + if [[ -n "${!var:-}" && "${!var}" != "${_CREDS_GH_TOKEN_FETCH_FAILED}" ]]; then + debug_log "${var} already set, skipping vault lookup" + ((loaded += 1)) + continue + fi + + if token="$(_creds_read_token_ref "${token_ref}")"; then + export "${var}=${token}" + debug_log "${var} loaded from Automation vault (${token_ref})" + ((loaded += 1)) + else + # Same fail-closed reasoning as GH_TOKEN: a caller that substitutes an + # empty value would fall through to the keyring OAuth token, silently + # widening scope. The sentinel makes gh fail with an auth error instead. + export "${var}=${_CREDS_GH_TOKEN_FETCH_FAILED}" + log_warn "Failed to fetch ${var} from 1Password — cross-owner gh calls for that owner will fail closed" + fi + unset token + done + + debug_log "Per-owner tokens available: ${loaded}/${#specs[@]}" +} + # ========================================================= # MAIN # ========================================================= _load_service_account_token _load_gh_token +_load_owner_gh_tokens diff --git a/tests/test-credentials.sh b/tests/test-credentials.sh index 7f309c8..4a80a7d 100755 --- a/tests/test-credentials.sh +++ b/tests/test-credentials.sh @@ -334,6 +334,136 @@ mkdir -p "${repo_dir}/nested/deeper" assert_equals "op://Automation/CCCLI-SWM/token" "$(ref_selected_from "${repo_dir}/nested/deeper")" \ "subdirectory of a repo -> enclosing repo's owner" +# --- Per-owner token loading (_load_owner_gh_tokens) --- +# +# These assert only against the fixed sentinel literal and stub-supplied +# values. No assertion prints a real credential, and the stub `op` never +# returns one — a test that must echo a live token to prove itself is the +# test that leaks it. + +# KNOWN-BAD GATE. Before asserting that the three vars get set, prove the +# assertion can fail: with no OP_SERVICE_ACCOUNT_TOKEN, nothing is exported +# and op is never called. A suite that only ever sees the success path cannot +# distinguish "loaded correctly" from "assertion never ran". +stub_dir="$(make_stub_dir env bash cat id timeout)" +call_log="${stub_dir}/op-calls.log" +cat >"${stub_dir}/op" <>"${call_log}" +echo "should-never-be-reached" +EOF +chmod +x "${stub_dir}/op" +result="$( + PATH="${stub_dir}" \ + bash -c "unset OP_SERVICE_ACCOUNT_TOKEN GH_TOKEN GH_TOKEN_SWM GH_TOKEN_NOS GH_TOKEN_TWM; source '${LIB_DIR}/logging.sh'; source '${LIB_DIR}/credentials.sh'; echo \"\${GH_TOKEN_SWM:-unset}/\${GH_TOKEN_NOS:-unset}/\${GH_TOKEN_TWM:-unset}\"" 2>/dev/null +)" +assert_equals "unset/unset/unset" "${result}" \ + "no OP_SERVICE_ACCOUNT_TOKEN -> per-owner tokens not exported" +assert_equals "" "$([[ -f "${call_log}" ]] && cat "${call_log}" || true)" \ + "no OP_SERVICE_ACCOUNT_TOKEN -> op never invoked for per-owner tokens" + +# All three refs resolve -> all three vars exported, each from its own ref. +# The stub echoes the ref it was asked for, so a var populated from the wrong +# ref is visible rather than merely non-empty. +stub_dir="$(make_stub_dir env bash cat id security timeout)" +cat >"${stub_dir}/op" <<'EOF' +#!/usr/bin/env bash +# args: read +case "$2" in + "op://Automation/CCCLI-SWM/token") echo "tok-swm" ;; + "op://Automation/CCCLI-NOS/token") echo "tok-nos" ;; + "op://Automation/GitHub - CCCLI/Token") echo "tok-twm" ;; + *) exit 1 ;; +esac +EOF +chmod +x "${stub_dir}/op" +result="$( + PATH="${stub_dir}" \ + OP_SERVICE_ACCOUNT_TOKEN="dummy" \ + bash -c "unset GH_TOKEN GH_TOKEN_SWM GH_TOKEN_NOS GH_TOKEN_TWM; source '${LIB_DIR}/logging.sh'; source '${LIB_DIR}/credentials.sh'; echo \"\${GH_TOKEN_SWM}/\${GH_TOKEN_NOS}/\${GH_TOKEN_TWM}\"" 2>/dev/null +)" +assert_equals "tok-swm/tok-nos/tok-twm" "${result}" \ + "all refs resolve -> each per-owner var loaded from its own ref" + +# FAIL-CLOSED PATH. Every ref fails -> each var holds the invalid sentinel, +# never an empty string. An empty value would let gh fall through to the +# keyring OAuth token (repo/workflow/admin:org), silently widening scope on a +# transient vault failure. This is the assertion the export flagged as +# unverified; it is safe because the sentinel is a fixed literal, not a token. +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 GH_TOKEN_SWM GH_TOKEN_NOS GH_TOKEN_TWM; source '${LIB_DIR}/logging.sh'; source '${LIB_DIR}/credentials.sh'; echo \"\${GH_TOKEN_SWM}/\${GH_TOKEN_NOS}/\${GH_TOKEN_TWM}\"" 2>/dev/null +)" +assert_equals "invalid-cccli-token-vault-fetch-failed/invalid-cccli-token-vault-fetch-failed/invalid-cccli-token-vault-fetch-failed" "${result}" \ + "all refs fail -> every per-owner var set to the invalid sentinel (fails closed)" + +# A partial failure must not poison the refs that did resolve: one bad ref +# fails closed on its own var only. Mixed outcomes are the realistic vault +# failure, and the dangerous version is one failure zeroing the others. +stub_dir="$(make_stub_dir env bash cat id security timeout)" +cat >"${stub_dir}/op" <<'EOF' +#!/usr/bin/env bash +case "$2" in + "op://Automation/CCCLI-NOS/token") exit 1 ;; + "op://Automation/CCCLI-SWM/token") echo "tok-swm" ;; + "op://Automation/GitHub - CCCLI/Token") echo "tok-twm" ;; + *) exit 1 ;; +esac +EOF +chmod +x "${stub_dir}/op" +result="$( + PATH="${stub_dir}" \ + OP_SERVICE_ACCOUNT_TOKEN="dummy" \ + bash -c "unset GH_TOKEN GH_TOKEN_SWM GH_TOKEN_NOS GH_TOKEN_TWM; source '${LIB_DIR}/logging.sh'; source '${LIB_DIR}/credentials.sh'; echo \"\${GH_TOKEN_SWM}/\${GH_TOKEN_NOS}/\${GH_TOKEN_TWM}\"" 2>/dev/null +)" +assert_equals "tok-swm/invalid-cccli-token-vault-fetch-failed/tok-twm" "${result}" \ + "one ref fails -> only that var gets the sentinel, others keep their tokens" + +# The fail-closed path must warn, so a degraded session is visible. A silent +# sentinel looks identical to a working token until a gh call fails oddly. +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" +warn_output="$( + PATH="${stub_dir}" \ + OP_SERVICE_ACCOUNT_TOKEN="dummy" \ + bash -c "unset GH_TOKEN GH_TOKEN_SWM GH_TOKEN_NOS GH_TOKEN_TWM; source '${LIB_DIR}/logging.sh'; source '${LIB_DIR}/credentials.sh'" 2>&1 1>/dev/null +)" +assert_contains "Failed to fetch GH_TOKEN_SWM from 1Password" "${warn_output}" \ + "per-owner fetch failure warns rather than failing silently" + +# An already-set var is respected and costs no vault read — matching +# _load_gh_token. The stub exits non-zero, so a lookup would overwrite the +# pre-set value with the sentinel and fail this assertion. +stub_dir="$(make_stub_dir env bash cat id security timeout)" +call_log="${stub_dir}/op-calls.log" +cat >"${stub_dir}/op" <>"${call_log}" +exit 1 +EOF +chmod +x "${stub_dir}/op" +result="$( + PATH="${stub_dir}" \ + OP_SERVICE_ACCOUNT_TOKEN="dummy" \ + GH_TOKEN_SWM="preset-swm" \ + bash -c "unset GH_TOKEN GH_TOKEN_NOS GH_TOKEN_TWM; source '${LIB_DIR}/logging.sh'; source '${LIB_DIR}/credentials.sh'; echo \"\${GH_TOKEN_SWM}\"" 2>/dev/null +)" +assert_equals "preset-swm" "${result}" \ + "already-set per-owner var is preserved, not overwritten" +assert_equals "" "$(grep -F "op://Automation/CCCLI-SWM/token" "${call_log}" 2>/dev/null || true)" \ + "already-set per-owner var -> its ref is never read from the vault" + # --- Summary --- echo "" From 3deb766985906c94289eb2edbaef9fbb2646a753 Mon Sep 17 00:00:00 2001 From: Claude Code Bot Date: Thu, 17 Sep 2026 19:35:40 -0700 Subject: [PATCH 2/3] test(credentials): isolate the call-count assertion from per-owner loading The "op read succeeds on first attempt -> called exactly once" case shares one call log with everything the sourced file does. Adding _load_owner_gh_tokens added three more reads to that log, so the assertion saw 4 where it expects 1. Preset the per-owner vars for this case so _load_owner_gh_tokens takes its already-set path and reads nothing. The assertion then counts only _load_gh_token's calls, which is what it was written to measure. Presetting rather than filtering the log keeps the count exact instead of merely plausible. This passed locally and failed in CI, which is the whole lesson: the developer environment already exports GH_TOKEN_SWM/NOS/TWM, so the already-set path was being taken by accident. The test was green for a reason unrelated to what it asserts. Verified the fix by reproducing CI's environment -- `env -u GH_TOKEN_SWM -u GH_TOKEN_NOS -u GH_TOKEN_TWM -u GH_TOKEN` -- where the unfixed version fails 31/32 and the fixed version passes 32/32. No change to lib/credentials.sh; the code under test was correct. Claude-Session: https://claude.ai/code/session_017s2qrmkQaV54fbcMzB2KRm --- tests/test-credentials.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test-credentials.sh b/tests/test-credentials.sh index 4a80a7d..7607e50 100755 --- a/tests/test-credentials.sh +++ b/tests/test-credentials.sh @@ -175,6 +175,13 @@ assert_equals "" "$([[ -f "${call_log}" ]] && cat "${call_log}" || true)" \ # OP_SERVICE_ACCOUNT_TOKEN available, op read succeeds on first try (with # timeout available) -> GH_TOKEN exported, single call. +# +# The per-owner vars are preset so _load_owner_gh_tokens takes its already-set +# path and reads nothing: this assertion counts _load_gh_token's calls, and a +# shared call log would otherwise attribute all four reads to it. Presetting +# rather than filtering keeps the count exact instead of merely plausible. +# They must be set explicitly here — inheriting them from the developer's live +# environment is what let this pass locally while failing in CI. stub_dir="$(make_stub_dir env bash cat id security timeout)" call_log="${stub_dir}/op-calls.log" cat >"${stub_dir}/op" </dev/null )" assert_equals "vault-gh-token" "${result}" \ From cd47b9c43a78c60205e054a23514b298ec6193f4 Mon Sep 17 00:00:00 2001 From: Claude Code Bot Date: Thu, 17 Sep 2026 19:40:05 -0700 Subject: [PATCH 3/3] test(wrapper): isolate the no-timeout call-count assertion the same way Same defect as the previous commit, in the other suite. test-wrapper.sh's "op read attempted exactly once (no retry loop without timeout)" shares one call log with everything sourcing credentials.sh does, so _load_owner_gh_tokens' three reads were attributed to _load_gh_token and the count read 4. Preset the per-owner vars for this case so the already-set path is taken and nothing is read. The assertion then measures what it was written to measure: that a missing `timeout` produces one attempt rather than a retry loop. I should have found this with the first fix instead of after a second red CI run. Both suites were checked for the pattern this time -- these are the only two call-count assertions in the repo. Verified in a CI-equivalent environment (`env -u GH_TOKEN_SWM -u GH_TOKEN_NOS -u GH_TOKEN_TWM -u GH_TOKEN`): without the preset this fails 1 with Actual: 4, matching CI exactly; with it all four suites pass -- test-credentials 32/32, test-launch-dir-check 6/6, test-remote-session 26/26, test-wrapper 69/69. shellcheck -S info clean. No change to lib/credentials.sh. Claude-Session: https://claude.ai/code/session_017s2qrmkQaV54fbcMzB2KRm --- tests/test-wrapper.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/test-wrapper.sh b/tests/test-wrapper.sh index e589ae6..1b92149 100755 --- a/tests/test-wrapper.sh +++ b/tests/test-wrapper.sh @@ -606,10 +606,17 @@ EOF [[ -n "${real_bin}" ]] && ln -s "${real_bin}" "${stub_dir}/${bin}" done + # The per-owner vars are preset so _load_owner_gh_tokens takes its + # already-set path and reads nothing. This assertion counts _load_gh_token's + # attempts, and the call log is shared by everything the sourced file does, + # so without this the three per-owner reads are attributed here too. They + # must be set explicitly rather than inherited: the developer environment + # exports them, which would make this pass locally and fail in CI. local debug_output debug_output="$( PATH="${stub_dir}" \ OP_SERVICE_ACCOUNT_TOKEN="dummy-token-for-test" \ + GH_TOKEN_SWM="preset" GH_TOKEN_NOS="preset" GH_TOKEN_TWM="preset" \ CLAUDE_DEBUG=true \ bash -c "unset GH_TOKEN; source '${LIB_DIR}/logging.sh'; source '${LIB_DIR}/credentials.sh'" 2>&1 1>/dev/null )"