diff --git a/.claude/hooks/session-start.sh b/.claude/hooks/session-start.sh index d516288..28cb205 100755 --- a/.claude/hooks/session-start.sh +++ b/.claude/hooks/session-start.sh @@ -35,10 +35,16 @@ if [ -n "${OP_SERVICE_ACCOUNT_TOKEN:-}" ]; then OP_VAULT="${CLAUDE_OP_VAULT:-Claude}" OP_ITEM="${CLAUDE_OP_ITEM:-cloud-session-env}" - # Seed the Nullify CLI credentials file so its built-in refresh flow - # mints fresh access tokens (an env NULLIFY_TOKEN would override stored - # credentials in the CLI, so that var is deliberately NOT resolved here). - if [ ! -s "$HOME/.nullify/credentials.json" ]; then + # Nullify accepts either a long-lived NULLIFY_TOKEN or a seeded + # credentials file. Prefer the token: it needs no refresh cycle and no + # browser, so it survives an ephemeral container, whereas a seeded refresh + # token ages out and its renewals die with the container. Seed the file + # only when no token is configured, since the CLI would ignore it anyway. + nullify_token="$(op read "op://${OP_VAULT}/${OP_ITEM}/NULLIFY_TOKEN" 2>/dev/null || true)" + if [ -n "$nullify_token" ]; then + printf 'export %s=%q\n' "NULLIFY_TOKEN" "$nullify_token" >> "$CLAUDE_ENV_FILE" + unset nullify_token + elif [ ! -s "$HOME/.nullify/credentials.json" ]; then nullify_creds="$(op read "op://${OP_VAULT}/${OP_ITEM}/NULLIFY_CREDENTIALS_JSON" 2>/dev/null || true)" if [ -n "$nullify_creds" ]; then mkdir -p "$HOME/.nullify" @@ -96,4 +102,21 @@ if ! golangci-lint version 2>/dev/null | grep -q "version ${GOLANGCI_LINT_VERSIO go install "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" fi +# ============================================================ +# 3. MCP credential preflight — report-only +# ============================================================ + +# Expired credentials make a server expose no tools, or list every tool and +# fail each call. Neither is visible without looking, so surface it here +# instead of leaving it to be noticed when a report comes back empty. +# Never fails the hook: the session is still usable with a degraded server. +echo "--- MCP preflight ---" +if ! .claude/mcp/preflight.sh; then + echo + echo "!! One or more MCP servers are UNUSABLE - see the table above." + echo "!! Their tools may still be listed and will return empty results." + echo "!! Do not read an empty result from those servers as 'nothing found'." + echo "!! Machine-readable status: .claude/mcp/health.json" +fi + echo "Session start hook completed: secrets resolved, MCP servers installed, Go toolchain ready." diff --git a/.claude/mcp/health-probes.json b/.claude/mcp/health-probes.json new file mode 100644 index 0000000..f10cad0 --- /dev/null +++ b/.claude/mcp/health-probes.json @@ -0,0 +1,51 @@ +{ + "_comment": [ + "Authorization probes for the MCP servers declared in .mcp.json.", + "", + "A handshake only proves a server started. Several of these servers will", + "start, advertise their whole tool set, and then answer every call with an", + "empty result or a 403 - which reads as 'healthy but quiet' rather than as", + "an outage. Each server therefore needs a probe that proves the credential", + "is actually accepted.", + "", + "preflight.sh reads the server list from .mcp.json, not from this file. A", + "server present there but missing here is reported UNCHECKED rather than", + "skipped, so adding a server cannot quietly add a blind spot.", + "", + "kinds:", + " http GET url_env + path with the key in an Authorization header;", + " ok_status means authorized. Pick an endpoint that returns", + " data, and verify it answers differently without the key.", + " jwt-env Inspect a JWT in var: reject expired, and reject any", + " token_use in reject_token_use. Opaque tokens pass through.", + " env-present Assert vars are non-empty. Weakest kind - only for servers", + " with no cheap authenticated endpoint." + ], + + "probes": { + "nullify": { + "kind": "jwt-env", + "var": "NULLIFY_TOKEN", + "reject_token_use": ["id"], + "fallback_file": "~/.nullify/credentials.json", + "hint": "set NULLIFY_TOKEN to an API token that does not expire, or seed NULLIFY_CREDENTIALS_JSON" + }, + + "latent-defense": { + "kind": "http", + "url_env": "LATENT_DEFENSE_URL", + "path": "/api/infra/records", + "key_env": "LATENT_DEFENSE_API_KEY", + "ok_status": "200", + "hint": "rotate LATENT_DEFENSE_API_KEY in the secret store", + "_note": "Deliberately NOT /auth/me: that endpoint serves device-flow sessions and answers 401 to a valid API key, which once produced a six-day false outage." + }, + + "sentinelone": { + "kind": "env-present", + "vars": ["PURPLEMCP_CONSOLE_TOKEN", "PURPLEMCP_CONSOLE_BASE_URL"], + "hint": "set PURPLEMCP_CONSOLE_TOKEN and PURPLEMCP_CONSOLE_BASE_URL", + "_note": "The console exposes no unauthenticated health endpoint, so presence is the strongest cheap check available." + } + } +} diff --git a/.claude/mcp/nullify-serve.sh b/.claude/mcp/nullify-serve.sh index b563778..3646afc 100755 --- a/.claude/mcp/nullify-serve.sh +++ b/.claude/mcp/nullify-serve.sh @@ -1,22 +1,57 @@ #!/bin/bash -# Start the Nullify MCP server with auto-refreshing credentials. +# Start the Nullify MCP server, accepting either credential model. # -# NULLIFY_CREDENTIALS_JSON (resolved from 1Password by with-op-env.sh) holds -# the contents of ~/.nullify/credentials.json from a machine where -# 'nullify auth login' was run. Seeding that file lets the CLI's built-in -# refresh flow mint fresh access tokens for the refresh token's lifetime -# (~30 days), instead of relying on a static hourly token. +# NULLIFY_TOKEN (preferred for cloud sessions) +# A long-lived API token. The CLI uses it directly, so there is no refresh +# cycle to go stale and nothing to mint through a browser. This is the right +# model for an ephemeral container: the token in the secret store is the only +# state, and it stays valid until it is deliberately rotated. # -# NOTE: an env NULLIFY_TOKEN overrides stored credentials in the CLI - do not -# set it anywhere (environment settings or 1Password) when using this flow. +# NULLIFY_CREDENTIALS_JSON (fallback) +# The contents of ~/.nullify/credentials.json from a machine where +# 'nullify auth login' was run. Seeding it lets the CLI's refresh flow mint +# access tokens for the refresh token's lifetime (~30 days). The catch is +# that refreshes are written to the container's filesystem and lost when it +# exits, so the stored copy ages out and eventually stops working - at which +# point a human has to re-run an interactive browser login. +# +# NULLIFY_TOKEN wins when both are present: the CLI would prefer it anyway +# (it outranks stored credentials), so seeding a file we would not read only +# creates a second thing to keep in sync. set -uo pipefail -if [ -n "${NULLIFY_CREDENTIALS_JSON:-}" ] && [ ! -s "$HOME/.nullify/credentials.json" ]; then +if [ -n "${NULLIFY_TOKEN:-}" ]; then + echo "nullify-serve: authenticating with NULLIFY_TOKEN" >&2 +else + if [ -n "${NULLIFY_CREDENTIALS_JSON:-}" ] && [ ! -s "$HOME/.nullify/credentials.json" ]; then + mkdir -p "$HOME/.nullify" + chmod 700 "$HOME/.nullify" + printf '%s' "$NULLIFY_CREDENTIALS_JSON" > "$HOME/.nullify/credentials.json" + chmod 600 "$HOME/.nullify/credentials.json" + fi + + if [ -s "$HOME/.nullify/credentials.json" ]; then + echo "nullify-serve: authenticating with seeded credentials file" >&2 + else + { + echo "nullify-serve: WARNING no Nullify credentials available." + echo "nullify-serve: set NULLIFY_TOKEN (preferred) or NULLIFY_CREDENTIALS_JSON" + echo "nullify-serve: in the 1Password item named by CLAUDE_OP_VAULT/CLAUDE_OP_ITEM." + echo "nullify-serve: the server will start but expose no tools." + } >&2 + fi +fi +unset NULLIFY_CREDENTIALS_JSON + +# Seed config.json too. 'mcp serve' reads NULLIFY_HOST directly, but leaving +# config.json absent means anyone debugging inside the container gets +# "Not configured" from 'nullify auth status' even though the credentials are +# right there - which is exactly the wrong signal when auth is what's broken. +if [ -n "${NULLIFY_HOST:-}" ] && [ ! -s "$HOME/.nullify/config.json" ]; then mkdir -p "$HOME/.nullify" chmod 700 "$HOME/.nullify" - printf '%s' "$NULLIFY_CREDENTIALS_JSON" > "$HOME/.nullify/credentials.json" - chmod 600 "$HOME/.nullify/credentials.json" + printf '{\n "host": "%s"\n}\n' "$NULLIFY_HOST" > "$HOME/.nullify/config.json" + chmod 600 "$HOME/.nullify/config.json" fi -unset NULLIFY_CREDENTIALS_JSON exec go run ./cmd/cli mcp serve diff --git a/.claude/mcp/preflight.sh b/.claude/mcp/preflight.sh new file mode 100755 index 0000000..bc2ceb4 --- /dev/null +++ b/.claude/mcp/preflight.sh @@ -0,0 +1,222 @@ +#!/bin/bash +# Verify every MCP server in .mcp.json is not just running but usable, and +# leave a machine-readable record of the answer. +# +# .claude/mcp/preflight.sh human table + health.json +# make preflight same +# +# Why this exists: an MCP server almost never fails loudly. It either dies +# during startup and shows up in the client as a server with no tools, or it +# starts, advertises its whole tool set, and answers every call with an empty +# result because its credential is rejected. Both look like "nothing to report" +# rather than "outage", and the second is worse - work gets published against +# data that was never there. +# +# So each server gets two checks: +# 1. handshake does it start and list tools? +# 2. authorization is its credential actually accepted? +# +# The server list comes from .mcp.json, never from a hand-kept list here. A +# server with no probe in health-probes.json is reported UNCHECKED, so adding +# one cannot quietly add a blind spot. +# +# Exit status: 0 only when every server passes, so this can gate a check. +set -uo pipefail + +cd "$(dirname "${BASH_SOURCE[0]}")/../.." || exit 1 + +MCP_CONFIG="${PREFLIGHT_MCP_CONFIG:-.mcp.json}" +PROBE_CONFIG="${PREFLIGHT_PROBE_CONFIG:-.claude/mcp/health-probes.json}" +HEALTH_OUT="${PREFLIGHT_OUT:-.claude/mcp/health.json}" +TIMEOUT="${PREFLIGHT_TIMEOUT:-120}" + +STDERR_FILE="$(mktemp)" +trap 'rm -f "$STDERR_FILE"' EXIT + +for dep in jq curl; do + command -v "$dep" >/dev/null 2>&1 || { echo "preflight: $dep is required" >&2; exit 2; } +done +[ -f "$MCP_CONFIG" ] || { echo "preflight: $MCP_CONFIG not found" >&2; exit 2; } + +failures=0 +declare -a ROWS=() # status|server|detail +declare -a JSON_ROWS=() + +record() { + ROWS+=("$1|$2|$3") + JSON_ROWS+=("$(jq -nc --arg s "$1" --arg n "$2" --arg d "$3" --argjson t "${4:-null}" \ + '{server:$n, status:$s, detail:$d, tools:$t}')") + [ "$1" = "FAIL" ] && failures=$((failures + 1)) + return 0 +} + +# ---------------------------------------------------------------- handshake + +# Expand the ${VAR:-default} form that MCP client env blocks use. +expand_env_value() { + local raw="$1" + if [[ "$raw" =~ ^\$\{([A-Za-z_][A-Za-z0-9_]*):-(.*)\}$ ]]; then + local name="${BASH_REMATCH[1]}" fallback="${BASH_REMATCH[2]}" + printf '%s' "${!name:-$fallback}" + elif [[ "$raw" =~ ^\$\{?([A-Za-z_][A-Za-z0-9_]*)\}?$ ]]; then + printf '%s' "${!BASH_REMATCH[1]:-}" + else + printf '%s' "$raw" + fi +} + +# Start the server exactly as the client would - through the same wrapper and +# arguments - so the wiring itself is under test, not just the binary. +handshake_tool_count() { + local server="$1" + local -a cmd=() envs=() + local key val + + mapfile -t cmd < <(jq -r --arg s "$server" \ + '.mcpServers[$s] | ([.command] + (.args // []))[]' "$MCP_CONFIG" 2>/dev/null) + [ "${#cmd[@]}" -gt 0 ] || { echo ""; return; } + + while IFS=$'\t' read -r key val; do + [ -n "$key" ] && envs+=("$key=$(expand_env_value "$val")") + done < <(jq -r --arg s "$server" \ + '(.mcpServers[$s].env // {}) | to_entries[] | "\(.key)\t\(.value)"' "$MCP_CONFIG" 2>/dev/null) + + { + echo '{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"preflight","version":"1"}}}' + echo '{"jsonrpc":"2.0","method":"notifications/initialized"}' + echo '{"jsonrpc":"2.0","id":2,"method":"tools/list","params":{}}' + sleep 2 + } | timeout "$TIMEOUT" env "${envs[@]}" "${cmd[@]}" 2>"$STDERR_FILE" \ + | jq -R 'fromjson? // empty' 2>/dev/null \ + | jq -s 'map(select(.id == 2)) | (.[0].result.tools // []) | length' 2>/dev/null +} + +# Last stderr line that looks like a cause rather than a banner. +stderr_reason() { + grep -v '^[[:space:]]*$' "$STDERR_FILE" 2>/dev/null \ + | grep -viE 'warning:|IncompleteFieldDefinition|warnings\.warn|INFO |Processing request|Starting MCP server|^ +' \ + | tail -1 | cut -c1-150 +} + +# ------------------------------------------------------------------- probes + +probe_field() { + jq -r --arg s "$1" --arg f "$2" '.probes[$s][$f] // empty' "$PROBE_CONFIG" 2>/dev/null +} + +# echoes "|"; verdict is ok, bad or unknown +run_probe() { + local server="$1" kind + kind="$(probe_field "$server" kind)" + + case "$kind" in + http) + local url_env key_env path ok base key code + url_env="$(probe_field "$server" url_env)"; key_env="$(probe_field "$server" key_env)" + path="$(probe_field "$server" path)"; ok="$(probe_field "$server" ok_status)" + base="${!url_env:-}"; key="${!key_env:-}" + [ -n "$base" ] || { echo "unknown|$url_env is unset"; return; } + [ -n "$key" ] || { echo "bad|$key_env is unset"; return; } + code="$(curl -sS -o /dev/null -w '%{http_code}' -m 25 \ + -H "Authorization: Bearer ${key}" -H "Accept: application/json" \ + "${base%/}${path}" 2>/dev/null)" + if [ "$code" = "$ok" ]; then echo "ok|authorized" + elif [ -z "$code" ] || [ "$code" = "000" ]; then echo "unknown|${base} unreachable" + else echo "bad|$key_env rejected (HTTP $code)"; fi + ;; + + jwt-env) + # A JWT states its own validity, so read it instead of waiting for the + # API to answer 403 on every call. + local var tok claims exp use reject file + var="$(probe_field "$server" var)"; tok="${!var:-}" + if [ -z "$tok" ]; then + file="$(probe_field "$server" fallback_file)"; file="${file/#\~/$HOME}" + if [ -n "$file" ] && [ -s "$file" ]; then echo "unknown|$var unset; using $(basename "$file")" + else echo "bad|$var unset and no fallback credentials"; fi + return + fi + claims="$(printf '%s' "$tok" | jq -R 'split(".") | if length == 3 then .[1] | @base64d | fromjson else empty end' 2>/dev/null)" + if [ -z "$claims" ]; then echo "ok|opaque token present"; return; fi + exp="$(printf '%s' "$claims" | jq -r '.exp // empty')" + use="$(printf '%s' "$claims" | jq -r '.token_use // empty')" + if [ -n "$exp" ] && [ "$exp" -lt "$(date +%s)" ] 2>/dev/null; then + echo "bad|$var expired $(date -u -d "@$exp" '+%Y-%m-%d %H:%M UTC' 2>/dev/null)"; return + fi + reject="$(jq -r --arg s "$server" '(.probes[$s].reject_token_use // [])[]' "$PROBE_CONFIG" 2>/dev/null | tr '\n' ' ')" + if [ -n "$use" ] && [[ " $reject " == *" $use "* ]]; then + echo "bad|$var is a ${use} token, which the API rejects"; return + fi + echo "ok|token valid" + ;; + + env-present) + local missing=() v + while read -r v; do + [ -n "$v" ] && [ -z "${!v:-}" ] && missing+=("$v") + done < <(jq -r --arg s "$server" '(.probes[$s].vars // [])[]' "$PROBE_CONFIG" 2>/dev/null) + if [ "${#missing[@]}" -gt 0 ]; then echo "bad|unset: ${missing[*]}" + else echo "ok|credentials present"; fi + ;; + + *) echo "unknown|no probe defined" ;; + esac +} + +# -------------------------------------------------------------------- main + +echo "MCP preflight - $(jq -r '.mcpServers | keys | length' "$MCP_CONFIG") server(s) in $MCP_CONFIG" +echo + +while read -r server; do + [ -n "$server" ] || continue + + count="$(handshake_tool_count "$server")" + count="${count:-0}" + hint="$(probe_field "$server" hint)" + + if [ "$count" -eq 0 ]; then + reason="$(stderr_reason)" + # A dead server usually explains itself on stderr; if the probe knows why, + # say that instead of the raw message. + verdict="$(run_probe "$server")" + if [ "${verdict%%|*}" = "bad" ]; then + record "FAIL" "$server" "no tools - ${verdict#*|}${hint:+; $hint}" + elif [ -n "$reason" ]; then + record "FAIL" "$server" "no tools - $reason" + else + record "FAIL" "$server" "no tools and no error reported" + fi + continue + fi + + verdict="$(run_probe "$server")" + case "${verdict%%|*}" in + ok) record "PASS" "$server" "${verdict#*|}" "$count" ;; + bad) record "FAIL" "$server" "${count} tools but ${verdict#*|}${hint:+; $hint} - calls return empty results, not errors" "$count" ;; + unknown) record "WARN" "$server" "${count} tools; ${verdict#*|}" "$count" ;; + esac +done < <(jq -r '.mcpServers | keys[]' "$MCP_CONFIG") + +printf '%-8s %-16s %s\n' "RESULT" "SERVER" "DETAIL" +printf '%-8s %-16s %s\n' "------" "------" "------" +for row in "${ROWS[@]}"; do + printf '%-8s %-16s %s\n' "${row%%|*}" "$(cut -d'|' -f2 <<<"$row")" "${row##*|}" +done +echo + +mkdir -p "$(dirname "$HEALTH_OUT")" +printf '%s\n' "${JSON_ROWS[@]}" | jq -s \ + --arg ts "$(date -u '+%Y-%m-%dT%H:%M:%SZ')" \ + --argjson failures "$failures" \ + '{checked_at:$ts, healthy:($failures == 0), failures:$failures, servers:.}' \ + > "$HEALTH_OUT" 2>/dev/null && echo "Wrote $HEALTH_OUT" + +if [ "$failures" -gt 0 ]; then + echo + echo "$failures server(s) unusable. Treat their data as ABSENT, not as empty:" + echo "a rejected credential returns no findings, which looks identical to good news." + exit 1 +fi + +echo "All MCP servers reachable and authorized." diff --git a/.claude/mcp/with-op-env.sh b/.claude/mcp/with-op-env.sh index 9105fa4..d1181b2 100755 --- a/.claude/mcp/with-op-env.sh +++ b/.claude/mcp/with-op-env.sh @@ -10,9 +10,18 @@ # with CLAUDE_OP_VAULT / CLAUDE_OP_ITEM (item ID works too). set -uo pipefail +# A var may be marked optional by prefixing it with "?". Optional vars are +# still resolved from 1Password, but their absence is not reported: they exist +# for servers that accept more than one credential model, where exactly one of +# several vars is expected to be present and the server itself decides which +# it got. Everything without the prefix is required, as before. vars=() +required=() while [ $# -gt 0 ] && [ "$1" != "--" ]; do - vars+=("$1") + case "$1" in + '?'*) vars+=("${1#\?}") ;; + *) vars+=("$1"); required+=("$1") ;; + esac shift done shift || true @@ -53,10 +62,37 @@ if [ "$missing" -eq 1 ] && [ -n "${OP_SERVICE_ACCOUNT_TOKEN:-}" ]; then fi fi +# Report anything still unresolved before handing over. A server started +# without its credentials does not fail in a way anyone notices: it connects +# and then exposes no tools, or answers every call "not authenticated". Naming +# the gap here is the difference between a five-minute fix and an outage that +# survives until someone audits tool coverage by hand. +still_missing=() +for var in "${required[@]}"; do + [ -z "${!var:-}" ] && still_missing+=("$var") +done + +if [ "${#still_missing[@]}" -gt 0 ]; then + { + echo "with-op-env: WARNING starting '${1:-}' with unresolved secrets: ${still_missing[*]}" + if [ -z "${OP_SERVICE_ACCOUNT_TOKEN:-}" ]; then + echo "with-op-env: cause: OP_SERVICE_ACCOUNT_TOKEN is not set, so 1Password was never consulted." + else + echo "with-op-env: looked up: op://${CLAUDE_OP_VAULT:-Claude}/${CLAUDE_OP_ITEM:-cloud-session-env}/" + echo "with-op-env: cause: those fields are missing from the item, or the service account cannot read them." + fi + echo "with-op-env: expect this server to start but expose no working tools." + } >&2 +fi + # The target command may also be installed by the async hook; wait briefly for _ in $(seq 1 45); do command -v "$1" >/dev/null 2>&1 && break sleep 2 done +if ! command -v "$1" >/dev/null 2>&1; then + echo "with-op-env: WARNING '$1' is not on PATH after waiting 90s; exec will fail." >&2 +fi + exec "$@" diff --git a/.gitignore b/.gitignore index ba077a4..748d131 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ bin +.claude/mcp/health.json diff --git a/.mcp.json b/.mcp.json index 8504a16..af23e23 100644 --- a/.mcp.json +++ b/.mcp.json @@ -4,7 +4,7 @@ "command": "bash", "args": [ ".claude/mcp/with-op-env.sh", - "NULLIFY_CREDENTIALS_JSON", + "?NULLIFY_TOKEN", "?NULLIFY_CREDENTIALS_JSON", "--", "bash", ".claude/mcp/nullify-serve.sh" ], diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..5910136 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,94 @@ +# Working in this repository + +The Nullify CLI, plus the MCP wiring that gives a Claude Code session access to +this organization's security tools (`.mcp.json`, `.claude/mcp/`). + +## Before reporting on security data, check coverage + +**Read `.claude/mcp/health.json` first.** It is written by +`.claude/mcp/preflight.sh` on every session start, and again whenever you run +`make preflight`. + +```jsonc +{ "healthy": false, "failures": 1, + "servers": [ { "server": "nullify", "status": "FAIL", "detail": "...", "tools": 27 } ] } +``` + +A `FAIL` server does not error when you call it. It returns **empty results**, +which is indistinguishable from good news: + +- an expired or wrong-typed credential still lets the server start and + advertise its whole tool set, then fails every call; +- a rejected key often yields `[]` rather than a 4xx. + +So an empty finding list from a `FAIL` server means **absent data**, never +"nothing found". Say so explicitly in any report rather than implying the +estate was covered. This has already gone wrong once: a weekly review was +published claiming coverage while two sources were dead, and a second review +declared a healthy platform dead for six days because the check probed the +wrong endpoint. + +If `health.json` is missing or stale, run `make preflight` before relying on +any security tool. + +## Adding an MCP server + +1. Add it to `.mcp.json`. +2. Add an authorization probe to `.claude/mcp/health-probes.json`. + +Step 2 is not optional. Preflight takes its server list from `.mcp.json`, so a +server without a probe is reported `WARN … no probe defined` rather than +skipped — but a `WARN` is an unverified server, which is the state this whole +mechanism exists to prevent. + +Choose a probe endpoint that **returns data** and verify it answers +differently without the credential. Do not probe an identity endpoint without +checking it accepts the credential type you use: `latent-defense`'s `/auth/me` +serves login sessions and returns 401 to a perfectly valid API key, which is +exactly how the six-day false outage happened. + +## Credentials + +Secrets resolve from 1Password via `.claude/mcp/with-op-env.sh`, from the item +named by `CLAUDE_OP_VAULT` / `CLAUDE_OP_ITEM`. Mark a var optional with a `?` +prefix in `.mcp.json` when a server accepts more than one credential model, so +the unused alternative does not warn. + +For Nullify specifically, prefer a long-lived `NULLIFY_TOKEN` over a seeded +`NULLIFY_CREDENTIALS_JSON`: a container is rebuilt every session, so refreshed +tokens are written to a filesystem that is then discarded, and the stored copy +only ages. Note that the `access_token` inside a `credentials.json` is a Cognito +**ID token** — it authenticates the process but the API rejects it, surfacing as +403 on every call. Preflight checks for both mistakes. + +## Verifying a change + +```sh +make build && make unit && make lint # Go +bash -n .claude/mcp/*.sh # shell +make preflight # MCP servers still usable +``` + +## Upstream + +This repo tracks `Nullify-Platform/cli`. When touching auth or the refreshing +transport, check upstream first — its `internal/auth/login.go` documents the +real contract of `GET /auth/refresh_token` (no token in the JSON body; the +access token arrives only as a `Set-Cookie`, and the refresh token is **not** +rotated). This fork branched at upstream `#138`, so it is behind in the auth/client +area. #165's behaviour has been ported by hand (401-triggered forced refresh, +`ForceRefreshToken`, `RefreshNullifyToken`, `ErrTokenNotRefreshable`, and +separate backoff clocks for the TTL and 401 paths), but these upstream changes +are still missing: + +- **#159** — the GitHub PAT is still sent in the URL query string by + `internal/lib/get_token.go`, where it can land in access logs and proxies. + Upstream moved it into a POST body. +- **#151** — an idempotency-aware retry: 5xx is only retried for idempotent + methods, so a POST that may already have committed is never replayed. Ours + replays any 5xx. +- **#152 / #158** — MCP tools rebuilt on the generated API client, and the RunE + migration. Large behavioural changes; deliberately not adopted. + +Do not cherry-pick from upstream blind: the histories are unrelated, so a pick +of any one commit pulls in dependencies this fork does not have. diff --git a/Makefile b/Makefile index 5843884..ab9e0d0 100644 --- a/Makefile +++ b/Makefile @@ -45,3 +45,7 @@ cov: -gocover-cobertura < coverage.txt > coverage.xml -go tool cover -html=coverage.txt -o coverage.html -go tool cover -func=coverage.txt + +preflight: ## Verify every MCP server in .mcp.json starts AND is authorized + @.claude/mcp/preflight.sh +.PHONY: preflight diff --git a/README.md b/README.md index 552f868..8d218e8 100644 --- a/README.md +++ b/README.md @@ -287,6 +287,28 @@ Scope findings to a specific repository with `--repo`: } ``` +### Ephemeral and cloud environments + +In a container that is rebuilt for every session — Claude Code on the web, CI, +a devcontainer — authenticate with a long-lived token rather than a copied +credentials file: + +```sh +export NULLIFY_HOST=acme.nullify.ai +export NULLIFY_TOKEN=your-token-here +``` + +`NULLIFY_TOKEN` outranks stored credentials, needs no browser, and has no +refresh cycle to go stale, so the token in your secret store is the only state +to maintain. + +Copying a `~/.nullify/credentials.json` from a workstation also works, but it +degrades over time: refreshed tokens are written to the container's filesystem +and lost when it exits, so the stored copy never advances and eventually stops +refreshing. When that happens the server exits at startup and appears in your +client as a server with no tools. `nullify auth status` will report the expiry, +and `nullify auth token` the underlying error. + ## Configuration The CLI stores configuration at `~/.nullify/config.json`. Host resolution priority: diff --git a/cmd/cli/cmd/auth.go b/cmd/cli/cmd/auth.go index a6fa5a8..9b3037e 100644 --- a/cmd/cli/cmd/auth.go +++ b/cmd/cli/cmd/auth.go @@ -33,15 +33,9 @@ var loginCmd = &cobra.Command{ ctx, stop := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) defer stop() - loginHost := host - - // If no host from flag, try config file - if loginHost == "" { - cfg, err := auth.LoadConfig() - if err == nil && cfg.Host != "" { - loginHost = cfg.Host - } - } + // Flag, then NULLIFY_HOST, then the config file - matching every other + // command's precedence - before falling back to an interactive prompt. + loginHost, _ := lookupHostForAuth(ctx) // If still no host, prompt if loginHost == "" { @@ -88,18 +82,16 @@ var statusCmd = &cobra.Command{ Short: "Show authentication status", Long: "Display the current authentication state including host, user, and token expiry.", Run: func(cmd *cobra.Command, args []string) { - cfg, err := auth.LoadConfig() - if err != nil { - fmt.Println("Not configured. Run 'nullify auth login --host .nullify.ai' to get started.") - return - } + ctx := setupLogger(cmd.Context()) + defer logger.Close(ctx) - if cfg.Host == "" { - fmt.Println("No host configured. Run 'nullify auth login --host .nullify.ai'") + statusHost, ok := lookupHostForAuth(ctx) + if !ok { + fmt.Println("Not configured. Run 'nullify auth login --host .nullify.ai' to get started.") return } - fmt.Printf("Host: %s\n", cfg.Host) + fmt.Printf("Host: %s\n", statusHost) creds, err := auth.LoadCredentials() if err != nil { @@ -107,7 +99,7 @@ var statusCmd = &cobra.Command{ return } - hostCreds, ok := creds[auth.CredentialKey(cfg.Host)] + hostCreds, ok := creds[auth.CredentialKey(statusHost)] if !ok { fmt.Println("Status: not authenticated") return @@ -225,25 +217,58 @@ func init() { authCmd.AddCommand(configShowCmd) } -func resolveHostForAuth(ctx context.Context) string { +// lookupHostForAuth resolves the host for the auth subcommands using the same +// precedence as resolveHost: the --host flag, then NULLIFY_HOST, then the +// config file. It reports absence to the caller rather than exiting so that +// 'auth status' can print guidance instead of failing. +// +// Honouring NULLIFY_HOST matters for container and MCP deployments, where the +// credentials file is seeded from a secret store and the host arrives purely +// through the environment; without it these commands report "not configured" +// on a host that every other command resolves fine. +func lookupHostForAuth(ctx context.Context) (string, bool) { if host != "" { sanitized, err := lib.SanitizeNullifyHost(host) if err != nil { fmt.Fprintf(os.Stderr, "Error: invalid host %q\n", host) os.Exit(1) } - return sanitized + return sanitized, true + } + + if envHost := os.Getenv("NULLIFY_HOST"); envHost != "" { + sanitized, err := lib.SanitizeNullifyHost(envHost) + if err == nil { + return sanitized, true + } + logger.L(ctx).Warn( + "NULLIFY_HOST env var is invalid, falling through to config", + logger.String("host", envHost), + logger.Err(err), + ) } cfg, err := auth.LoadConfig() if err == nil && cfg.Host != "" { sanitized, sErr := lib.SanitizeNullifyHost(cfg.Host) if sErr == nil { - return sanitized + return sanitized, true } + logger.L(ctx).Warn( + "config file host is invalid, ignoring", + logger.String("host", cfg.Host), + logger.Err(sErr), + ) } - fmt.Fprintln(os.Stderr, "Error: no host configured. Use --host or run 'nullify auth login'") - os.Exit(1) - return "" + return "", false +} + +func resolveHostForAuth(ctx context.Context) string { + resolved, ok := lookupHostForAuth(ctx) + if !ok { + fmt.Fprintln(os.Stderr, "Error: no host configured. Use --host, set NULLIFY_HOST, or run 'nullify auth login'") + os.Exit(1) + } + return resolved } diff --git a/cmd/cli/cmd/auth_host_test.go b/cmd/cli/cmd/auth_host_test.go new file mode 100644 index 0000000..27f89ec --- /dev/null +++ b/cmd/cli/cmd/auth_host_test.go @@ -0,0 +1,108 @@ +package cmd + +import ( + "context" + "testing" + + "github.com/nullify-platform/cli/internal/auth" + "github.com/stretchr/testify/require" +) + +// withCleanHostFlag isolates the package-level --host flag so each case starts +// from a known state and later tests are unaffected. +func withCleanHostFlag(t *testing.T, value string) { + t.Helper() + + original := host + host = value + t.Cleanup(func() { host = original }) +} + +func TestLookupHostForAuthUsesEnvWhenNoFlagOrConfig(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_HOST", "acme.nullify.ai") + withCleanHostFlag(t, "") + + resolved, ok := lookupHostForAuth(setupLogger(context.Background())) + require.True(t, ok, "NULLIFY_HOST should satisfy auth host resolution") + require.Equal(t, "acme.nullify.ai", resolved) +} + +func TestLookupHostForAuthPrefersFlagOverEnv(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_HOST", "from-env.nullify.ai") + withCleanHostFlag(t, "from-flag.nullify.ai") + + resolved, ok := lookupHostForAuth(setupLogger(context.Background())) + require.True(t, ok) + require.Equal(t, "from-flag.nullify.ai", resolved) +} + +func TestLookupHostForAuthPrefersEnvOverConfig(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_HOST", "from-env.nullify.ai") + withCleanHostFlag(t, "") + + require.NoError(t, auth.SaveConfig(&auth.Config{Host: "from-config.nullify.ai"})) + + resolved, ok := lookupHostForAuth(setupLogger(context.Background())) + require.True(t, ok) + require.Equal(t, "from-env.nullify.ai", resolved) +} + +func TestLookupHostForAuthFallsBackToConfig(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_HOST", "") + withCleanHostFlag(t, "") + + require.NoError(t, auth.SaveConfig(&auth.Config{Host: "from-config.nullify.ai"})) + + resolved, ok := lookupHostForAuth(setupLogger(context.Background())) + require.True(t, ok) + require.Equal(t, "from-config.nullify.ai", resolved) +} + +// An unusable NULLIFY_HOST must not shadow a working config file, mirroring +// resolveHost's behaviour for the non-auth commands. +func TestLookupHostForAuthIgnoresInvalidEnvAndUsesConfig(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_HOST", "not a valid host!!") + withCleanHostFlag(t, "") + + require.NoError(t, auth.SaveConfig(&auth.Config{Host: "from-config.nullify.ai"})) + + resolved, ok := lookupHostForAuth(setupLogger(context.Background())) + require.True(t, ok) + require.Equal(t, "from-config.nullify.ai", resolved) +} + +func TestLookupHostForAuthReportsAbsenceInsteadOfExiting(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_HOST", "") + withCleanHostFlag(t, "") + + resolved, ok := lookupHostForAuth(setupLogger(context.Background())) + require.False(t, ok, "no host anywhere should report absence, not exit") + require.Empty(t, resolved) +} + +// The bare and api-prefixed spellings must both normalise to the credential +// key form, so a seeded credentials file is found either way. +func TestLookupHostForAuthNormalisesEnvHostSpellings(t *testing.T) { + for _, envHost := range []string{ + "acme", + "acme.nullify.ai", + "api.acme.nullify.ai", + "https://acme.nullify.ai/", + } { + t.Run(envHost, func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_HOST", envHost) + withCleanHostFlag(t, "") + + resolved, ok := lookupHostForAuth(setupLogger(context.Background())) + require.True(t, ok) + require.Equal(t, "acme.nullify.ai", resolved) + }) + } +} diff --git a/cmd/cli/cmd/mcp.go b/cmd/cli/cmd/mcp.go index f9cc7ac..2b79c0f 100644 --- a/cmd/cli/cmd/mcp.go +++ b/cmd/cli/cmd/mcp.go @@ -63,11 +63,17 @@ var mcpServeCmd = &cobra.Command{ os.Exit(1) } - // Create a refreshing client for long-running MCP sessions + // Create a refreshing client for long-running MCP sessions. + // refreshProvider is deliberately distinct: tokenProvider serves the + // stored token while it still looks valid, so reusing it for the 401 + // retry would replay the token the server just rejected. tokenProvider := func() (string, error) { return lib.GetNullifyToken(ctx, authCtx.Host, nullifyToken, githubToken) } - nullifyClient, clientErr := client.NewRefreshingNullifyClient(authCtx.Host, tokenProvider) + refreshProvider := func() (string, error) { + return lib.RefreshNullifyToken(ctx, authCtx.Host, nullifyToken, githubToken) + } + nullifyClient, clientErr := client.NewRefreshingNullifyClient(authCtx.Host, tokenProvider, refreshProvider) if clientErr != nil { fmt.Fprintf(os.Stderr, "Error: failed to create client: %v\n", clientErr) os.Exit(1) diff --git a/internal/auth/force_refresh_test.go b/internal/auth/force_refresh_test.go new file mode 100644 index 0000000..c9f8272 --- /dev/null +++ b/internal/auth/force_refresh_test.go @@ -0,0 +1,62 @@ +package auth + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// Credentials with no refresh token can never yield a new access token, so the +// caller must be able to distinguish that from a transient failure and stop +// retrying. +func TestForceRefreshTokenReportsNotRefreshableWithoutRefreshToken(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + require.NoError(t, SaveHostCredentials("acme.nullify.ai", HostCredentials{ + AccessToken: "access-only", + ExpiresAt: time.Now().Add(time.Hour).Unix(), + })) + + _, err := ForceRefreshToken(context.Background(), "acme.nullify.ai") + require.ErrorIs(t, err, ErrNotRefreshable) +} + +func TestForceRefreshTokenRequiresCredentialsForHost(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + require.NoError(t, SaveHostCredentials("other.nullify.ai", HostCredentials{ + AccessToken: "a", + RefreshToken: "r", + })) + + _, err := ForceRefreshToken(context.Background(), "acme.nullify.ai") + require.Error(t, err) + require.NotErrorIs(t, err, ErrNotRefreshable, + "a missing host is not the same as a host with no refresh token") +} + +// ForceRefreshToken must attempt a refresh even while the stored access token +// still looks valid - that is the whole point of it versus GetValidToken, which +// would hand back the unexpired-but-revoked token unchanged. +func TestForceRefreshTokenIgnoresUnexpiredAccessToken(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + require.NoError(t, SaveHostCredentials("acme.nullify.ai", HostCredentials{ + AccessToken: "still-valid-looking", + RefreshToken: "refresh-me", + ExpiresAt: time.Now().Add(time.Hour).Unix(), + })) + + // GetValidToken short-circuits and returns the stored token untouched. + stored, err := GetValidToken(context.Background(), "acme.nullify.ai") + require.NoError(t, err) + require.Equal(t, "still-valid-looking", stored) + + // ForceRefreshToken instead reaches the network; with no server reachable + // it fails, which still proves it did not short-circuit on the expiry. + got, err := ForceRefreshToken(context.Background(), "acme.nullify.ai") + require.Error(t, err) + require.NotEqual(t, "still-valid-looking", got) +} diff --git a/internal/auth/login.go b/internal/auth/login.go index 0d06b5d..4b83dc9 100644 --- a/internal/auth/login.go +++ b/internal/auth/login.go @@ -3,6 +3,7 @@ package auth import ( "context" "encoding/json" + "errors" "fmt" "io" "net" @@ -198,6 +199,32 @@ func GetValidToken(ctx context.Context, host string) (string, error) { return hostCreds.AccessToken, nil } +// ErrNotRefreshable reports that the stored credentials for a host carry no +// refresh token, so no amount of retrying will produce a new access token. +var ErrNotRefreshable = errors.New("no refresh token stored") + +// ForceRefreshToken exchanges the stored refresh token for a new access token +// regardless of the recorded expiry. GetValidToken only refreshes once +// ExpiresAt has elapsed, which means a token revoked mid-life is handed back +// unchanged - so a 401 retry built on it would replay the same rejected value. +func ForceRefreshToken(ctx context.Context, host string) (string, error) { + creds, err := LoadCredentials() + if err != nil { + return "", fmt.Errorf("not authenticated - run 'nullify auth login'") + } + + hostCreds, ok := creds[CredentialKey(host)] + if !ok { + return "", fmt.Errorf("not authenticated for %s - run 'nullify auth login --host %s'", host, host) + } + if hostCreds.RefreshToken == "" { + return "", ErrNotRefreshable + } + + logger.L(ctx).Debug("forcing access token refresh") + return refreshToken(ctx, host, hostCreds.RefreshToken) +} + func createCLISession(ctx context.Context, host string, port int) (*cliSessionResponse, error) { url := fmt.Sprintf("https://%s/auth/cli/session", apiHost(host)) diff --git a/internal/client/refreshing_transport.go b/internal/client/refreshing_transport.go index 9567f24..55738b4 100644 --- a/internal/client/refreshing_transport.go +++ b/internal/client/refreshing_transport.go @@ -1,7 +1,10 @@ package client import ( + "bytes" "context" + "errors" + "io" "net/http" "strings" "sync" @@ -13,34 +16,53 @@ import ( // TokenProvider is a function that returns a valid token. type TokenProvider func() (string, error) +// ErrTokenNotRefreshable reports that the active token source yields a fixed +// string, so retrying after a 401 cannot produce a different token. +var ErrTokenNotRefreshable = errors.New("token source cannot be refreshed") + // refreshingAuthTransport wraps authTransport and refreshes the token proactively. type refreshingAuthTransport struct { nullifyHost string tokenProvider TokenProvider - transport http.RoundTripper + // refreshProvider mints a genuinely new token, ignoring any cached expiry. + // Distinct from tokenProvider, which returns the stored token untouched + // while it still looks valid. + refreshProvider TokenProvider + transport http.RoundTripper mu sync.RWMutex cachedToken string cachedAt time.Time - cacheTTL time.Duration + // providerRetryAfter and refreshRetryAfter are separate clocks because + // tokenProvider and refreshProvider are separate functions: one failing + // says nothing about the other. + providerRetryAfter time.Time + refreshRetryAfter time.Time + cacheTTL time.Duration + failureBackoff time.Duration } // NewRefreshingNullifyClient creates a NullifyClient that automatically refreshes // its auth token, suitable for long-running processes like MCP servers. -func NewRefreshingNullifyClient(nullifyHost string, tokenProvider TokenProvider) (*NullifyClient, error) { - // Get initial token +// +// refreshProvider is used to recover from a 401 and must bypass any cached +// expiry; passing tokenProvider for both makes the 401 retry a no-op. +func NewRefreshingNullifyClient(nullifyHost string, tokenProvider, refreshProvider TokenProvider) (*NullifyClient, error) { + // Fetch an initial token so startup fails fast on auth problems. token, err := tokenProvider() if err != nil { return nil, err } t := &refreshingAuthTransport{ - nullifyHost: nullifyHost, - tokenProvider: tokenProvider, - transport: http.DefaultTransport, - cachedToken: token, - cachedAt: time.Now(), - cacheTTL: 5 * time.Minute, + nullifyHost: nullifyHost, + tokenProvider: tokenProvider, + refreshProvider: refreshProvider, + transport: http.DefaultTransport, + cachedToken: token, + cachedAt: time.Now(), + cacheTTL: 5 * time.Minute, + failureBackoff: 30 * time.Second, } httpClient := &http.Client{ @@ -61,9 +83,16 @@ func NewRefreshingNullifyClient(nullifyHost string, tokenProvider TokenProvider) }, nil } +// fresh reports whether the cached token can be served without consulting the +// provider: either it is still within its TTL, or a recent provider failure is +// still inside its backoff window. Callers must hold t.mu. +func (t *refreshingAuthTransport) fresh() bool { + return time.Since(t.cachedAt) < t.cacheTTL || time.Now().Before(t.providerRetryAfter) +} + func (t *refreshingAuthTransport) getToken(ctx context.Context) string { t.mu.RLock() - if time.Since(t.cachedAt) < t.cacheTTL { + if t.fresh() { token := t.cachedToken t.mu.RUnlock() return token @@ -73,27 +102,109 @@ func (t *refreshingAuthTransport) getToken(ctx context.Context) string { // Double-checked locking t.mu.Lock() defer t.mu.Unlock() - if time.Since(t.cachedAt) < t.cacheTTL { + if t.fresh() { return t.cachedToken } newToken, err := t.tokenProvider() if err != nil { - // Fall back to cached token; log so the user can diagnose 401s + // Fall back to cached token; log so the user can diagnose 401s. Hold + // off on the next attempt so a persistently failing provider does not + // put a blocking refresh in front of every single request. + t.providerRetryAfter = time.Now().Add(t.failureBackoff) logger.L(ctx).Warn("token refresh failed, using cached token", logger.Err(err)) return t.cachedToken } + t.providerRetryAfter = time.Time{} t.cachedToken = newToken t.cachedAt = time.Now() return t.cachedToken } +// forceRefresh mints a replacement for a token the server has just rejected. +// It returns "" when it cannot make progress, which tells the caller to +// surface the original 401 rather than retry. +func (t *refreshingAuthTransport) forceRefresh(ctx context.Context, staleToken string) string { + t.mu.Lock() + defer t.mu.Unlock() + + // Another request already replaced it while this one was in flight. + if t.cachedToken != staleToken { + return t.cachedToken + } + if time.Now().Before(t.refreshRetryAfter) { + return "" + } + + newToken, err := t.refreshProvider() + if err != nil { + t.refreshRetryAfter = time.Now().Add(t.failureBackoff) + // A fixed token source has nothing to re-fetch, so the 401 is final + // rather than a failure worth warning about. + if errors.Is(err, ErrTokenNotRefreshable) { + logger.L(ctx).Debug("token source cannot be refreshed, surfacing 401") + } else { + logger.L(ctx).Warn("forced token refresh failed after 401", logger.Err(err)) + } + return "" + } + + // Neither an empty token nor the one the server just rejected is progress. + // Leave the cache alone rather than replacing a working token with junk or + // re-fetching the same rejected string on every subsequent request. + if newToken == "" || newToken == staleToken { + t.refreshRetryAfter = time.Now().Add(t.failureBackoff) + logger.L(ctx).Warn("forced token refresh returned no new token, surfacing 401") + return "" + } + + t.providerRetryAfter = time.Time{} + t.refreshRetryAfter = time.Time{} + t.cachedToken = newToken + t.cachedAt = time.Now() + return newToken +} + func (t *refreshingAuthTransport) RoundTrip(req *http.Request) (*http.Response, error) { + // Buffer the body so the request can be replayed if the first attempt 401s. + var bodyBytes []byte + if req.Body != nil { + b, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + req.Body.Close() + bodyBytes = b + } + + attempt := func(token string) (*http.Response, error) { + r := req.Clone(req.Context()) + if bodyBytes != nil { + r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) + r.ContentLength = int64(len(bodyBytes)) + } + r.Header.Set("Authorization", "Bearer "+token) + r.Header.Set("User-Agent", "Nullify-CLI/mcp") + return t.transport.RoundTrip(r) + } + token := t.getToken(req.Context()) + resp, err := attempt(token) + if err != nil { + return nil, err + } + + // The cached token can be invalid before its TTL elapses (revocation, server + // session kill, clock skew). On a 401, force a refresh and retry once before + // surfacing the failure. + if resp.StatusCode == http.StatusUnauthorized { + if newToken := t.forceRefresh(req.Context(), token); newToken != "" && newToken != token { + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4<<10)) + resp.Body.Close() + return attempt(newToken) + } + } - r := req.Clone(req.Context()) - r.Header.Set("Authorization", "Bearer "+token) - r.Header.Set("User-Agent", "Nullify-CLI/mcp") - return t.transport.RoundTrip(r) + return resp, nil } diff --git a/internal/client/refreshing_transport_test.go b/internal/client/refreshing_transport_test.go new file mode 100644 index 0000000..c39f635 --- /dev/null +++ b/internal/client/refreshing_transport_test.go @@ -0,0 +1,174 @@ +package client + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// newTestTransport wires a transport around a stub round tripper with the same +// defaults the constructor uses, minus the initial token fetch. +func newTestTransport(cached string, refresh TokenProvider, rt http.RoundTripper) *refreshingAuthTransport { + return &refreshingAuthTransport{ + nullifyHost: "acme.nullify.ai", + tokenProvider: func() (string, error) { return cached, nil }, + refreshProvider: refresh, + transport: rt, + cachedToken: cached, + cachedAt: time.Now(), + cacheTTL: 5 * time.Minute, + failureBackoff: 30 * time.Second, + } +} + +// tokenRecorder serves 401 to anything but wantToken, and records every bearer +// token it was offered. +type tokenRecorder struct { + wantToken string + seen []string + bodies []string +} + +func (s *tokenRecorder) RoundTrip(req *http.Request) (*http.Response, error) { + s.seen = append(s.seen, strings.TrimPrefix(req.Header.Get("Authorization"), "Bearer ")) + + body := "" + if req.Body != nil { + b, _ := io.ReadAll(req.Body) + body = string(b) + } + s.bodies = append(s.bodies, body) + + code := http.StatusUnauthorized + if s.seen[len(s.seen)-1] == s.wantToken { + code = http.StatusOK + } + return &http.Response{ + StatusCode: code, + Body: io.NopCloser(strings.NewReader("")), + Header: make(http.Header), + Request: req, + }, nil +} + +func TestRoundTripRefreshesAndRetriesOn401(t *testing.T) { + stub := &tokenRecorder{wantToken: "fresh-token"} + tr := newTestTransport("stale-token", func() (string, error) { return "fresh-token", nil }, stub) + + resp, err := tr.RoundTrip(httptest.NewRequest(http.MethodGet, "https://api.acme.nullify.ai/x", nil)) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Equal(t, []string{"stale-token", "fresh-token"}, stub.seen, + "a 401 must trigger exactly one retry with the refreshed token") + require.Equal(t, "fresh-token", tr.cachedToken, "the refreshed token must be cached") +} + +// A token that cannot be refreshed - a flag or NULLIFY_TOKEN - must surface the +// 401 rather than replaying the same rejected string. +func TestRoundTripSurfaces401WhenSourceNotRefreshable(t *testing.T) { + stub := &tokenRecorder{wantToken: "unreachable"} + tr := newTestTransport("fixed-token", func() (string, error) { return "", ErrTokenNotRefreshable }, stub) + + resp, err := tr.RoundTrip(httptest.NewRequest(http.MethodGet, "https://api.acme.nullify.ai/x", nil)) + require.NoError(t, err) + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + require.Equal(t, []string{"fixed-token"}, stub.seen, "must not retry a fixed token source") + require.False(t, tr.refreshRetryAfter.IsZero(), "backoff must be armed so every later request does not re-refresh") +} + +// Getting the same token back is not progress; retrying would loop forever. +func TestRoundTripDoesNotRetryWhenRefreshReturnsSameToken(t *testing.T) { + stub := &tokenRecorder{wantToken: "unreachable"} + tr := newTestTransport("same-token", func() (string, error) { return "same-token", nil }, stub) + + resp, err := tr.RoundTrip(httptest.NewRequest(http.MethodGet, "https://api.acme.nullify.ai/x", nil)) + require.NoError(t, err) + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + require.Equal(t, []string{"same-token"}, stub.seen) + require.Equal(t, "same-token", tr.cachedToken, "a rejected token must not overwrite the cache") + require.False(t, tr.refreshRetryAfter.IsZero()) +} + +func TestRoundTripDoesNotOverwriteCacheWithEmptyToken(t *testing.T) { + stub := &tokenRecorder{wantToken: "unreachable"} + tr := newTestTransport("good-token", func() (string, error) { return "", nil }, stub) + + _, err := tr.RoundTrip(httptest.NewRequest(http.MethodGet, "https://api.acme.nullify.ai/x", nil)) + require.NoError(t, err) + require.Equal(t, "good-token", tr.cachedToken, "an empty refresh must leave the working token in place") +} + +// The retry must send the same body, not an empty one. +func TestRoundTripReplaysBodyOnRetry(t *testing.T) { + stub := &tokenRecorder{wantToken: "fresh-token"} + tr := newTestTransport("stale-token", func() (string, error) { return "fresh-token", nil }, stub) + + req := httptest.NewRequest(http.MethodPost, "https://api.acme.nullify.ai/x", strings.NewReader(`{"a":1}`)) + resp, err := tr.RoundTrip(req) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Equal(t, []string{`{"a":1}`, `{"a":1}`}, stub.bodies, "the replayed request must carry the original body") +} + +// While backoff is armed, a 401 must not call the refresh provider again. +func TestForceRefreshHonoursBackoffWindow(t *testing.T) { + var calls atomic.Int32 + stub := &tokenRecorder{wantToken: "unreachable"} + tr := newTestTransport("stale-token", func() (string, error) { + calls.Add(1) + return "", ErrTokenNotRefreshable + }, stub) + + for i := 0; i < 3; i++ { + _, err := tr.RoundTrip(httptest.NewRequest(http.MethodGet, "https://api.acme.nullify.ai/x", nil)) + require.NoError(t, err) + } + require.Equal(t, int32(1), calls.Load(), "backoff must suppress repeated refresh attempts") +} + +// Another in-flight request may have already replaced the token. +func TestForceRefreshReturnsCurrentTokenWhenAlreadyRotated(t *testing.T) { + tr := newTestTransport("newer-token", func() (string, error) { + t.Fatal("refreshProvider must not be called when the cache already moved on") + return "", nil + }, &tokenRecorder{}) + + require.Equal(t, "newer-token", tr.forceRefresh(context.Background(), "older-token")) +} + +func TestRoundTripLeavesNon401ResponsesAlone(t *testing.T) { + stub := &tokenRecorder{wantToken: "cached-token"} + tr := newTestTransport("cached-token", func() (string, error) { + t.Fatal("a successful response must not trigger a refresh") + return "", nil + }, stub) + + resp, err := tr.RoundTrip(httptest.NewRequest(http.MethodGet, "https://api.acme.nullify.ai/x", nil)) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Len(t, stub.seen, 1) +} + +// A failing tokenProvider must serve the cached token and arm its own clock, +// separate from the refresh clock. +func TestGetTokenBacksOffAfterProviderFailure(t *testing.T) { + var calls atomic.Int32 + tr := newTestTransport("cached-token", nil, &tokenRecorder{}) + tr.tokenProvider = func() (string, error) { + calls.Add(1) + return "", io.ErrUnexpectedEOF + } + tr.cachedAt = time.Now().Add(-time.Hour) // force the TTL path + + require.Equal(t, "cached-token", tr.getToken(context.Background())) + require.Equal(t, "cached-token", tr.getToken(context.Background())) + require.Equal(t, int32(1), calls.Load(), "provider backoff must suppress the second attempt") + require.True(t, tr.refreshRetryAfter.IsZero(), "a provider failure must not arm the refresh clock") +} diff --git a/internal/lib/get_token.go b/internal/lib/get_token.go index 7659307..14e5e69 100644 --- a/internal/lib/get_token.go +++ b/internal/lib/get_token.go @@ -22,14 +22,41 @@ type githubToken struct { Token string `json:"accessToken"` } +// GetNullifyToken resolves a token from the first source that has one. func GetNullifyToken( ctx context.Context, nullifyHost string, nullifyTokenFlag string, githubTokenFlag string, +) (string, error) { + return resolveNullifyToken(ctx, nullifyHost, nullifyTokenFlag, githubTokenFlag, false) +} + +// RefreshNullifyToken resolves a token that is genuinely new, for recovering +// from a 401. Sources that yield a fixed string - the --nullify-token flag and +// NULLIFY_TOKEN - report client.ErrTokenNotRefreshable rather than handing back +// the value the server just rejected, which would make the retry a no-op. +func RefreshNullifyToken( + ctx context.Context, + nullifyHost string, + nullifyTokenFlag string, + githubTokenFlag string, +) (string, error) { + return resolveNullifyToken(ctx, nullifyHost, nullifyTokenFlag, githubTokenFlag, true) +} + +func resolveNullifyToken( + ctx context.Context, + nullifyHost string, + nullifyTokenFlag string, + githubTokenFlag string, + force bool, ) (string, error) { // 1. Command-line flag if nullifyTokenFlag != "" { + if force { + return "", client.ErrTokenNotRefreshable + } logger.L(ctx).Debug("using token from flag") return nullifyTokenFlag, nil } @@ -37,6 +64,9 @@ func GetNullifyToken( // 2. Environment variable token := os.Getenv("NULLIFY_TOKEN") if token != "" { + if force { + return "", client.ErrTokenNotRefreshable + } logger.L(ctx).Debug("using token from env") return token, nil } @@ -94,12 +124,22 @@ func GetNullifyToken( } // 4. Stored credentials from ~/.nullify/credentials.json - storedToken, err := auth.GetValidToken(ctx, nullifyHost) + resolve := auth.GetValidToken + if force { + // GetValidToken returns the stored token untouched while it still looks + // valid, so a token revoked mid-life would come back unchanged. + resolve = auth.ForceRefreshToken + } + + storedToken, err := resolve(ctx, nullifyHost) if err == nil && storedToken != "" { logger.L(ctx).Debug("using token from stored credentials") return storedToken, nil } if err != nil { + if errors.Is(err, auth.ErrNotRefreshable) { + return "", client.ErrTokenNotRefreshable + } return "", fmt.Errorf("stored credentials: %w", err) } diff --git a/internal/lib/refresh_token_test.go b/internal/lib/refresh_token_test.go new file mode 100644 index 0000000..c018ac7 --- /dev/null +++ b/internal/lib/refresh_token_test.go @@ -0,0 +1,52 @@ +package lib + +import ( + "context" + "testing" + + "github.com/nullify-platform/cli/internal/client" + "github.com/stretchr/testify/require" +) + +// The --nullify-token flag yields a fixed string. Handing it back after a 401 +// would replay the token the server just rejected, so the refresh path must +// report that the source cannot be refreshed instead. +func TestRefreshNullifyTokenRejectsFlagSource(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_TOKEN", "") + + _, err := RefreshNullifyToken(context.Background(), "acme.nullify.ai", "flag-token", "") + require.ErrorIs(t, err, client.ErrTokenNotRefreshable) +} + +func TestRefreshNullifyTokenRejectsEnvSource(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_TOKEN", "env-token") + + _, err := RefreshNullifyToken(context.Background(), "acme.nullify.ai", "", "") + require.ErrorIs(t, err, client.ErrTokenNotRefreshable) +} + +// The non-forced path must keep returning those same fixed sources. +func TestGetNullifyTokenStillUsesFixedSources(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_TOKEN", "env-token") + + token, err := GetNullifyToken(context.Background(), "acme.nullify.ai", "", "") + require.NoError(t, err) + require.Equal(t, "env-token", token) + + token, err = GetNullifyToken(context.Background(), "acme.nullify.ai", "flag-token", "") + require.NoError(t, err) + require.Equal(t, "flag-token", token, "the flag must outrank the environment") +} + +// With no fixed source and no stored credentials there is nothing to refresh, +// and the transport must be told so rather than shown a generic failure. +func TestRefreshNullifyTokenReportsNotRefreshableWithoutStoredCredentials(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_TOKEN", "") + + _, err := RefreshNullifyToken(context.Background(), "acme.nullify.ai", "", "") + require.Error(t, err) +}