feat(auth): add createDcrProvider for RFC 7591 dynamic client registration by scottlovegrove · Pull Request #31 · Doist/cli-core

scottlovegrove · 2026-05-17T17:13:19Z

Summary

New createDcrProvider factory for the RFC 7591 Dynamic Client Registration flow. prepare() POSTs clientMetadata to registrationUrl, threads the issued client_id (and client_secret, when returned) through the handshake; exchangeCode() authenticates the token POST per tokenEndpointAuthMethod (client_secret_basic default / _post / none / public-client fallback when DCR returned no secret).
Refactor pkce.ts onto a new private _oauth.ts helper module (postTokenEndpoint, buildPkceAuthorizeUrl, buildAuthError, resolve, safeReadText). PKCE provider drops from 182 → 115 LOC. The helper is grant-agnostic so a future refresh-token feature reuses it unchanged.
New errorHints?: string[] option on both createPkceProvider and createDcrProvider. Prepended to every CliError the factories throw; the server-returned response body (on non-2xx) is appended after so the actionable hint stays at the top.
New AUTH_DCR_FAILED code on the AuthErrorCode union.

No external API change for existing createPkceProvider consumers.

Validation

Verified end-to-end against twist.com via npm link into a downstream PR that migrates twist-cli's bespoke ~150-LOC AuthProvider onto createDcrProvider — the auth-provider module drops from 505 → 301 LOC there. Twist PR follows once this releases.

Test plan

npm run check — clean
npm run type-check — clean
npm test — 387 pass (372 before this branch, +15 covering DCR factory, _oauth helpers, errorHints ordering)
npm run build — clean
Live OAuth flow via linked twist-cli against twist.com — completed full DCR + PKCE + token exchange + persist

🤖 Generated with Claude Code

