fix(credentials): fail closed when GH_TOKEN vault fetch fails - #122
Open
twistedmelonman wants to merge 1 commit into
Open
twistedmelonman wants to merge 1 commit into
twistedmelonman wants to merge 1 commit into
Conversation
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
|
The change is a targeted security improvement: when the vault fetch for GH_TOKEN fails, the code now exports a deliberately invalid sentinel instead of leaving the variable unset. This prevents gh from silently falling back to the broader keyring OAuth token on transient failures. The sentinel skip-logic in _load_gh_token() correctly allows a genuine retry while still skipping on a real pre-set token. Three new tests cover the three cases (failure to sentinel, failure to warn, sentinel to retry still reaches vault). No reliability regressions or security issues introduced. VERDICT: PASS |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When the 1Password fetch failed,
_load_gh_tokenleftGH_TOKENunset andlogged a warning saying gh would "use keyring fallback." That fallback is the
user's keyring OAuth token, which carries
repo,workflow,admin:org,delete_repoandadmin:public_key— far wider than the restricted CCCLI PATthe function exists to inject.
So the degraded path was strictly more privileged than the healthy one. Any
transient
opfailure — locked vault, missing service account token, networkblip, backoff exhausted — silently handed the agent full admin access,
announced only as a launch-time warning nobody reads.
Found while diagnosing #120.
Fix
Export a deliberately invalid sentinel instead of leaving
GH_TOKENunset, sogh 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.
Verification
Checked against the pre-change code with a stubbed failing
op:GH_TOKENafter failed fetchThe failure path had no test coverage. Adds three: the sentinel is
exported, the warning is emitted, and a present sentinel does not suppress a
later vault lookup.
test-credentials.sh14/14test-launch-dir-check.sh6/6test-remote-session.sh26/26test-wrapper.shpassshellcheck -S infocleantest-gh-token-permissions.shwas deliberately not run: it mutates a livesandbox repo and needs org write access the current PAT lacks (that being the
subject of #120).
Also
Corrects the file header, which claimed these credentials are "not present in
the interactive shell environment."
OP_SERVICE_ACCOUNT_TOKENis in factinherited by child processes of the wrapper, including the agent's own shell —
verified this session.
Note for reviewers
test-remote-session.shhas a pre-existing cwd dependency: it fails 25/26 whenrun from outside the repo (it derives a session name from
$PWD) and passes26/26 from inside. Confirmed pre-existing by stashing this branch's changes and
re-running. Not addressed here.
Refs #120
https://claude.ai/code/session_015WZskZun8am3TWVS5pMYii