Refuse credentials from a newer CLI, and stop refreshing a refused client

Merged
JE
1d ago
+404-20

Bug

prepareRefresh's default: branch treated any oauth_type it didn't recognize as Launchpad. When a CLI older than v0.8.0 reads a credential that a newer CLI wrote (bc5), it sends Launchpad's client id and secret to bc3's token endpoint. The endpoint answers invalid_client, and the old CLI sends the same refresh again on every later command. One real user on a stale binary is hitting this about 100 times a day.

Old binaries can't be fixed after the fact. This PR makes sure the same thing can't happen again between this version and whatever type a future CLI writes, and that an invalid_client refusal stops the refreshes instead of repeating them.

Fix

  • Unknown types are refused. Only "" (migrated as before) and launchpad take the Launchpad path. bc5 and legacy bc3 behave as before. Any other type gets an auth error before any request is sent and before the Launchpad token-endpoint migration can fill anything in. The error reads: These credentials were written by a newer basecamp CLI (OAuth type "…") and this version cannot refresh them, with the hint Run: basecamp upgrade (or, to keep this version, log in again with it: basecamp auth login …). The type is %q-quoted, so a damaged store can't put terminal controls into the message. The credential stays in the store untouched, for the newer CLI to use. RefreshRefusal (status and doctor) reports the same error.
  • invalid_client on a refresh is terminal. It is read from the SDK's typed refusal, only on 400 or 401 (RFC 6749 §5.2). A 429 is still handled as a rate-limit hold, and a 5xx that names the client is not a verdict on it. The refusal is stored as a permanent refused hold (Until: 0) on the stored login, using the existing RenewalHold machinery that agent mints already use for invalid_client. It is fingerprinted to the refresh token and the client it was sent as. The error tells the person to log in again. Later commands, in this process or any other, answer from the store and send nothing. The hold doesn't block a fresh login (new refresh token) or a corrected BASECAMP_OAUTH_CLIENT_ID/SECRET (different client); either of those sends the refresh again.
  • rememberRenewalHold now takes the subject the stored credential must still match, because the hold's own fingerprint is no longer always renewalSubject. Existing callers pass hold.Client, so their behavior is unchanged.

Tests

internal/auth/refresh_client_test.go:

  • An unknown type is refused with no network request, with and without a stored token endpoint. The message is quoted and sanitized, the hint names upgrade and login, the credential is left as written, and RefreshRefusal agrees.
  • invalid_client is terminal for both bc5 and launchpad: one request, then repeated AccessToken calls and a second Manager on the same store send nothing. The hold is permanent and reported as permanent, and a new refresh token isn't held to it.
  • The hold covers only the client that was refused: after BASECAMP_OAUTH_CLIENT_ID/SECRET change, the refresh goes out again.
  • A 503 naming invalid_client holds nothing and is retried.
  • invalidClient reads the typed code and status, not the message text.

The tests were checked against the old code: the unknown-type and terminal tests fail without the fix. bin/ci passes on Linux. Locally on macOS, the PTY-dependent interactivity tests (/dev/ptmx) fail the same way with or without this change.

Summary by cubic

Fixes refreshing credentials written by a newer CLI: old versions treated any unknown oauth_type as Launchpad, sending Launchpad's client to the wrong token endpoint and drawing invalid_client on every command. It also stops retrying a refresh that the server refused as invalid_client.

  • Unknown OAuth types are refused before anything else in the credential is examined, so even one without a refresh token is named as newer rather than reported as a missing token; the message points to basecamp upgrade or logging in again, and the credential is left untouched for the newer CLI.
  • A refresh answered with invalid_client on a 400 or 401 is stored as a permanent hold, so later commands answer from the store and send nothing; the refusal promises no resend only when that hold was actually stored. 429s and 5xx keep their existing behavior.
  • The hold is fingerprinted to the refresh token plus the client it was sent as, so a fresh login or a corrected BASECAMP_OAUTH_CLIENT_ID/SECRET sends the refresh again.
  • auth status names "log in again" as the remedy for a login held this way, rather than "log in with a new secret".