…ation Ships the missing OAuth provider factory for flows that mint a per-install `client_id` / `client_secret` via [RFC 7591](https://datatracker.ietf.org/doc/html/rfc7591). `prepare()` POSTs the client metadata, threads the issued credentials through the handshake, and `exchangeCode()` authenticates the token POST per `tokenEndpointAuthMethod` (`client_secret_basic` default, `_post`, or `none` for public-client). Mirrors `createPkceProvider`'s ergonomics — caller supplies `validate`; same `PkceLazyString` resolvers for URLs. Also: - Extract a private `_oauth.ts` with `postTokenEndpoint`, `buildPkceAuthorizeUrl`, `buildAuthError`, `resolve`, `safeReadText`. `pkce.ts` is now thin (≈115 LOC). The helper is grant-agnostic so a future refresh-token feature reuses it unchanged — `body: new URLSearchParams({grant_type: 'refresh_token', …})`. - New `errorHints?: string[]` option on both factories. Prepended to every `CliError` they throw; server-returned body text (on non-2xx) is appended after so the actionable hint stays at the top. - New `AUTH_DCR_FAILED` error code on the auth-error union. No external API change for existing `createPkceProvider` consumers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

doistbot

This PR introduces the createDcrProvider for RFC 7591 dynamic client registration and cleanly refactors shared OAuth logic into a reusable helper module. The implementation successfully sets up the new dynamic flow while keeping the API surface consistent and significantly reducing downstream code size. There are a few remaining refinements to ensure protocol correctness—such as using the server-provided token_endpoint_auth_method, URL-encoding Basic Auth credentials, and caching client credentials to prevent redundant registrations—along with some opportunities to generalize type names, further deduplicate fetch and authorization logic, and expand test coverage for error hints and fallback behaviors.

_{Share Feedback • Review Logs}

doistbot · 2026-05-17T17:16:57Z

+    }
+    if (input.basicAuth) {
+        const encoded = Buffer.from(
+            `${input.basicAuth.clientId}:${input.basicAuth.clientSecret}`,


[P2] RFC 6749 Section 2.3.1 requires client_id and client_secret to be URL-encoded before being concatenated with a colon for HTTP Basic Authentication. If either value contains a colon natively, the authorization server will misparse the credentials.

`${encodeURIComponent(input.basicAuth.clientId)}:${encodeURIComponent(input.basicAuth.clientSecret)}`

Fixed in c1d9c85. Pulled into a named encodeBasicAuth(clientId, clientSecret) helper in _oauth.ts that URL-encodes each component before joining with the colon, per RFC 6749 §2.3.1.

doistbot · 2026-05-17T17:16:57Z

+): AuthProvider<TAccount> {
+    const fetchImpl = options.fetchImpl ?? fetch
+    const scopeSeparator = options.scopeSeparator ?? ' '
+    const tokenEndpointAuthMethod =


[P1] This bakes the token auth method from the request metadata, but the RFC 7591 registration response is authoritative and can legally default or override token_endpoint_auth_method. If a server registers the client as client_secret_post (or any other supported method) while this value remains client_secret_basic, exchangeCode() will authenticate the token request incorrectly and the new flow will fail against an otherwise valid DCR server. Please capture the returned token_endpoint_auth_method in prepare() and use that handshake value during exchangeCode().

Fixed in c1d9c85. prepare() now persists payload.token_endpoint_auth_method on the handshake (validated against the known set), and exchangeCode() reads it back with the configured value as a fallback. Test added: "honours the server's token_endpoint_auth_method from the registration response over the configured one (RFC 7591 §3.2.1)".

doistbot · 2026-05-17T17:16:57Z

+            const fail = (message: string, extra?: string) =>
+                buildAuthError('AUTH_DCR_FAILED', message, options.errorHints, extra)
+
+            let response: Response


[P2] The try/catch around fetch, the !response.ok check using safeReadText, and the defensive response.json() parsing block here are identical to the logic in postTokenEndpoint (in _oauth.ts). Since the error messages also share the exact same structure (<Endpoint> request failed..., <Endpoint> returned HTTP...), consider extracting this boilerplate into a shared fetchJsonEndpoint helper in _oauth.ts to fully deduplicate the network and parse error handling.

Done in c1d9c85. Extracted postAndParseJson<T> in _oauth.ts — handles POST + network/non-2xx/non-JSON error wrapping with caller-supplied errorCode and errorLabel. Both postTokenEndpoint and DCR prepare() now route through it; each caller still validates its own success shape (access_token vs client_id) since those differ.

doistbot · 2026-05-17T17:16:57Z

+                )
+            }
+
+            const verifier = generateVerifier({


[P2] This authorize() path is now a second copy of the PKCE authorize flow from createPkceProvider (verifier generation, challenge derivation, authorize URL assembly, and codeVerifier handshake plumbing). Pull that sequence into a shared helper and let each provider only supply its clientId; otherwise any future authorize-parameter change has to be kept in sync in two places.

Considered and declining for now. buildPkceAuthorizeUrl already absorbs the URL-assembly half (used by both factories). What's left in each authorize() body is ~10 LOC of factory-specific glue: where clientId comes from (factory config vs. handshake), the typed handshake guard, and what gets written back to the handshake. A shared helper would have to accept all of those as inputs, so the indirection cost would exceed the LOC saved. If a third provider lands that needs the same shape, this becomes the right time.

doistbot · 2026-05-17T17:16:57Z

+ * and runtime flags. Used by every provider in this directory.
+ */
+export function resolve(
+    resolver: PkceLazyString,


[P3] The generic resolve helper and the new DcrProviderOptions both rely on PkceLazyString. Consider renaming this type to something grant-agnostic (e.g. OAuthLazyString or LazyEndpoint) and moving it to _oauth.ts or types.ts so the shared OAuth utilities and DCR provider don't depend on a PKCE-specific name.

Added OAuthLazyString as a grant-agnostic alias of PkceLazyString in c1d9c85, exported from the auth subpath alongside the original. New code can use the grant-agnostic name; existing PkceLazyString consumers stay green. Kept the alias rather than a rename because the original is already part of the published surface (cli-core 0.16.x).

doistbot · 2026-05-17T17:16:58Z

+        options.clientMetadata.tokenEndpointAuthMethod ?? 'client_secret_basic'
+
+    return {
+        async prepare(input: PrepareInput): Promise<PrepareResult> {


[P2] prepare() always performs a fresh RFC 7591 registration and only keeps the issued client_id / client_secret in the ephemeral handshake. That means every repeated login from the same install pays an extra registration round-trip and creates another server-side client record, even though this provider is otherwise framed around per-install clients. Consider adding a caller-supplied cache/persistence hook (or another reuse path) so registration can be skipped when the client is already known.

Deferred — it's a deliberate design choice for the current consumer. twist-cli wants stateless one-shot DCR (fresh client per login, no server-side residue) so it matches their threat model and keeps the persistence story per-CLI. The handshake-only model also keeps cli-core out of "where do we put long-lived OAuth client credentials" territory, which would otherwise pull createSecureStore into the factory's contract. Happy to revisit as a follow-up if a downstream actually wants reuse — the most natural shape would be a caller-supplied {load, save} hook so the storage choice stays in the CLI.

doistbot · 2026-05-17T17:16:58Z

+        expect(tokenBody.get('client_secret')).toBe('sec')
+    })
+
+    it('tokenEndpointAuthMethod=none (or missing client_secret) sends client_id in the body and no Authorization header', async () => {


[P2] This test doesn't actually cover the "missing client_secret" fallback it describes, because it also sets tokenEndpointAuthMethod: 'none'. That means it would still pass even if a client_secret_post or default-basic registration that returned no secret started authenticating the token request incorrectly. Add a case where the DCR response omits client_secret while the requested method is 'client_secret_post' (or the default) so the fallback branch is exercised independently.

Fixed in c1d9c85. Added a dedicated test: "falls back to public-client POST when registration omits client_secret even though client_secret_post was requested" — sets tokenEndpointAuthMethod: 'client_secret_post', has the DCR response return only client_id, asserts the token request has no Authorization header and client_id in the body with no client_secret.

doistbot · 2026-05-17T17:16:58Z

+        ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' })
+    })
+
+    it('errorHints are prepended to every CliError, with body text appended after on non-2xx', async () => {


[P2] These assertions prove the helper can prepend errorHints, but they don't cover the new createPkceProvider({ errorHints }) API itself. If PKCE stopped forwarding options.errorHints into postTokenEndpoint, or into the lost-handshake guard, this suite would still stay green. Please add a provider-level case in pkce.test.ts for both paths.

Fixed in c1d9c85. Added "forwards errorHints onto both the token-endpoint failure and the handshake-lost guard" in pkce.test.ts — exercises createPkceProvider({ errorHints }) end-to-end on both throw sites, so a regression in either plumbing path now turns the suite red.

Drop tests that duplicate behaviour already exercised elsewhere or that test obvious helpers in isolation: - `_oauth.test.ts`: remove standalone describes for `resolve`, `safeReadText`, and `buildPkceAuthorizeUrl` — already covered by the provider integration tests. Consolidate the four token-endpoint error cases into a single parametrised case and merge the duplicate errorHints assertion into the non-2xx test. - `dcr.test.ts`: collapse the three DCR error tests (missing client_id + non-JSON kept as a single parametrised case; non-2xx absorbed the errorHints assertion) and drop the standalone "token endpoint non-2xx" test that overlaps with `_oauth.test.ts`. Net –231 LOC across the two test files. 374 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…code Basic, dedupe fetch+parse - DCR now persists the registration response's `token_endpoint_auth_method` on the handshake and lets it override the configured one at `exchangeCode()`, per RFC 7591 §3.2.1. Validate the value against the known set so an unknown method silently falls back to the configured one. - URL-encode `client_id` and `client_secret` before joining with a colon for HTTP Basic auth per RFC 6749 §2.3.1. A literal colon (or any reserved character) in either value would otherwise corrupt the credential. - Extract `postAndParseJson` in `_oauth.ts`. Both `postTokenEndpoint` and DCR `prepare()` now share the network / non-2xx / parse error wrapping while still validating their own success shape. - Add `OAuthLazyString` as a grant-agnostic alias of `PkceLazyString` (non-breaking; existing name still exported). - Export `DcrTokenEndpointAuthMethod` from the auth subpath so callers can refer to the union directly. - Tests: cover the "no `client_secret` but `client_secret_post` requested → falls back to public-client POST" branch, the "server-issued `token_endpoint_auth_method` wins" branch, and the PKCE-level `errorHints` plumbing (both the token-endpoint path and the handshake-lost guard). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

doistbot reviewed May 17, 2026

View reviewed changes

scottlovegrove and others added 2 commits May 17, 2026 18:18

Conversation

scottlovegrove commented May 17, 2026

Summary

Validation

Test plan

Uh oh!

doistbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants