feat(auth): silent refresh helper + PKCE refreshToken via oauth4webapi#39
Merged
Conversation
PR3 of the refresh-token feature split. Login persists the full `TokenBundle` (refresh token + expiry) and `refreshAccessToken` rotates the access token proactively (skew window) or reactively (`force: true` after a 401), serialised via an `O_EXCL` file lock so concurrent CLI invocations don't issue parallel refresh grants. The PKCE provider implements `refreshToken` via `oauth4webapi`, declared as an optional peer dep — only refresh-capable consumers install it. `invalid_grant` (any status) maps to `AUTH_REFRESH_EXPIRED`; other failures to `AUTH_REFRESH_TRANSIENT`; missing peer / no refresh token / no `activeBundle` to `AUTH_REFRESH_UNAVAILABLE`. `KeyringTokenStore.activeBundle` parallel-reads both slots, honouring the `hasRefreshToken: false` record gate so access-only records skip the refresh-slot IPC. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The whole point of using oauth4webapi was to write less code — the initial PR3 commit didn't fully lean on that. Cuts: - `mapRefreshResponse` + `translateRefreshError` extracted as named functions called exactly once each. Inlined into `refreshToken`; the surrounding code reads top-to-bottom. - `loadOauth4webapi` was distinguishing `ERR_MODULE_NOT_FOUND` from other errors and surfacing the same `AUTH_REFRESH_UNAVAILABLE` code either way. Collapsed to one branch. - Verbose JSDoc on internal `acquireLock` / `translateRefreshError` restated what the code already says. Kept only the WHY-comments (re-read invariant on both acquire and timeout). - Three test-bloat cuts: provider 400-vs-401 + transient-variant it.eaches (oauth4webapi treats them identically — passes by construction); `refresh.test.ts#propagates AUTH_REFRESH_EXPIRED` (helper does no wrapping — `await` propagates by default); `internal.test.ts` redundant slot-read + slot-unavailable cases (covered by `activeBundle` integration tests in token-store). 412 tests still pass; net -128 LOC.
doistbot
reviewed
May 19, 2026
Member
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR successfully implements the silent refresh capability by introducing the refreshAccessToken helper, integrating oauth4webapi for PKCE token rotation, and expanding the token store contract to handle full credential bundles. These additions robustly round out the CLI core auth storage and refresh lifecycle, laying a solid foundation for uninterrupted user sessions. There are a few areas to refine before merging, primarily around strengthening lock file management against stale state and race conditions, deduplicating shared logic, enforcing network timeouts, and ensuring the README and test suite fully reflect the new capabilities and peer dependencies.
Two enablers so consumers that need async config resolution and a custom HTTP transport can use createPkceProvider (and inherit refreshToken) instead of hand-rolling a provider: - PkceLazyString resolvers may now return string | Promise<string>; resolve() is async and awaited at authorize / exchangeCode / refreshToken. Lets a consumer resolve base URL / client id asynchronously (config read, user prompt). - refreshToken threads an injected fetchImpl into oauth4webapi via its customFetch symbol, so a custom transport (proxy dispatcher, decompression) applies to the refresh grant rather than being bypassed by the library's global fetch — closing the global-fetch gap flagged in this PR's risk notes. Backwards compatible: a plain string or sync resolver still satisfies the widened type; customFetch is only set when fetchImpl is provided. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A non-invalid_grant ResponseBodyError was reported as AUTH_REFRESH_TRANSIENT with oauth4webapi's opaque "server responded with an error in the response body" — hiding the actual reason. Now both the EXPIRED and TRANSIENT paths include the server's `error` + `error_description`, e.g. "invalid_request (Missing client_secret for confidential client)", so a misconfigured OAuth client is diagnosable instead of a mystery. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Concurrency / robustness (refresh.ts):
- Forward a consumer `handshake` to provider.refreshToken instead of
hardcoding `{}`, so resolvers can see runtime context.
- Steal a stale lock (mtime older than the POST-bounded threshold) so a
crashed holder can't block every future refresh forever.
- Re-read the bundle after *any* lock acquire (not just the contended
path) and adopt a concurrent rotation, closing the
read-then-acquire race that could POST a stale refresh token.
- Release the lock if the under-lock re-read throws (no orphan).
- Require both activeBundle + setBundle as the refresh capability;
fail AUTH_REFRESH_UNAVAILABLE rather than silently dropping the
rotated bundle via a set() fallback.
Network (pkce.ts):
- Bound the refresh POST with an AbortSignal timeout so a hung token
endpoint can't block the CLI / hold the lock indefinitely.
Hot path (status.ts):
- Read activeBundle once for status when fetchLive is present, deriving
both token and bundle; fall back to active() otherwise (and on a
bundle-read fault). Removes the double keyring IPC.
Tests + docs:
- Cover missing-oauth4webapi, lock-release-after-error, no-setBundle;
replace a leaky setTimeout with awaited Promise.all.
- README: oauth4webapi in the deps list; fetchLive `bundle` + the
single-read behaviour.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- internal.ts: extract readSecretSlot for the shared getSecret/trim + slot-empty/unavailable/error mapping; the fallback field, the refresh not-present gate, and the empty-slot detail stay in each reader. - token-store.ts: extract resolveRecord (the ref→record resolution) and accessReadError (the access-outcome → AUTH_STORE_READ_FAILED mapping), shared by active() and activeBundle(). - pkce.ts: extract expiresAtFromExpiresIn, the one bit of shared logic between exchangeCode and refreshToken (field copying stays inline — a full shared mapper snagged on access_token optionality). Behaviour unchanged; 418 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@doistbot /review |
doistbot
reviewed
May 21, 2026
Member
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR introduces a robust silent refresh helper, integrates PKCE token rotation via oauth4webapi, and enhances provider reusability with async resolvers and custom fetch support. The implementation thoroughly tackles the complexities of concurrent token rotation and async configuration, laying excellent groundwork for resilient client authentication. A few adjustments are needed to address lock lifecycle race conditions, ensure accurate token rotation checks, refine error handling and module caching, remove redundant comments, align the README with the updated API, and expand test coverage for the new edge cases and async paths.
refresh.ts: - Ownership-token lock: each holder writes a unique token and only unlinks a lock it still owns, so a stolen stale lock can't be deleted by the original (slow) holder reopening the critical section. - Under-lock null re-read now aborts AUTH_REFRESH_UNAVAILABLE instead of falling back to the pre-lock snapshot — a concurrent logout can no longer be revived by a racing refresh. - hasRotated() compares the refresh-side fields too, so a refresh-only rotation by the holder is adopted by waiters. pkce.ts: - refreshToken passes handshake.flags to its resolvers (parity with exchangeCode). - loadOauth4webapi distinguishes a missing peer (install hint) from an init failure (real cause) and memoises the import promise. persist.ts: - bundleFromExchange only carries the previous refresh expiry forward when the refresh token itself is unchanged (no stale expiry on a freshly rotated token). status.ts: - The activeBundle→active fallback now only catches AUTH_STORE_READ_FAILED; other errors propagate instead of being masked. Tests: stale-lock steal (backdated mtime), refresh-path async resolvers, activeBundle slot-error → AUTH_STORE_READ_FAILED, non-empty handshake passthrough. Dropped two restate-the-code comments. README synced (handshake?, setBundle requirement). 422 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fold the five near-identical AUTH_REFRESH_UNAVAILABLE gate tests and the two rotated:false tests into two it.each blocks. Same branch coverage (every gate + both no-rotate paths still exercised), ~35 fewer lines, less duplication. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
doist-release-bot Bot
added a commit
that referenced
this pull request
May 21, 2026
## [0.19.0](v0.18.0...v0.19.0) (2026-05-21) ### Features * **auth:** silent refresh helper + PKCE refreshToken via oauth4webapi ([#39](#39)) ([45e4f22](45e4f22)), closes [refresh.test.ts#propagates](https://github.com/Doist/refresh.test.ts/issues/propagates)
Contributor
|
🎉 This PR is included in version 0.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 tasks
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
2 participants
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
refreshAccessToken({ store, provider, lockPath, skewMs?, force?, ref? })rotates the access token proactively (default 60s skew) or reactively (force: trueafter a 401).O_EXCLfile lock atlockPathserialises concurrent invocations; waiters re-read the rotated bundle and skip POSTing.createPkceProvidernow implementsrefreshTokenviaoauth4webapi, declared as an optional peer dep (only refresh-capable consumers install it).invalid_grant(any status — proxies vary) →AUTH_REFRESH_EXPIRED; everything else →AUTH_REFRESH_TRANSIENT; missing peer / no refresh token / noactiveBundle/ no provider hook →AUTH_REFRESH_UNAVAILABLE.createPkceProviderand inheritrefreshTokeninstead of hand-rolling a provider):authorizeUrl/tokenUrl/clientIdresolvers may now returnstring | Promise<string>(resolve config / prompt asynchronously). Backwards-compatible — a literal string or sync resolver still satisfies the widened type.fetchImplis threaded into the oauth4webapi refresh grant via itscustomFetch, so a custom transport (proxy dispatcher, decompression) applies to refresh rather than being bypassed by the library's globalfetch.runOAuthFlowpersists the full bundle viapersistBundle({ promoteDefault: true })so refresh tokens issued at login land on the keyring's refresh slot.TokenStore.activeBundle?(ref)added (required override onKeyringTokenStore). Parallel-reads both slots, honouring thehasRefreshToken: falserecord gate so access-only records skip the refresh-slot IPC.active()stays narrow.status.fetchLivectx now optionally carriesbundle— best-effort populated when the store implementsactiveBundle.Driver: Doist/outline-cli#74.
PR1 (#37) established the storage contract; PR2 (#38) hardened legacy-record migration. This PR completes the trio.
Out of scope (deferred)
refreshToken(no consumer needs it yet; theAuthProviderhook is already in place).exchangeCodeontooauth4webapi.oauth4webapion logout.createPkceProvideris public-client only — the refresh grant sends noclient_secret. OAuth apps must be registered as public clients; a confidential client rejects public-client refresh (invalid_request: Missing client_secret). Addingclient_secretauth (ClientSecretPost) would be a follow-up.Test plan
npm run checkcleannpm run type-checkcleannpm test— 414 tests pass (23 new)npm run buildcleanol auth loginpersists the bundle (both keyring slots), and a command past access-token expiry refreshes silently end-to-end (validated against a public Outline OAuth app)AUTH_REFRESH_EXPIREDwith re-login hintolcommands at expired moment → one POST hits the serveroauth4webapiinstalled in consumer: refresh throwsAUTH_REFRESH_UNAVAILABLEwith install hint; login still works🤖 Generated with Claude Code