fix: KEEP-1049 validate credentials against an endpoint that requires auth - #75
Merged
suisuss merged 2 commits intoJul 28, 2026
Conversation
… auth Doctor and API-key validation both probed endpoints that answer 200 to anonymous callers, so neither could fail. /api/auth/get-session returns a null session rather than 401, and /api/workflows resolves auth with required:false and returns an empty list. Doctor reported every caller as authenticated - including with no credential stored - and kh auth status accepted any string beginning with kh_. Both now resolve their endpoint from one shared constant pointing at /api/projects, which authenticates via resolveOrganizationId and answers 401 when nothing authenticates. A test pins the constant against the known anonymous-tolerant endpoints, since a test at either call site would just follow the constant wherever it points - which is how this went unnoticed. Doctor also never sent a credential at all: doGet applied Cloudflare Access headers but no Authorization. That is why wallet and spend-cap reported 'requires authentication' even for a valid key. It now attaches the resolved token, so those checks report real state. This repairs the login skip merged in #73. It decides whether a stored credential is still good via FetchTokenInfo, so a revoked key counted as valid and left the user needing --force to recover. Verified live: valid key passes, bogus key fails both checks, no credential fails, and login re-runs the device flow on a revoked key without --force.
…ndition Doctor fans its checks out across goroutines, so the httptest handler in the new probe-path test ran concurrently and wrote an unguarded map. It passed locally and failed under CI's go test -race. Guarded with a mutex. Also applies De Morgan to the login skip condition, which staticcheck QF1001 flags. That line came from #73 and is failing lint on main, not only on this branch.
suisuss
deleted the
fix/KEEP-1049-validate-credentials-against-auth-required-endpoint
branch
July 28, 2026 02:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
kh doctorreported every caller as authenticated, including a caller with nocredential stored at all, and
kh auth statusaccepted any string beginning withkh_. Both checks probed endpoints that answer 200 to anonymous callers, soneither could ever fail.
Note the self-contradiction: Auth passes while the checks that work report the
caller as unauthenticated.
Two layers
1. The endpoints could not fail.
checkAuthprobed/api/auth/get-session, which returns a null session with200 rather than 401.
fetchAPIKeyInfovalidated against/api/workflows, which resolves auth withrequired: falseand returns an empty list to anonymous callers.Confirmed against a dev server - the same fabricated key gets 401 from
/api/projectsand 200 from/api/workflows.Both now resolve their endpoint from one shared constant,
CredentialProbePath,pointing at
/api/projects, which authenticates throughresolveOrganizationId(OAuth, then API key, then session) and answers 401 when none of them
authenticates.
2. Doctor never sent a credential at all.
Found only by testing against a real server: fixing layer 1 alone made doctor
report "not authenticated" for a valid key.
doGetapplied Cloudflare Accessheaders but never set
Authorization. That is also why wallet and spend-capreported "requires authentication" even for a valid key - they were genuinely
anonymous.
doGetnow attaches the resolved token, and those checks report realstate.
Why this matters beyond diagnostics
kh auth logindecides whether to skip usingFetchTokenInfo. Because validationwas vacuous for
kh_keys, a revoked key counted as valid: the skip fired andthe user had to discover
--forceto recover. This repairs behaviour alreadymerged in #73, so it should ship in the same release.
A note on the tests
The negative control is worth repeating. Repointing the constant back at
/api/workflowsfailed the API-key test but the doctor tests still passed -they assert against the constant, so they follow it wherever it points. That is
structurally the same blindness that let the original defect through.
credential_probe_test.gotherefore pins the constant itself against the knownanonymous-tolerant endpoints (
/api/workflows,/api/auth/get-session,/api/openapi), so a regression fails regardless of which call site changes.Also included
The
KH-Minimum-CLI-Versionwarning now names the remedy:Updates are manual and unprompted, so this line is the only signal an out-of-date
CLI gets. Not included: having
checkCLIVersioncompare against the server'sadvertised floor, which needs
semverLessThanexported and the header capturedduring a check. Left to a follow-up.
Test plan
Verified live against a dev server:
[pass] authenticated;auth statusresolves[warn] not authenticated;auth statusreports"API key is invalid or revoked"
[warn] not authenticatedkh auth loginre-runs the device flow without--force