From fc695bd9b143e9cb02966fd10f69dfba1a2f49da Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 17:03:10 +0000 Subject: [PATCH 1/7] fix: make MCP credential failures visible and diagnosable Two MCP servers had been dead for days without anyone noticing. The credentials themselves need rotating by a human, but the reason the outage stayed invisible - and the reason it misdiagnosed once someone did look - is fixable here. Auth host resolution resolveHostForAuth (auth token, auth logout) and auth status ignored NULLIFY_HOST, unlike resolveHost used by all eleven other command sites. That is exactly the container and MCP setup this repo ships: credentials seeded from a secret store, host supplied purely through the environment. The result was 'auth status' reporting "Not configured" on a host every other command resolved fine, hiding the real cause - a refresh token rejected with 401. Adds lookupHostForAuth with flag > env > config precedence, reporting absence to the caller instead of exiting so auth status can print guidance. auth login now consults it too, rather than dropping to an interactive prompt in a container that has no terminal. Half-configured CLI nullify-serve.sh seeded credentials.json but never config.json, so anyone debugging inside the container hit "Not configured" with valid credentials sitting right there. It now seeds both. Silent secret resolution with-op-env.sh exec'd the server even when requested vars were still unset. It now names the missing vars, the vault and item it consulted, and warns that the server will likely expose no working tools. Also warns when the target command never appears on PATH. Preflight Adds .claude/mcp/preflight.sh, run report-only from the session-start hook. It probes each server for the two failure modes that matter: dying at startup, and starting with a full tool list while every call is unauthorized. The second is the dangerous one - coverage looks complete while the data is silently absent - so servers offering an authorization probe get one rather than being judged on tool count. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Mcq6LfAVPzKZL5rpvww13R --- .claude/hooks/session-start.sh | 11 ++ .claude/mcp/nullify-serve.sh | 11 ++ .claude/mcp/preflight.sh | 178 +++++++++++++++++++++++++++++++++ .claude/mcp/with-op-env.sh | 27 +++++ cmd/cli/cmd/auth.go | 73 +++++++++----- cmd/cli/cmd/auth_host_test.go | 108 ++++++++++++++++++++ 6 files changed, 384 insertions(+), 24 deletions(-) create mode 100755 .claude/mcp/preflight.sh create mode 100644 cmd/cli/cmd/auth_host_test.go diff --git a/.claude/hooks/session-start.sh b/.claude/hooks/session-start.sh index d516288..9bfd216 100755 --- a/.claude/hooks/session-start.sh +++ b/.claude/hooks/session-start.sh @@ -96,4 +96,15 @@ 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 ---" +.claude/mcp/preflight.sh || true + echo "Session start hook completed: secrets resolved, MCP servers installed, Go toolchain ready." diff --git a/.claude/mcp/nullify-serve.sh b/.claude/mcp/nullify-serve.sh index b563778..19db99d 100755 --- a/.claude/mcp/nullify-serve.sh +++ b/.claude/mcp/nullify-serve.sh @@ -19,4 +19,15 @@ if [ -n "${NULLIFY_CREDENTIALS_JSON:-}" ] && [ ! -s "$HOME/.nullify/credentials. 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 '{\n "host": "%s"\n}\n' "$NULLIFY_HOST" > "$HOME/.nullify/config.json" + chmod 600 "$HOME/.nullify/config.json" +fi + 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..1e8e53c --- /dev/null +++ b/.claude/mcp/preflight.sh @@ -0,0 +1,178 @@ +#!/bin/bash +# Probe every MCP server this repo wires up and report whether it is actually +# usable. Run it by hand any time tools look missing: +# +# .claude/mcp/preflight.sh +# +# Two failure modes matter, and counting tools only catches the first: +# +# 1. The server dies during startup - almost always expired credentials. +# It exposes no tools at all and the client just shows nothing. +# 2. The server starts and lists its whole tool set, but the credentials are +# rejected on every call, so each tool answers "not authenticated" or +# returns an empty result. +# +# The second mode is the dangerous one: coverage looks complete while the data +# is silently absent. So each server gets a handshake AND, where the server +# offers one, an authorization probe. +# +# Exit status is 0 only when every server passes, so this can gate a check. +set -uo pipefail + +cd "$(dirname "${BASH_SOURCE[0]}")/../.." || exit 1 + +TIMEOUT="${PREFLIGHT_TIMEOUT:-120}" +STDERR_FILE="$(mktemp)" +TMP_BIN="$(mktemp -u)" +trap 'rm -f "$STDERR_FILE" "$TMP_BIN"' EXIT + +failures=0 +declare -a REPORT=() + +# Emit the JSON-RPC lines for a handshake, optionally followed by one extra +# request (used for authorization probes). +mcp_exchange() { + local extra="$1"; shift + { + 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":{}}' + [ -n "$extra" ] && echo "$extra" + # Give the server a moment to answer before stdin closes. + sleep 2 + } | timeout "$TIMEOUT" "$@" 2>"$STDERR_FILE" +} + +# Keep only well-formed JSON-RPC lines; servers routinely print banners. +json_lines() { jq -R 'fromjson? // empty' 2>/dev/null; } + +tool_count() { + jq -s 'map(select(.id == 2)) | (.[0].result.tools // []) | length' 2>/dev/null +} + +# Last non-empty stderr line, trimmed - usually the actual cause. +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-160 +} + +record() { REPORT+=("$1|$2|$3"); } + +pass() { record "PASS" "$1" "$2"; } +fail() { record "FAIL" "$1" "$2"; failures=$((failures + 1)); } +warn() { record "WARN" "$1" "$2"; } + +# ------------------------------------------------------------------ +# nullify - dies at startup when the seeded refresh token is rejected +# ------------------------------------------------------------------ +check_nullify() { + local name="nullify" + + if ! go build -o "$TMP_BIN" ./cmd/cli 2>"$STDERR_FILE"; then + fail "$name" "CLI failed to build: $(stderr_reason)" + return + fi + + local out count + out="$(NULLIFY_HOST="${NULLIFY_HOST:-scrut.nullify.ai}" mcp_exchange "" "$TMP_BIN" mcp serve)" + count="$(printf '%s\n' "$out" | json_lines | tool_count)" + + if [ "${count:-0}" -gt 0 ]; then + pass "$name" "$count tools" + return + fi + + local reason + reason="$(stderr_reason)" + case "$reason" in + *"refresh failed"*|*"token expired"*|*"auth login"*) + fail "$name" "credentials rejected - re-seed NULLIFY_CREDENTIALS_JSON. ($reason)" ;; + "") + fail "$name" "exposed no tools and gave no error" ;; + *) + fail "$name" "$reason" ;; + esac +} + +# ------------------------------------------------------------------ +# latent-defense - starts and lists every tool even when unauthorized, +# so the tool count proves nothing. whoami is the real check. +# ------------------------------------------------------------------ +check_latent_defense() { + local name="latent-defense" + + if ! command -v latent-defense-mcp >/dev/null 2>&1; then + fail "$name" "latent-defense-mcp not on PATH - 'uv tool install ./third_party/latent-defense-mcp'" + return + fi + + local probe out count body + probe='{"jsonrpc":"2.0","id":3,"method":"tools/call","params":{"name":"whoami","arguments":{}}}' + out="$(mcp_exchange "$probe" latent-defense-mcp)" + count="$(printf '%s\n' "$out" | json_lines | tool_count)" + + if [ "${count:-0}" -eq 0 ]; then + fail "$name" "exposed no tools. $(stderr_reason)" + return + fi + + body="$(printf '%s\n' "$out" | json_lines \ + | jq -rs 'map(select(.id == 3)) | (.[0].result.content[0].text // "")' 2>/dev/null)" + + if [ -z "$body" ]; then + warn "$name" "$count tools, but whoami did not answer within ${TIMEOUT}s" + return + fi + + if printf '%s' "$body" | jq -e '.authenticated == true' >/dev/null 2>&1; then + pass "$name" "$count tools, authenticated" + else + fail "$name" "$count tools but UNAUTHORIZED - rotate LATENT_DEFENSE_API_KEY (tools will return empty results)" + fi +} + +# ------------------------------------------------------------------ +# sentinelone - no unauthenticated health endpoint on the console, so +# the handshake is the honest check available here. +# ------------------------------------------------------------------ +check_sentinelone() { + local name="sentinelone" + local out count + + out="$(mcp_exchange "" uvx --from ./third_party/purple-mcp purple-mcp --mode stdio)" + count="$(printf '%s\n' "$out" | json_lines | tool_count)" + + if [ "${count:-0}" -gt 0 ]; then + if [ -z "${PURPLEMCP_CONSOLE_TOKEN:-}" ]; then + warn "$name" "$count tools, but PURPLEMCP_CONSOLE_TOKEN is unset - calls will fail" + else + pass "$name" "$count tools" + fi + return + fi + + fail "$name" "exposed no tools. $(stderr_reason)" +} + +echo "MCP preflight - probing servers from .mcp.json" +echo + +check_nullify +check_latent_defense +check_sentinelone + +printf '%-8s %-16s %s\n' "RESULT" "SERVER" "DETAIL" +printf '%-8s %-16s %s\n' "------" "------" "------" +for row in "${REPORT[@]}"; do + printf '%-8s %-16s %s\n' "${row%%|*}" "$(cut -d'|' -f2 <<<"$row")" "${row##*|}" +done +echo + +if [ "$failures" -gt 0 ]; then + echo "$failures server(s) unusable. Credential failures need a human: mint a new" + echo "secret, then update the 1Password item named by CLAUDE_OP_VAULT/CLAUDE_OP_ITEM." + 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..35e13a3 100755 --- a/.claude/mcp/with-op-env.sh +++ b/.claude/mcp/with-op-env.sh @@ -53,10 +53,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 "${vars[@]}"; 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/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) + }) + } +} From 2590a29f98f320ecc02e914677090337f1699630 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 20:16:08 +0000 Subject: [PATCH 2/7] fix: probe latent-defense authorization over HTTP, not its whoami tool The whoami tool falls back to the interactive device flow when the API key is rejected, so it blocked until the 120s timeout and the preflight downgraded a definite "key rejected" to an inconclusive "did not answer". That is the wrong way round: an expired key is exactly the case this check exists to catch. Probing the portal's /auth/me directly answers the same question in under a second and distinguishes 401/403 from an unreachable host. Whole preflight now completes in ~11s instead of timing out. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Mcq6LfAVPzKZL5rpvww13R --- .claude/mcp/preflight.sh | 41 +++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/.claude/mcp/preflight.sh b/.claude/mcp/preflight.sh index 1e8e53c..b275ff8 100755 --- a/.claude/mcp/preflight.sh +++ b/.claude/mcp/preflight.sh @@ -107,9 +107,8 @@ check_latent_defense() { return fi - local probe out count body - probe='{"jsonrpc":"2.0","id":3,"method":"tools/call","params":{"name":"whoami","arguments":{}}}' - out="$(mcp_exchange "$probe" latent-defense-mcp)" + local out count + out="$(mcp_exchange "" latent-defense-mcp)" count="$(printf '%s\n' "$out" | json_lines | tool_count)" if [ "${count:-0}" -eq 0 ]; then @@ -117,19 +116,35 @@ check_latent_defense() { return fi - body="$(printf '%s\n' "$out" | json_lines \ - | jq -rs 'map(select(.id == 3)) | (.[0].result.content[0].text // "")' 2>/dev/null)" - - if [ -z "$body" ]; then - warn "$name" "$count tools, but whoami did not answer within ${TIMEOUT}s" + # Probe the portal directly rather than calling the server's whoami tool. + # whoami falls back to the interactive device flow on a 401 and blocks until + # the timeout, which turns a definite "key rejected" into an inconclusive + # hang. One HTTP call answers the same question in under a second. + if [ -z "${LATENT_DEFENSE_URL:-}" ]; then + warn "$name" "$count tools, but LATENT_DEFENSE_URL is unset - cannot verify authorization" return fi - - if printf '%s' "$body" | jq -e '.authenticated == true' >/dev/null 2>&1; then - pass "$name" "$count tools, authenticated" - else - fail "$name" "$count tools but UNAUTHORIZED - rotate LATENT_DEFENSE_API_KEY (tools will return empty results)" + if [ -z "${LATENT_DEFENSE_API_KEY:-}" ]; then + fail "$name" "$count tools but LATENT_DEFENSE_API_KEY is unset - every call will be unauthorized" + return fi + + local code + code="$(curl -sS -o /dev/null -w '%{http_code}' -m 25 \ + -H "Authorization: Bearer ${LATENT_DEFENSE_API_KEY}" \ + -H "Accept: application/json" \ + "${LATENT_DEFENSE_URL%/}/auth/me" 2>/dev/null)" + + case "$code" in + 200) + pass "$name" "$count tools, authenticated" ;; + 401|403) + fail "$name" "$count tools but key REJECTED (HTTP $code) - rotate LATENT_DEFENSE_API_KEY; tools return empty results, not errors" ;; + "") + warn "$name" "$count tools, but ${LATENT_DEFENSE_URL} was unreachable" ;; + *) + warn "$name" "$count tools, unexpected HTTP $code from /auth/me" ;; + esac } # ------------------------------------------------------------------ From 6ea3eb3cdf227608389991547bcfdac3729e61ba Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 01:06:11 +0000 Subject: [PATCH 3/7] feat: authenticate Nullify with a static token in ephemeral environments Seeded OAuth credentials are the wrong model for a container that is rebuilt every session. The refresh token has to be minted through a browser, any token the CLI refreshes is written to a filesystem that is discarded on exit, and so the copy in the secret store only ever ages. When it finally expires the server exits during startup and shows up as a server with no tools. The CLI has always accepted a long-lived NULLIFY_TOKEN, which outranks stored credentials and has no refresh cycle at all - but nothing in the cloud wiring resolved it, so putting one in the secret store had no effect. Now: - .mcp.json requests NULLIFY_TOKEN and NULLIFY_CREDENTIALS_JSON as alternatives, and nullify-serve.sh uses whichever arrived, preferring the token and logging which model is live. - The session-start hook resolves NULLIFY_TOKEN first and only seeds the credentials file when no token is configured, rather than skipping the var deliberately as before. - with-op-env.sh gained optional vars, marked with a "?" prefix. They are still resolved, but their absence is not reported - without this, offering two alternative credentials guarantees a spurious warning about whichever one is unused. - preflight names the credential model in use, so a failure points at the secret that actually needs attention. Verified end to end: with NULLIFY_TOKEN set the server completes a handshake and lists 27 tools instead of dying on the refresh. Also fixes a latent bug that may be the cause of the current outage. refreshToken() re-saved the same refresh token and never looked for a new one, even though the same handler already falls back to a Set-Cookie for the access token. Against a provider that rotates refresh tokens on use, the replacement is dropped and the next refresh fails with a 401 - which is exactly the symptom seen here. A rotated token is now read from the JSON body (either spelling) or a Set-Cookie and persisted; absent means non-rotating, so the existing token carries forward. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Mcq6LfAVPzKZL5rpvww13R --- .claude/hooks/session-start.sh | 14 ++- .claude/mcp/nullify-serve.sh | 50 +++++++--- .claude/mcp/preflight.sh | 20 +++- .claude/mcp/with-op-env.sh | 13 ++- .mcp.json | 2 +- README.md | 22 +++++ internal/auth/login.go | 29 +++++- internal/auth/refresh_rotation_test.go | 126 +++++++++++++++++++++++++ 8 files changed, 251 insertions(+), 25 deletions(-) create mode 100644 internal/auth/refresh_rotation_test.go diff --git a/.claude/hooks/session-start.sh b/.claude/hooks/session-start.sh index 9bfd216..d6a5a92 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" diff --git a/.claude/mcp/nullify-serve.sh b/.claude/mcp/nullify-serve.sh index 19db99d..3646afc 100755 --- a/.claude/mcp/nullify-serve.sh +++ b/.claude/mcp/nullify-serve.sh @@ -1,21 +1,45 @@ #!/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 - mkdir -p "$HOME/.nullify" - chmod 700 "$HOME/.nullify" - printf '%s' "$NULLIFY_CREDENTIALS_JSON" > "$HOME/.nullify/credentials.json" - chmod 600 "$HOME/.nullify/credentials.json" +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 diff --git a/.claude/mcp/preflight.sh b/.claude/mcp/preflight.sh index b275ff8..876053e 100755 --- a/.claude/mcp/preflight.sh +++ b/.claude/mcp/preflight.sh @@ -74,12 +74,24 @@ check_nullify() { return fi + # Name the credential model in use, so a failure points at the right secret + # instead of leaving someone to guess which of the two is live. + local mode + if [ -n "${NULLIFY_TOKEN:-}" ]; then + mode="NULLIFY_TOKEN" + elif [ -s "$HOME/.nullify/credentials.json" ]; then + mode="seeded credentials" + else + fail "$name" "no credentials at all - set NULLIFY_TOKEN (preferred for cloud) or NULLIFY_CREDENTIALS_JSON" + return + fi + local out count out="$(NULLIFY_HOST="${NULLIFY_HOST:-scrut.nullify.ai}" mcp_exchange "" "$TMP_BIN" mcp serve)" count="$(printf '%s\n' "$out" | json_lines | tool_count)" if [ "${count:-0}" -gt 0 ]; then - pass "$name" "$count tools" + pass "$name" "$count tools, via $mode" return fi @@ -87,11 +99,11 @@ check_nullify() { reason="$(stderr_reason)" case "$reason" in *"refresh failed"*|*"token expired"*|*"auth login"*) - fail "$name" "credentials rejected - re-seed NULLIFY_CREDENTIALS_JSON. ($reason)" ;; + fail "$name" "$mode rejected - a seeded refresh token cannot be renewed from a container; set NULLIFY_TOKEN instead. ($reason)" ;; "") - fail "$name" "exposed no tools and gave no error" ;; + fail "$name" "exposed no tools and gave no error (using $mode)" ;; *) - fail "$name" "$reason" ;; + fail "$name" "$reason (using $mode)" ;; esac } diff --git a/.claude/mcp/with-op-env.sh b/.claude/mcp/with-op-env.sh index 35e13a3..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 @@ -59,7 +68,7 @@ fi # 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 "${vars[@]}"; do +for var in "${required[@]}"; do [ -z "${!var:-}" ] && still_missing+=("$var") done 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/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/internal/auth/login.go b/internal/auth/login.go index 0d06b5d..06ddaa1 100644 --- a/internal/auth/login.go +++ b/internal/auth/login.go @@ -287,6 +287,8 @@ func refreshToken(ctx context.Context, host string, refreshTok string) (string, var result struct { AccessToken string `json:"accessToken"` + RefreshToken string `json:"refreshToken"` + RefreshTokenAlt string `json:"refresh_token"` ExpiresIn int `json:"expiresIn"` QueryParameters map[string]string `json:"queryParameters"` } @@ -314,11 +316,36 @@ func refreshToken(ctx context.Context, host string, refreshTok string) (string, return "", fmt.Errorf("refresh returned empty access token") } + // Keep a rotated refresh token if the server issued one. Providers that + // rotate on use invalidate the token we just sent, so re-saving the old + // one guarantees the next refresh fails with a 401 - and on an ephemeral + // container that failure is permanent, because the seeded copy in the + // secret store is never updated. Delivery mirrors the access token: JSON + // body first, Set-Cookie as the fallback. Absent means non-rotating, so + // carry the existing token forward. + rotated := result.RefreshToken + if rotated == "" { + rotated = result.RefreshTokenAlt + } + if rotated == "" { + for _, c := range resp.Cookies() { + if c.Name == "refresh_token" && c.Value != "" { + rotated = c.Value + break + } + } + } + if rotated == "" { + rotated = refreshTok + } else if rotated != refreshTok { + logger.L(ctx).Debug("server rotated the refresh token; persisting the new one") + } + expiresAt := time.Now().Add(time.Duration(result.ExpiresIn) * time.Second).Unix() err = SaveHostCredentials(host, HostCredentials{ AccessToken: result.AccessToken, - RefreshToken: refreshTok, + RefreshToken: rotated, ExpiresAt: expiresAt, QueryParameters: result.QueryParameters, }) diff --git a/internal/auth/refresh_rotation_test.go b/internal/auth/refresh_rotation_test.go new file mode 100644 index 0000000..ef20d1c --- /dev/null +++ b/internal/auth/refresh_rotation_test.go @@ -0,0 +1,126 @@ +package auth + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// refreshAgainst points the refresh flow at a test server. The production code +// builds "https://api./auth/refresh_token", so the host is derived from +// the test server's address and the transport is redirected to it. +func refreshAgainst(t *testing.T, handler http.HandlerFunc) func(refreshTok string) (string, error) { + t.Helper() + + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + + target := strings.TrimPrefix(srv.URL, "http://") + + originalClient := httpClient + httpClient = &http.Client{ + Timeout: 10 * time.Second, + Transport: rewriteTransport{target: target}, + } + t.Cleanup(func() { httpClient = originalClient }) + + return func(refreshTok string) (string, error) { + return refreshToken(context.Background(), "acme.nullify.ai", refreshTok) + } +} + +// rewriteTransport sends every request to the test server over plain HTTP, +// preserving the path so the handler still sees /auth/refresh_token. +type rewriteTransport struct{ target string } + +func (rt rewriteTransport) RoundTrip(req *http.Request) (*http.Response, error) { + req.URL.Scheme = "http" + req.URL.Host = rt.target + return http.DefaultTransport.RoundTrip(req) +} + +func storedRefreshToken(t *testing.T) string { + t.Helper() + + creds, err := LoadCredentials() + require.NoError(t, err) + return creds[CredentialKey("acme.nullify.ai")].RefreshToken +} + +func TestRefreshPersistsRotatedTokenFromJSONBody(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + refresh := refreshAgainst(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"accessToken":"new-access","refreshToken":"rotated-abc","expiresIn":3600}`)) + }) + + token, err := refresh("original-xyz") + require.NoError(t, err) + require.Equal(t, "new-access", token) + require.Equal(t, "rotated-abc", storedRefreshToken(t), + "a rotated refresh token must replace the one we sent") +} + +func TestRefreshPersistsRotatedTokenFromSnakeCaseBody(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + refresh := refreshAgainst(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"accessToken":"new-access","refresh_token":"rotated-snake","expiresIn":3600}`)) + }) + + _, err := refresh("original-xyz") + require.NoError(t, err) + require.Equal(t, "rotated-snake", storedRefreshToken(t)) +} + +// The access token already arrives via Set-Cookie on this endpoint, so a +// rotated refresh token plausibly arrives the same way. +func TestRefreshPersistsRotatedTokenFromCookie(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + refresh := refreshAgainst(t, func(w http.ResponseWriter, r *http.Request) { + http.SetCookie(w, &http.Cookie{Name: "access_token", Value: "cookie-access", MaxAge: 3600}) + http.SetCookie(w, &http.Cookie{Name: "refresh_token", Value: "rotated-cookie", MaxAge: 86400}) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + }) + + token, err := refresh("original-xyz") + require.NoError(t, err) + require.Equal(t, "cookie-access", token) + require.Equal(t, "rotated-cookie", storedRefreshToken(t)) +} + +// Non-rotating providers send nothing back; the existing token must survive. +func TestRefreshKeepsExistingTokenWhenServerDoesNotRotate(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + refresh := refreshAgainst(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"accessToken":"new-access","expiresIn":3600}`)) + }) + + _, err := refresh("original-xyz") + require.NoError(t, err) + require.Equal(t, "original-xyz", storedRefreshToken(t), + "absent rotation must carry the existing refresh token forward") +} + +func TestRefreshSurfacesNonOKStatus(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + refresh := refreshAgainst(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + }) + + _, err := refresh("expired-token") + require.Error(t, err) + require.Contains(t, err.Error(), "401") +} From bc9dc1f9772e917d69b29b9208145adfeb6bbd35 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 01:20:19 +0000 Subject: [PATCH 4/7] fix: probe latent-defense on a data endpoint, and validate NULLIFY_TOKEN Two corrections, both cases where the preflight confidently reported the wrong thing. latent-defense was never broken. /auth/me serves device-flow sessions only and answers 401 to a perfectly valid API key, so checking it produced a false failure - and the no-key control returned the same 401, which made the wrong conclusion look confirmed. The whoami tool is worse: it calls /auth/me, reads the 401 as unauthenticated, then blocks on the interactive device flow. /api/infra/records discriminates honestly - 200 with a valid key, 401 with a missing or bogus one - and the server reports 130 tools against a graph of 31,111 nodes and 21 attack paths. NULLIFY_TOKEN now gets validated before use. A JWT carries its own verdict, so read it rather than waiting for the API to answer 403 on every call. Two mistakes are worth naming explicitly: an expired token, and the "access_token" field copied out of a credentials.json, which on this provider is an ID token (token_use=id) that authenticates the process but is rejected for API calls. Opaque non-JWT tokens are left alone. Covered: expired ID token, in-window ID token, valid access token, and an opaque token that must not trip either guard. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Mcq6LfAVPzKZL5rpvww13R --- .claude/mcp/preflight.sh | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/.claude/mcp/preflight.sh b/.claude/mcp/preflight.sh index 876053e..180e988 100755 --- a/.claude/mcp/preflight.sh +++ b/.claude/mcp/preflight.sh @@ -79,6 +79,27 @@ check_nullify() { local mode if [ -n "${NULLIFY_TOKEN:-}" ]; then mode="NULLIFY_TOKEN" + + # A JWT pasted here carries its own verdict, so read it rather than + # waiting for the API to answer 403. The common mistake is copying the + # "access_token" out of a credentials.json, which on this provider is an + # *ID* token that expires in an hour - it authenticates the process but is + # rejected for API calls, which surfaces as a puzzling 403 on every tool. + local claims exp token_use + claims="$(printf '%s' "$NULLIFY_TOKEN" | jq -R 'split(".") | if length == 3 then .[1] | @base64d | fromjson else empty end' 2>/dev/null)" + if [ -n "$claims" ]; then + exp="$(printf '%s' "$claims" | jq -r '.exp // empty' 2>/dev/null)" + token_use="$(printf '%s' "$claims" | jq -r '.token_use // empty' 2>/dev/null)" + + if [ -n "$exp" ] && [ "$exp" -lt "$(date +%s)" ] 2>/dev/null; then + fail "$name" "NULLIFY_TOKEN expired on $(date -u -d "@$exp" '+%Y-%m-%d %H:%M UTC' 2>/dev/null) - it needs a token that does not expire, not one copied from a login session" + return + fi + if [ "$token_use" = "id" ]; then + fail "$name" "NULLIFY_TOKEN is an ID token (token_use=id), which the API rejects - use an API token, not the access_token field of a credentials.json" + return + fi + fi elif [ -s "$HOME/.nullify/credentials.json" ]; then mode="seeded credentials" else @@ -128,10 +149,15 @@ check_latent_defense() { return fi - # Probe the portal directly rather than calling the server's whoami tool. - # whoami falls back to the interactive device flow on a 401 and blocks until - # the timeout, which turns a definite "key rejected" into an inconclusive - # hang. One HTTP call answers the same question in under a second. + # Probe a real data endpoint, not /auth/me and not the whoami tool. + # + # /auth/me serves device-flow sessions only: it answers 401 to a perfectly + # good API key, so testing it reports a false failure. whoami is worse - it + # calls /auth/me, reads the 401 as unauthenticated, and then blocks on the + # interactive device flow until the timeout. + # + # /api/infra/records is the honest check: it discriminates cleanly, with a + # valid key returning 200 while a missing or bogus key returns 401. if [ -z "${LATENT_DEFENSE_URL:-}" ]; then warn "$name" "$count tools, but LATENT_DEFENSE_URL is unset - cannot verify authorization" return @@ -145,7 +171,7 @@ check_latent_defense() { code="$(curl -sS -o /dev/null -w '%{http_code}' -m 25 \ -H "Authorization: Bearer ${LATENT_DEFENSE_API_KEY}" \ -H "Accept: application/json" \ - "${LATENT_DEFENSE_URL%/}/auth/me" 2>/dev/null)" + "${LATENT_DEFENSE_URL%/}/api/infra/records" 2>/dev/null)" case "$code" in 200) From 1be8e292e7b6fada8c63c7b9c48eecc0f9c1bbf0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 01:27:31 +0000 Subject: [PATCH 5/7] revert: drop speculative refresh-token rotation handling Upstream documents the actual contract: GET /auth/refresh_token returns no token in its JSON body at all (RefreshTokenOutput has no token field), and the access token arrives solely as a Set-Cookie. This provider does not rotate refresh tokens, so the rotation handling added earlier was guarding against a case that cannot occur, and it would conflict with adopting the upstream fix. The 401 was a refresh token reaching its own expiry. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Mcq6LfAVPzKZL5rpvww13R --- internal/auth/login.go | 29 +----- internal/auth/refresh_rotation_test.go | 126 ------------------------- 2 files changed, 1 insertion(+), 154 deletions(-) delete mode 100644 internal/auth/refresh_rotation_test.go diff --git a/internal/auth/login.go b/internal/auth/login.go index 06ddaa1..0d06b5d 100644 --- a/internal/auth/login.go +++ b/internal/auth/login.go @@ -287,8 +287,6 @@ func refreshToken(ctx context.Context, host string, refreshTok string) (string, var result struct { AccessToken string `json:"accessToken"` - RefreshToken string `json:"refreshToken"` - RefreshTokenAlt string `json:"refresh_token"` ExpiresIn int `json:"expiresIn"` QueryParameters map[string]string `json:"queryParameters"` } @@ -316,36 +314,11 @@ func refreshToken(ctx context.Context, host string, refreshTok string) (string, return "", fmt.Errorf("refresh returned empty access token") } - // Keep a rotated refresh token if the server issued one. Providers that - // rotate on use invalidate the token we just sent, so re-saving the old - // one guarantees the next refresh fails with a 401 - and on an ephemeral - // container that failure is permanent, because the seeded copy in the - // secret store is never updated. Delivery mirrors the access token: JSON - // body first, Set-Cookie as the fallback. Absent means non-rotating, so - // carry the existing token forward. - rotated := result.RefreshToken - if rotated == "" { - rotated = result.RefreshTokenAlt - } - if rotated == "" { - for _, c := range resp.Cookies() { - if c.Name == "refresh_token" && c.Value != "" { - rotated = c.Value - break - } - } - } - if rotated == "" { - rotated = refreshTok - } else if rotated != refreshTok { - logger.L(ctx).Debug("server rotated the refresh token; persisting the new one") - } - expiresAt := time.Now().Add(time.Duration(result.ExpiresIn) * time.Second).Unix() err = SaveHostCredentials(host, HostCredentials{ AccessToken: result.AccessToken, - RefreshToken: rotated, + RefreshToken: refreshTok, ExpiresAt: expiresAt, QueryParameters: result.QueryParameters, }) diff --git a/internal/auth/refresh_rotation_test.go b/internal/auth/refresh_rotation_test.go deleted file mode 100644 index ef20d1c..0000000 --- a/internal/auth/refresh_rotation_test.go +++ /dev/null @@ -1,126 +0,0 @@ -package auth - -import ( - "context" - "net/http" - "net/http/httptest" - "strings" - "testing" - "time" - - "github.com/stretchr/testify/require" -) - -// refreshAgainst points the refresh flow at a test server. The production code -// builds "https://api./auth/refresh_token", so the host is derived from -// the test server's address and the transport is redirected to it. -func refreshAgainst(t *testing.T, handler http.HandlerFunc) func(refreshTok string) (string, error) { - t.Helper() - - srv := httptest.NewServer(handler) - t.Cleanup(srv.Close) - - target := strings.TrimPrefix(srv.URL, "http://") - - originalClient := httpClient - httpClient = &http.Client{ - Timeout: 10 * time.Second, - Transport: rewriteTransport{target: target}, - } - t.Cleanup(func() { httpClient = originalClient }) - - return func(refreshTok string) (string, error) { - return refreshToken(context.Background(), "acme.nullify.ai", refreshTok) - } -} - -// rewriteTransport sends every request to the test server over plain HTTP, -// preserving the path so the handler still sees /auth/refresh_token. -type rewriteTransport struct{ target string } - -func (rt rewriteTransport) RoundTrip(req *http.Request) (*http.Response, error) { - req.URL.Scheme = "http" - req.URL.Host = rt.target - return http.DefaultTransport.RoundTrip(req) -} - -func storedRefreshToken(t *testing.T) string { - t.Helper() - - creds, err := LoadCredentials() - require.NoError(t, err) - return creds[CredentialKey("acme.nullify.ai")].RefreshToken -} - -func TestRefreshPersistsRotatedTokenFromJSONBody(t *testing.T) { - t.Setenv("HOME", t.TempDir()) - - refresh := refreshAgainst(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"accessToken":"new-access","refreshToken":"rotated-abc","expiresIn":3600}`)) - }) - - token, err := refresh("original-xyz") - require.NoError(t, err) - require.Equal(t, "new-access", token) - require.Equal(t, "rotated-abc", storedRefreshToken(t), - "a rotated refresh token must replace the one we sent") -} - -func TestRefreshPersistsRotatedTokenFromSnakeCaseBody(t *testing.T) { - t.Setenv("HOME", t.TempDir()) - - refresh := refreshAgainst(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"accessToken":"new-access","refresh_token":"rotated-snake","expiresIn":3600}`)) - }) - - _, err := refresh("original-xyz") - require.NoError(t, err) - require.Equal(t, "rotated-snake", storedRefreshToken(t)) -} - -// The access token already arrives via Set-Cookie on this endpoint, so a -// rotated refresh token plausibly arrives the same way. -func TestRefreshPersistsRotatedTokenFromCookie(t *testing.T) { - t.Setenv("HOME", t.TempDir()) - - refresh := refreshAgainst(t, func(w http.ResponseWriter, r *http.Request) { - http.SetCookie(w, &http.Cookie{Name: "access_token", Value: "cookie-access", MaxAge: 3600}) - http.SetCookie(w, &http.Cookie{Name: "refresh_token", Value: "rotated-cookie", MaxAge: 86400}) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{}`)) - }) - - token, err := refresh("original-xyz") - require.NoError(t, err) - require.Equal(t, "cookie-access", token) - require.Equal(t, "rotated-cookie", storedRefreshToken(t)) -} - -// Non-rotating providers send nothing back; the existing token must survive. -func TestRefreshKeepsExistingTokenWhenServerDoesNotRotate(t *testing.T) { - t.Setenv("HOME", t.TempDir()) - - refresh := refreshAgainst(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"accessToken":"new-access","expiresIn":3600}`)) - }) - - _, err := refresh("original-xyz") - require.NoError(t, err) - require.Equal(t, "original-xyz", storedRefreshToken(t), - "absent rotation must carry the existing refresh token forward") -} - -func TestRefreshSurfacesNonOKStatus(t *testing.T) { - t.Setenv("HOME", t.TempDir()) - - refresh := refreshAgainst(t, func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusUnauthorized) - }) - - _, err := refresh("expired-token") - require.Error(t, err) - require.Contains(t, err.Error(), "401") -} From 186ffc05e14f7226af91f9bbad1df35ef81f9b3c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 01:32:12 +0000 Subject: [PATCH 6/7] feat: verify MCP servers are authorized, not merely running An MCP server almost never fails loudly. It either dies during startup and appears 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 read as "nothing to report" rather than "outage", and the second is worse: work gets published against data that was never there. Preflight now takes its server list from .mcp.json rather than a list kept by hand, so a newly added server cannot quietly become a blind spot - it is reported WARN "no probe defined" until someone gives it one. Each server is started through its real .mcp.json command, so the wrapper and argument wiring are under test too, not just the binary. Authorization probes move into .claude/mcp/health-probes.json, with three kinds: an HTTP call against a data endpoint, JWT inspection for expiry and token type, and a presence check for servers with no cheap authenticated endpoint. The file documents why latent-defense is probed on /api/infra/records and explicitly not on /auth/me, which serves login sessions and answers 401 to a valid API key - the cause of a six-day false outage. Results are written to .claude/mcp/health.json (gitignored, per-session) so a later session can read coverage instead of assuming it, and the session-start hook now prints an unmissable banner when anything is unusable. CLAUDE.md tells whoever comes next to read that file before reporting on security data, and that an empty result from a failing server means absent data, never "nothing found" - the loop that failed the first time round. Also adds `make preflight`, and records upstream's refresh-token contract and the one fix (#165) this fork still lacks. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Mcq6LfAVPzKZL5rpvww13R --- .claude/hooks/session-start.sh | 8 +- .claude/mcp/health-probes.json | 51 +++++ .claude/mcp/preflight.sh | 347 ++++++++++++++++----------------- .gitignore | 1 + CLAUDE.md | 82 ++++++++ Makefile | 4 + 6 files changed, 314 insertions(+), 179 deletions(-) create mode 100644 .claude/mcp/health-probes.json create mode 100644 CLAUDE.md diff --git a/.claude/hooks/session-start.sh b/.claude/hooks/session-start.sh index d6a5a92..28cb205 100755 --- a/.claude/hooks/session-start.sh +++ b/.claude/hooks/session-start.sh @@ -111,6 +111,12 @@ fi # 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 ---" -.claude/mcp/preflight.sh || true +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/preflight.sh b/.claude/mcp/preflight.sh index 180e988..bc2ceb4 100755 --- a/.claude/mcp/preflight.sh +++ b/.claude/mcp/preflight.sh @@ -1,230 +1,221 @@ #!/bin/bash -# Probe every MCP server this repo wires up and report whether it is actually -# usable. Run it by hand any time tools look missing: +# 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 +# .claude/mcp/preflight.sh human table + health.json +# make preflight same # -# Two failure modes matter, and counting tools only catches the first: +# 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. # -# 1. The server dies during startup - almost always expired credentials. -# It exposes no tools at all and the client just shows nothing. -# 2. The server starts and lists its whole tool set, but the credentials are -# rejected on every call, so each tool answers "not authenticated" or -# returns an empty result. +# So each server gets two checks: +# 1. handshake does it start and list tools? +# 2. authorization is its credential actually accepted? # -# The second mode is the dangerous one: coverage looks complete while the data -# is silently absent. So each server gets a handshake AND, where the server -# offers one, an authorization probe. +# 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 is 0 only when every server passes, so this can gate a check. +# 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)" -TMP_BIN="$(mktemp -u)" -trap 'rm -f "$STDERR_FILE" "$TMP_BIN"' EXIT +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 REPORT=() +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) -# Emit the JSON-RPC lines for a handshake, optionally followed by one extra -# request (used for authorization probes). -mcp_exchange() { - local extra="$1"; shift { 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":{}}' - [ -n "$extra" ] && echo "$extra" - # Give the server a moment to answer before stdin closes. sleep 2 - } | timeout "$TIMEOUT" "$@" 2>"$STDERR_FILE" + } | 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 } -# Keep only well-formed JSON-RPC lines; servers routinely print banners. -json_lines() { jq -R 'fromjson? // empty' 2>/dev/null; } - -tool_count() { - jq -s 'map(select(.id == 2)) | (.[0].result.tools // []) | length' 2>/dev/null -} - -# Last non-empty stderr line, trimmed - usually the actual cause. +# 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-160 + | tail -1 | cut -c1-150 } -record() { REPORT+=("$1|$2|$3"); } - -pass() { record "PASS" "$1" "$2"; } -fail() { record "FAIL" "$1" "$2"; failures=$((failures + 1)); } -warn() { record "WARN" "$1" "$2"; } - -# ------------------------------------------------------------------ -# nullify - dies at startup when the seeded refresh token is rejected -# ------------------------------------------------------------------ -check_nullify() { - local name="nullify" - - if ! go build -o "$TMP_BIN" ./cmd/cli 2>"$STDERR_FILE"; then - fail "$name" "CLI failed to build: $(stderr_reason)" - return - fi +# ------------------------------------------------------------------- probes - # Name the credential model in use, so a failure points at the right secret - # instead of leaving someone to guess which of the two is live. - local mode - if [ -n "${NULLIFY_TOKEN:-}" ]; then - mode="NULLIFY_TOKEN" - - # A JWT pasted here carries its own verdict, so read it rather than - # waiting for the API to answer 403. The common mistake is copying the - # "access_token" out of a credentials.json, which on this provider is an - # *ID* token that expires in an hour - it authenticates the process but is - # rejected for API calls, which surfaces as a puzzling 403 on every tool. - local claims exp token_use - claims="$(printf '%s' "$NULLIFY_TOKEN" | jq -R 'split(".") | if length == 3 then .[1] | @base64d | fromjson else empty end' 2>/dev/null)" - if [ -n "$claims" ]; then - exp="$(printf '%s' "$claims" | jq -r '.exp // empty' 2>/dev/null)" - token_use="$(printf '%s' "$claims" | jq -r '.token_use // empty' 2>/dev/null)" +probe_field() { + jq -r --arg s "$1" --arg f "$2" '.probes[$s][$f] // empty' "$PROBE_CONFIG" 2>/dev/null +} - if [ -n "$exp" ] && [ "$exp" -lt "$(date +%s)" ] 2>/dev/null; then - fail "$name" "NULLIFY_TOKEN expired on $(date -u -d "@$exp" '+%Y-%m-%d %H:%M UTC' 2>/dev/null) - it needs a token that does not expire, not one copied from a login session" +# 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 - if [ "$token_use" = "id" ]; then - fail "$name" "NULLIFY_TOKEN is an ID token (token_use=id), which the API rejects - use an API token, not the access_token field of a credentials.json" - return + 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 - fi - elif [ -s "$HOME/.nullify/credentials.json" ]; then - mode="seeded credentials" - else - fail "$name" "no credentials at all - set NULLIFY_TOKEN (preferred for cloud) or NULLIFY_CREDENTIALS_JSON" - return - fi - - local out count - out="$(NULLIFY_HOST="${NULLIFY_HOST:-scrut.nullify.ai}" mcp_exchange "" "$TMP_BIN" mcp serve)" - count="$(printf '%s\n' "$out" | json_lines | tool_count)" - - if [ "${count:-0}" -gt 0 ]; then - pass "$name" "$count tools, via $mode" - return - fi - - local reason - reason="$(stderr_reason)" - case "$reason" in - *"refresh failed"*|*"token expired"*|*"auth login"*) - fail "$name" "$mode rejected - a seeded refresh token cannot be renewed from a container; set NULLIFY_TOKEN instead. ($reason)" ;; - "") - fail "$name" "exposed no tools and gave no error (using $mode)" ;; - *) - fail "$name" "$reason (using $mode)" ;; - esac -} - -# ------------------------------------------------------------------ -# latent-defense - starts and lists every tool even when unauthorized, -# so the tool count proves nothing. whoami is the real check. -# ------------------------------------------------------------------ -check_latent_defense() { - local name="latent-defense" - - if ! command -v latent-defense-mcp >/dev/null 2>&1; then - fail "$name" "latent-defense-mcp not on PATH - 'uv tool install ./third_party/latent-defense-mcp'" - return - fi - - local out count - out="$(mcp_exchange "" latent-defense-mcp)" - count="$(printf '%s\n' "$out" | json_lines | tool_count)" - - if [ "${count:-0}" -eq 0 ]; then - fail "$name" "exposed no tools. $(stderr_reason)" - return - fi - - # Probe a real data endpoint, not /auth/me and not the whoami tool. - # - # /auth/me serves device-flow sessions only: it answers 401 to a perfectly - # good API key, so testing it reports a false failure. whoami is worse - it - # calls /auth/me, reads the 401 as unauthenticated, and then blocks on the - # interactive device flow until the timeout. - # - # /api/infra/records is the honest check: it discriminates cleanly, with a - # valid key returning 200 while a missing or bogus key returns 401. - if [ -z "${LATENT_DEFENSE_URL:-}" ]; then - warn "$name" "$count tools, but LATENT_DEFENSE_URL is unset - cannot verify authorization" - return - fi - if [ -z "${LATENT_DEFENSE_API_KEY:-}" ]; then - fail "$name" "$count tools but LATENT_DEFENSE_API_KEY is unset - every call will be unauthorized" - return - fi - - local code - code="$(curl -sS -o /dev/null -w '%{http_code}' -m 25 \ - -H "Authorization: Bearer ${LATENT_DEFENSE_API_KEY}" \ - -H "Accept: application/json" \ - "${LATENT_DEFENSE_URL%/}/api/infra/records" 2>/dev/null)" - - case "$code" in - 200) - pass "$name" "$count tools, authenticated" ;; - 401|403) - fail "$name" "$count tools but key REJECTED (HTTP $code) - rotate LATENT_DEFENSE_API_KEY; tools return empty results, not errors" ;; - "") - warn "$name" "$count tools, but ${LATENT_DEFENSE_URL} was unreachable" ;; - *) - warn "$name" "$count tools, unexpected HTTP $code from /auth/me" ;; + 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 } -# ------------------------------------------------------------------ -# sentinelone - no unauthenticated health endpoint on the console, so -# the handshake is the honest check available here. -# ------------------------------------------------------------------ -check_sentinelone() { - local name="sentinelone" - local out count +# -------------------------------------------------------------------- main - out="$(mcp_exchange "" uvx --from ./third_party/purple-mcp purple-mcp --mode stdio)" - count="$(printf '%s\n' "$out" | json_lines | tool_count)" +echo "MCP preflight - $(jq -r '.mcpServers | keys | length' "$MCP_CONFIG") server(s) in $MCP_CONFIG" +echo - if [ "${count:-0}" -gt 0 ]; then - if [ -z "${PURPLEMCP_CONSOLE_TOKEN:-}" ]; then - warn "$name" "$count tools, but PURPLEMCP_CONSOLE_TOKEN is unset - calls will fail" +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 - pass "$name" "$count tools" + record "FAIL" "$server" "no tools and no error reported" fi - return + continue fi - fail "$name" "exposed no tools. $(stderr_reason)" -} - -echo "MCP preflight - probing servers from .mcp.json" -echo - -check_nullify -check_latent_defense -check_sentinelone + 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 "${REPORT[@]}"; do +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 "$failures server(s) unusable. Credential failures need a human: mint a new" - echo "secret, then update the 1Password item named by CLAUDE_OP_VAULT/CLAUDE_OP_ITEM." + 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 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/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..22a0531 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,82 @@ +# 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). As of this writing upstream carries a fix this fork does not have: +`fix(auth,client): cover token refresh with tests and repair the MCP 401 retry` +(#165), which adds `ForceRefreshToken`, refresh backoff, and +`ErrTokenNotRefreshable` for fixed token sources. Adopting it conflicts in +`internal/client/` because this fork diverged there, so it needs its own change. 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 From 38046dd09a2025b4237262360b1c48baf34f884b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 01:50:48 +0000 Subject: [PATCH 7/7] feat(client): recover from a 401 by forcing a token refresh Our transport had no 401 handling at all. It refreshed on a five-minute TTL and, when the provider failed, fell back to the stale cached token - so against a credential revoked mid-life every single request failed, forever, with no attempt to fetch a replacement. That is the shape of the outage we just spent a week diagnosing. This ports the behaviour of upstream #165 onto our simpler transport rather than adopting the file wholesale, because this fork branched at #138 and the upstream version depends on #151/#158 that we do not carry. - refreshProvider is a second, distinct provider used only after a 401. tokenProvider returns the stored token untouched while it still looks valid, so reusing it for the retry would replay the rejected string - the exact bug #165 was written to fix. - auth.ForceRefreshToken refreshes regardless of the recorded expiry, and reports ErrNotRefreshable when the credentials carry no refresh token. - lib.RefreshNullifyToken reports client.ErrTokenNotRefreshable for sources that yield a fixed string (--nullify-token, NULLIFY_TOKEN), so the transport surfaces the 401 instead of retrying something that cannot change. - Separate backoff clocks for the TTL and 401 paths: one provider failing says nothing about the other, and without them a rejected credential puts a blocking refresh in front of every request. - A refresh that returns nothing, or returns the token just rejected, leaves the cache alone rather than overwriting a working value with junk. - Request bodies are buffered so a retried POST carries its payload. Tested: retry-and-succeed, fixed-source surfaces the 401, same-token and empty returns do not loop or clobber the cache, body replay, backoff suppression, concurrent rotation, non-401 passthrough, and provider-failure backoff. Upstream's own note on #165 was that this code went broken from February to August precisely because it had no tests. CLAUDE.md records what remains unported, including #159 - the GitHub PAT is still sent in a URL query string. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Mcq6LfAVPzKZL5rpvww13R --- CLAUDE.md | 22 ++- cmd/cli/cmd/mcp.go | 10 +- internal/auth/force_refresh_test.go | 62 +++++++ internal/auth/login.go | 27 +++ internal/client/refreshing_transport.go | 145 ++++++++++++++-- internal/client/refreshing_transport_test.go | 174 +++++++++++++++++++ internal/lib/get_token.go | 42 ++++- internal/lib/refresh_token_test.go | 52 ++++++ 8 files changed, 509 insertions(+), 25 deletions(-) create mode 100644 internal/auth/force_refresh_test.go create mode 100644 internal/client/refreshing_transport_test.go create mode 100644 internal/lib/refresh_token_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 22a0531..5910136 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -75,8 +75,20 @@ 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). As of this writing upstream carries a fix this fork does not have: -`fix(auth,client): cover token refresh with tests and repair the MCP 401 retry` -(#165), which adds `ForceRefreshToken`, refresh backoff, and -`ErrTokenNotRefreshable` for fixed token sources. Adopting it conflicts in -`internal/client/` because this fork diverged there, so it needs its own change. +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/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) +}