Skip to content

fix: KEEP-1049 validate credentials against an endpoint that requires auth - #75

Merged
suisuss merged 2 commits into
mainfrom
fix/KEEP-1049-validate-credentials-against-auth-required-endpoint
Jul 28, 2026
Merged

fix: KEEP-1049 validate credentials against an endpoint that requires auth#75
suisuss merged 2 commits into
mainfrom
fix/KEEP-1049-validate-credentials-against-auth-required-endpoint

Conversation

@suisuss

@suisuss suisuss commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

kh doctor reported every caller as authenticated, including a caller with no
credential stored at all, and kh auth status accepted any string beginning with
kh_. Both checks probed endpoints that answer 200 to anonymous callers, so
neither could ever fail.

# before, with no token in hosts.yml
[pass] Auth: authenticated
[warn] Wallet: requires authentication
[warn] Spend Cap: requires authentication

Note the self-contradiction: Auth passes while the checks that work report the
caller as unauthenticated.

Two layers

1. The endpoints could not fail.

  • checkAuth probed /api/auth/get-session, which returns a null session with
    200
    rather than 401.
  • fetchAPIKeyInfo validated against /api/workflows, which resolves auth with
    required: false and returns an empty list to anonymous callers.

Confirmed against a dev server - the same fabricated key gets 401 from
/api/projects
and 200 from /api/workflows.

Both now resolve their endpoint from one shared constant, CredentialProbePath,
pointing at /api/projects, which authenticates through resolveOrganizationId
(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. doGet applied Cloudflare Access
headers but never set Authorization. That is also why wallet and spend-cap
reported "requires authentication" even for a valid key - they were genuinely
anonymous. doGet now attaches the resolved token, and those checks report real
state.

Why this matters beyond diagnostics

kh auth login decides whether to skip using FetchTokenInfo. Because validation
was vacuous for kh_ keys, a revoked key counted as valid: the skip fired and
the user had to discover --force to recover. This repairs behaviour already
merged 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/workflows failed 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.go therefore pins the constant itself against the known
anonymous-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-Version warning now names the remedy:

warning: your CLI version (0.3.0) is outdated; minimum required is 0.11.1. Run: kh update

Updates are manual and unprompted, so this line is the only signal an out-of-date
CLI gets. Not included: having checkCLIVersion compare against the server's
advertised floor, which needs semverLessThan exported and the header captured
during a check. Left to a follow-up.

Test plan

Verified live against a dev server:

  • Valid key: doctor [pass] authenticated; auth status resolves
  • Bogus key: doctor [warn] not authenticated; auth status reports
    "API key is invalid or revoked"
  • No credential: doctor [warn] not authenticated
  • Revoked key: kh auth login re-runs the device flow without --force
  • Wallet and spend-cap report real state rather than "requires authentication"
  • Full suite green (28 packages), vet and gofmt clean on all touched files
  • Negative control: reverting the constant fails the pinning test

suisuss added 2 commits July 28, 2026 11:57
… 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
suisuss merged commit 72f6889 into main Jul 28, 2026
4 checks passed
@suisuss
suisuss deleted the fix/KEEP-1049-validate-credentials-against-auth-required-endpoint branch July 28, 2026 02:39
@eskp eskp mentioned this pull request Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant