Repository navigation
Refuse credentials from a newer CLI, and stop refreshing a refused client - #959
Conversation
…ient A credential whose oauth_type this version doesn't know was refreshed as Launchpad: an older CLI reading a newer login sent Launchpad's client to that login's token endpoint, which answered invalid_client on every command, indefinitely. Only "" and "launchpad" take the Launchpad path now; any other unknown type is refused before anything is sent, with the upgrade as the remedy. invalid_client on a refresh is terminal: asking again as the same client cannot succeed, and each ask is charged to the abuse tracker. The refusal is held on the stored login with no recheck, for that refresh token and client, so later commands answer locally and say to log in again. A login or a changed BASECAMP_OAUTH_CLIENT_ID/SECRET is not held to it.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approved
The focused refresh safeguards have targeted regression coverage, and no blocking issues were identified.
No open issues 🎉
What changed in this PR
This PR hardens OAuth refresh handling so unsupported credentials are preserved and a refused OAuth client does not trigger repeated refresh requests.
Changes:
- Reject unknown OAuth types before sending requests, with guidance to upgrade or log in again.
- Persist permanent
invalid_clientholds tied to the refresh token and client, allowing retries when either changes. - Add regression tests and update existing hold callers to pass an explicit credential subject.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| internal/auth/renewal_hold.go | Adds client-specific refresh refusal holds and explicit persistence subjects. |
| internal/auth/renewal_hold_test.go | Updates tests for the hold persistence signature. |
| internal/auth/refresh_client_test.go | Tests unknown types, permanent refusals, client changes, and transient failures. |
| internal/auth/auth.go | Rejects unknown types and handles typed invalid_client refusals. |
| internal/auth/agent.go | Adapts agent hold persistence without changing its behavior. |
Review effort: Balanced
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89dc7bedc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A newer type need not renew by refresh token, and one without was reported as "No refresh token available" with a login as the remedy, rather than as a newer credential to upgrade for.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bffbd21a6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…emedy A refused refresh said it would not be sent again even when the hold that keeps that promise could not be stored. And auth status told a login held for invalid_client to log in with a new secret, the remedy for an agent's refused secret, rather than to log in again.
|
Review threads: 3 resolved (3 fixed, 0 declined). |
Bug
prepareRefresh'sdefault:branch treated anyoauth_typeit 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 answersinvalid_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_clientrefusal stops the refreshes instead of repeating them.Fix
""(migrated as before) andlaunchpadtake the Launchpad path.bc5and legacybc3behave 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 hintRun: 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_clienton 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 permanentrefusedhold (Until: 0) on the stored login, using the existingRenewalHoldmachinery that agent mints already use forinvalid_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 correctedBASECAMP_OAUTH_CLIENT_ID/SECRET(different client); either of those sends the refresh again.rememberRenewalHoldnow takes the subject the stored credential must still match, because the hold's own fingerprint is no longer alwaysrenewalSubject. Existing callers passhold.Client, so their behavior is unchanged.Tests
internal/auth/refresh_client_test.go:RefreshRefusalagrees.invalid_clientis terminal for bothbc5andlaunchpad: one request, then repeatedAccessTokencalls 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.BASECAMP_OAUTH_CLIENT_ID/SECRETchange, the refresh goes out again.invalid_clientholds nothing and is retried.invalidClientreads 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/cipasses 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_typeas Launchpad, sending Launchpad's client to the wrong token endpoint and drawinginvalid_clienton every command. It also stops retrying a refresh that the server refused asinvalid_client.basecamp upgradeor logging in again, and the credential is left untouched for the newer CLI.invalid_clienton 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.BASECAMP_OAUTH_CLIENT_ID/SECRETsends the refresh again.auth statusnames "log in again" as the remedy for a login held this way, rather than "log in with a new secret".Written for commit 9219f45. Summary will update on new commits.