diff --git a/bash/gh-wrapper.sh b/bash/gh-wrapper.sh index 7e4a8b2..273f27c 100755 --- a/bash/gh-wrapper.sh +++ b/bash/gh-wrapper.sh @@ -224,6 +224,47 @@ _gh_wrapper_sync_identity() { ;; esac + # GH_TOKEN outranks the keyring identity that `gh auth switch` selects, so + # the hosts.yml check below verifies this function's own output rather than + # the auth `gh` will actually use. When a token is present and represents a + # different identity than the resolved owner needs, fail closed instead of + # silently acting as the wrong account. + # + # CLAUDE_GH_TOKEN_LOGIN names the identity GH_TOKEN authenticates as. The + # test fixture sets it directly. In production it is unset, so the `gh api + # user` fallback below resolves it — one network call per invocation. See + # Step 11: session-level caching is a known follow-up, deliberately not + # built here. + if [[ -n "${GH_TOKEN:-}" ]]; then + local token_login="${CLAUDE_GH_TOKEN_LOGIN:-}" + + if [[ -z "${token_login}" ]]; then + # NOT `command gh`: ~/.local/bin/gh is this same wrapper and precedes + # the real binary in PATH, so `command` re-enters this function. + # _gh_wrapper_find_real_gh scans PATH while skipping this file. + local real_gh + if real_gh="$(_gh_wrapper_find_real_gh)"; then + token_login="$(GH_TOKEN="${GH_TOKEN}" "${real_gh}" api user --jq .login 2>/dev/null)" + fi + fi + + if [[ -z "${token_login}" ]]; then + echo "[gh] ERROR: GH_TOKEN is set but its identity could not be resolved" >&2 + echo "[gh] Refusing to run: GH_TOKEN overrides 'gh auth switch', so the" >&2 + echo "[gh] identity check cannot be trusted. Unset GH_TOKEN to use the" >&2 + echo "[gh] keyring identity." >&2 + return 1 + fi + + if [[ "${token_login,,}" != "${desired,,}" ]]; then + echo "[gh] ERROR: GH_TOKEN authenticates as '${token_login}' but repo owner '${owner}' requires '${desired}'" >&2 + echo "[gh] GH_TOKEN takes precedence over 'gh auth switch', so this would" >&2 + echo "[gh] run as the wrong identity. Failing closed." >&2 + echo "[gh] Fix: unset GH_TOKEN to use the keyring identity for this repo." >&2 + return 1 + fi + fi + current=$(awk '/^github\.com:/{f=1} f && /^ *user:/{print $2; exit}' "${HOME}/.config/gh/hosts.yml" 2>/dev/null | tr -d "\"'") if [[ -n "${current}" && "${current}" != "${desired}" ]]; then diff --git a/bash/tests/test-gh-wrapper-gh-token-precedence.sh b/bash/tests/test-gh-wrapper-gh-token-precedence.sh new file mode 100755 index 0000000..8819fb1 --- /dev/null +++ b/bash/tests/test-gh-wrapper-gh-token-precedence.sh @@ -0,0 +1,86 @@ +#!/usr/bin/env bash +# Regression test: _gh_wrapper_sync_identity must not report success when +# GH_TOKEN will override the identity it just switched to. +# +# KNOWN-BAD CASE: case 2 below passes against the CURRENT code, because the +# fail-closed check reads hosts.yml (its own output) rather than the auth gh +# will actually use. That inverted assertion is the bug this test pins. +set -uo pipefail +unset CDPATH + +TESTS_DIR="$(CDPATH='' cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +WRAPPER="${TESTS_DIR}/../gh-wrapper.sh" + +WORKDIR="/tmp/gh-token-precedence-test-$$" +mkdir -p "${WORKDIR}" +trap 'rm -rf "${WORKDIR}"' EXIT + +GIT=/usr/bin/git +fail=0 +_pass() { echo " PASS: $1"; } +_fail() { + echo " FAIL: $1" >&2 + fail=1 +} + +# Sandbox HOME so the fixture never reads or writes the real hosts.yml. +export HOME="${WORKDIR}/home" +mkdir -p "${HOME}/.config/gh" +cat >"${HOME}/.config/gh/hosts.yml" <<'YAML' +github.com: + user: smartwatermelon + oauth_token: fake +YAML + +"${GIT}" init -q "${WORKDIR}/repo" +"${GIT}" -C "${WORKDIR}/repo" remote add origin \ + git@github.com:smartwatermelon/example.git + +# The driver each case runs: source the wrapper from inside the fixture repo and +# call the function under test. Kept in a file rather than `bash -c` so the +# wrapper path needs no nested quoting. +RUNNER="${WORKDIR}/run-sync.sh" +cat >"${RUNNER}" < sync succeeds, as it does today. + +if _sync_under_env; then + _pass "no GH_TOKEN: sync succeeds" +else + _fail "no GH_TOKEN: sync should succeed" +fi + +# Case 2: GH_TOKEN belonging to a DIFFERENT identity than the resolved owner +# maps to. Must fail closed. Fails against the current code, which passes. +if _sync_under_env GH_TOKEN="fake-token-for-andrewmrich" \ + CLAUDE_GH_TOKEN_LOGIN="andrewmrich"; then + _fail "mismatched GH_TOKEN: silently ran as the wrong identity" +else + _pass "mismatched GH_TOKEN: fails closed" +fi + +# Case 3: GH_TOKEN matching the resolved identity -> proceeds. +if _sync_under_env GH_TOKEN="fake-token-for-smartwatermelon" \ + CLAUDE_GH_TOKEN_LOGIN="smartwatermelon"; then + _pass "matching GH_TOKEN: proceeds" +else + _fail "matching GH_TOKEN: should proceed" +fi + +if [[ ${fail} -eq 0 ]]; then + echo "test-gh-wrapper-gh-token-precedence.sh: all assertions passed" + exit 0 +fi +exit 1 diff --git a/bash/tests/test-gh-wrapper-identity.sh b/bash/tests/test-gh-wrapper-identity.sh index e548403..9d1a7c1 100755 --- a/bash/tests/test-gh-wrapper-identity.sh +++ b/bash/tests/test-gh-wrapper-identity.sh @@ -28,6 +28,13 @@ isolate_git_env export HOME="/tmp/gh-wrapper-identity-test-home-$$" mkdir -p "${HOME}/.config/gh" +# GH_TOKEN outranks the keyring identity, so _gh_wrapper_sync_identity fails +# closed when a real token in the developer's environment disagrees with the +# fixture owner each case resolves. That guard is correct; inheriting the +# ambient token here is not. Sandbox it the same way HOME is sandboxed, so the +# cases exercise the hosts.yml path they are written to test. +unset GH_TOKEN CLAUDE_GH_TOKEN_LOGIN + # git init inside the sandboxed HOME must not pick up interactive prompts. export GIT_CONFIG_GLOBAL="${HOME}/.gitconfig" export GIT_CONFIG_SYSTEM=/dev/null