feat(packages/core)!: replace oauth with PKCE and add device code flow#7
Merged
zrosenbauer merged 8 commits intomainfrom Mar 10, 2026
Merged
feat(packages/core)!: replace oauth with PKCE and add device code flow#7zrosenbauer merged 8 commits intomainfrom
zrosenbauer merged 8 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements spec-compliant OAuth flows (Authorization Code + PKCE and Device Authorization Grant), adds shared OAuth utilities, introduces an AuthFactory with resolver builders and http integration, expands types/exports and constants, updates resolver dispatch, adds extensive unit/integration tests, examples, a mock OAuth server, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant LocalServer as Local Server
participant Provider as OAuth Provider
participant TokenEndpoint as Token Endpoint
CLI->>CLI: generate code_verifier & code_challenge
CLI->>LocalServer: start local HTTP server
LocalServer-->>CLI: listening port
CLI->>Provider: open auth URL (response_type=code, client_id, code_challenge, state, redirect_uri)
User->>Provider: authenticate & authorize
Provider->>LocalServer: redirect with code & state
LocalServer->>CLI: deliver code & state
CLI->>CLI: validate state
CLI->>TokenEndpoint: POST code + code_verifier + client_id to tokenUrl
TokenEndpoint-->>CLI: return access_token
LocalServer->>User: serve success HTML
sequenceDiagram
participant User
participant CLI
participant DeviceAuth as Device Auth Endpoint
participant Browser as Browser/Verification
participant TokenEndpoint as Token Endpoint
CLI->>DeviceAuth: POST client_id & scopes to deviceAuthUrl
DeviceAuth-->>CLI: device_code, user_code, verification_uri, interval
CLI->>User: display verification_uri & user_code (prompt)
CLI->>Browser: optionally open verification_uri
User->>Browser: enter user_code & approve
loop polling
CLI->>TokenEndpoint: POST device_code + client_id to tokenUrl
alt authorization_pending
TokenEndpoint-->>CLI: error authorization_pending
CLI->>CLI: wait & retry
else slow_down
TokenEndpoint-->>CLI: error slow_down
CLI->>CLI: increase interval & retry
else success
TokenEndpoint-->>CLI: access_token
CLI->>CLI: return credential
else denied/expired
TokenEndpoint-->>CLI: error access_denied/expired_token
CLI->>CLI: return null
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
packages/core/src/middleware/auth/resolve-oauth.test.ts (1)
164-219: Prove the invalid callbacks never reach the token endpoint.These cases currently assert the 400 response, then finish the resolver with a later valid callback. That still passes if the invalid request prematurely hits the token endpoint or tears down the flow before cleanup. Add a
fetchspy before the first callback and assert it stays untouched until the cleanup request.🧪 Suggested pattern
it('should return 400 when state does not match', async () => { + const fetchSpy = vi.spyOn(globalThis, 'fetch') const resultPromise = resolveFromOAuth({ authUrl: 'https://auth.example.com/authorize', callbackPath: '/callback', clientId: 'test-client', @@ }) expect(response.status).toBe(400) + expect(fetchSpy).not.toHaveBeenCalled() - vi.spyOn(globalThis, 'fetch').mockResolvedValue( + fetchSpy.mockResolvedValue( new Response(JSON.stringify({ access_token: 'cleanup' }), { status: 200 }) )Also applies to: 373-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/resolve-oauth.test.ts` around lines 164 - 219, Before sending the invalid redirect in the failing test cases for resolveFromOAuth, install a spy on globalThis.fetch (vi.spyOn(globalThis, 'fetch')) so you can assert the token endpoint is not invoked by the invalid callback; call the spy before sendCallback, assert spy.not.toHaveBeenCalled() immediately after verifying the 400 response, then replace the spy with a mockResolvedValue or set its resolved value for the cleanup valid callback, and finally assert the spy was called exactly once after shutdownServer and awaiting resultPromise; apply the same pattern to the other failing-test block that uses resolveFromOAuth/sendCallback/waitForServer/shutdownServer.packages/core/test/helpers/mock-oauth-server.ts (1)
9-53: Document the exported mock API and interval units.
deviceIntervalhere is RFC seconds, while the runtime resolver config uses millisecondpollInterval. A short JSDoc on the exported types and factory would make that distinction obvious to test authors and keep the new public test surface self-describing.As per coding guidelines "Use object parameters for functions with 2 or more parameters and provide explicit return types with JSDoc on exports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/helpers/mock-oauth-server.ts` around lines 9 - 53, Add clear JSDoc comments to the exported types and factory: document DevicePollResponse variants, MockOAuthServerOptions fields (explicitly note that deviceInterval is in RFC seconds), and the createMockOAuthServer factory return (Promise<MockOAuthServer>), so test authors know units and behavior; mention that runtime resolver uses millisecond pollInterval to avoid confusion between deviceInterval (seconds) and pollInterval (ms). Ensure JSDoc appears on MockOAuthServerOptions, DevicePollResponse, and createMockOAuthServer to make the public test API self-describing.packages/core/test/integration/auth-oauth-pkce.test.ts (2)
36-46: Silent fallback may mask test failures.When
redirect_uriis null, returning a placeholder URL (http://127.0.0.1:0) silently may mask legitimate test failures where the redirect URI wasn't properly set.♻️ Consider throwing or using a more explicit pattern
function extractRedirectUri(): URL { const [call] = vi.mocked(execFile).mock.calls const authUrl = new URL(call[1][0]) const redirectRaw = authUrl.searchParams.get('redirect_uri') if (redirectRaw === null) { - return new URL('http://127.0.0.1:0') + throw new Error('redirect_uri not found in authorization URL - test setup may be incorrect') } return new URL(redirectRaw) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/integration/auth-oauth-pkce.test.ts` around lines 36 - 46, The test helper extractRedirectUri currently returns a silent placeholder URL when redirect_uri is missing, which can mask failures; update extractRedirectUri (used with vi.mocked(execFile).mock.calls) to throw a clear error (or return a Result/optional that the test asserts) when authUrl.searchParams.get('redirect_uri') === null so tests fail fast and surface the missing redirect URI instead of returning 'http://127.0.0.1:0'.
97-107: Consider avoidingletper coding guidelines.The
let mockServerdeclaration violates the project's coding guideline. Consider using a factory pattern or test context object.♻️ Alternative pattern using test context
-describe('OAuth PKCE E2E (resolveFromOAuth with real mock server)', () => { - let mockServer: MockOAuthServer | null = null - - afterEach(async () => { - vi.clearAllMocks() - vi.restoreAllMocks() - - if (mockServer !== null) { - mockServer.close() - mockServer = null - } - }) +describe('OAuth PKCE E2E (resolveFromOAuth with real mock server)', () => { + const testState: { server: MockOAuthServer | null } = { server: null } + + afterEach(async () => { + vi.clearAllMocks() + vi.restoreAllMocks() + + if (testState.server !== null) { + testState.server.close() + testState.server = null + } + })As per coding guidelines: "Never use classes, loops,
let, orthrow".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/integration/auth-oauth-pkce.test.ts` around lines 97 - 107, The test uses a top-level mutable variable "mockServer" declared with let; replace it with an immutable test context or factory to comply with the "no let" guideline. For example, introduce a const testContext = { mockServer: null as MockOAuthServer | null } (or a createMockServer() factory) and update all references in beforeEach/afterEach and tests to use testContext.mockServer (or the factory return) and call testContext.mockServer.close() in afterEach; ensure the afterEach still clears and restores mocks (vi.clearAllMocks/vi.restoreAllMocks) and sets testContext.mockServer = null instead of reassigning a top-level variable.packages/core/test/integration/auth-http-chain.test.ts (1)
72-76: Consider avoidingletper coding guidelines.The
let fetchSpydeclaration violates the project's coding guideline that prohibitslet. Consider initializing the spy directly in each test or using a pattern that avoids reassignment.♻️ Suggested refactor using per-test initialization
-describe('Auth + HTTP Middleware Chain', () => { - let fetchSpy: ReturnType<typeof vi.spyOn<typeof globalThis, 'fetch'>> - - beforeEach(() => { - fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue(createMockResponse()) - }) +describe('Auth + HTTP Middleware Chain', () => { + const createFetchSpy = (): ReturnType<typeof vi.spyOn<typeof globalThis, 'fetch'>> => + vi.spyOn(globalThis, 'fetch').mockResolvedValue(createMockResponse())Then in each test:
it('should propagate bearer credential...', async () => { const fetchSpy = createFetchSpy() // ...rest of test })As per coding guidelines: "Never use classes, loops,
let, orthrow".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/integration/auth-http-chain.test.ts` around lines 72 - 76, Remove the module-level "let fetchSpy" and the beforeEach spy setup; instead add a small helper (e.g., createFetchSpy) that returns vi.spyOn(globalThis, 'fetch').mockResolvedValue(createMockResponse()), and update each test to call const fetchSpy = createFetchSpy() at the start of the test; this avoids reassignment and eliminates use of let and the beforeEach hook while keeping the same spy behavior (referencing fetchSpy, beforeEach, vi.spyOn, and createMockResponse to locate the affected code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/middleware/auth/README.md`:
- Line 130: Replace the literal allowlist sentence "Supported by GitHub, Azure
AD, and Google. Not supported by Clerk." with a capability-oriented statement
that avoids a short provider list—e.g., indicate the resolver supports major
OAuth/OIDC providers (mentioning GitHub, Azure AD, Google only as examples or
using "e.g.") and point readers to research/auth/device-code-overview.md for
full details and limitations (explicitly note Clerk is not supported if still
true); update the README.md sentence accordingly so it remains correct as
endpoints change.
In `@packages/core/test/helpers/mock-oauth-server.ts`:
- Around line 16-26: Add an explicit expected PKCE verifier to the mock and
validate it during the token exchange: extend MockOAuthServerOptions with an
expectedCodeVerifier (or similar) and in the token endpoint handler (the code
path that reads body.code_verifier for the authorization_code exchange—also
update the same logic referenced around lines 178-205) compare the incoming
code_verifier to expectedCodeVerifier; if they differ (or a verifier is missing)
return an error response (HTTP 400 with an OAuth error like
invalid_grant/invalid_request) instead of accepting any non-empty verifier.
Ensure tests that rely on rejectVerifier still behave by using the new option to
simulate a mismatch.
- Around line 213-224: handleDeviceCodeGrant currently only validates
device_code, so polling requests can omit/mangle client_id; update
handleDeviceCodeGrant to also read params.get('client_id') and verify it matches
options.clientId, and if not, call sendJsonResponse(res, 400, { error:
'invalid_client' }) (or reuse the existing error shape used for client
validation elsewhere) and return—this will mirror the /device/code validation
and prevent polls from succeeding without a valid client_id.
In `@research/auth/current-implementation.md`:
- Around line 16-49: Add an explicit language tag "text" to both fenced code
blocks that contain the ASCII OAuth flow diagram (the two triple-backtick
diagram fences in the file) so the blocks are typed as ```text ... ``` which
preserves the diagram formatting and removes markdownlint warnings; update each
fence surrounding the diagram (both the earlier and later occurrence)
accordingly.
In `@research/auth/device-code-overview.md`:
- Around line 13-41: The fenced flow block in
research/auth/device-code-overview.md is missing a language hint and triggers
MD040; update the opening fence for the block containing the numbered
device-code flow to use a language tag (e.g., change ``` to ```text) so the
block is treated as plain text and the linter warning is resolved.
In `@research/auth/pkce-overview.md`:
- Around line 28-48: The markdown blocks with the ASCII PKCE diagram and
protocol snippets are triggering MD040 because they lack a language; update the
three fenced code blocks (the ASCII flow containing "code_verifier" /
"code_challenge" and the protocol examples that include "Authorization Request"
/ "Token Request" and "Server: SHA256(code_verifier)") to use explicit info
strings: use "text" for the ASCII flow and "http" (or "text") for the
request/response snippets so the linter is satisfied and the doc remains
lint-clean.
---
Nitpick comments:
In `@packages/core/src/middleware/auth/resolve-oauth.test.ts`:
- Around line 164-219: Before sending the invalid redirect in the failing test
cases for resolveFromOAuth, install a spy on globalThis.fetch
(vi.spyOn(globalThis, 'fetch')) so you can assert the token endpoint is not
invoked by the invalid callback; call the spy before sendCallback, assert
spy.not.toHaveBeenCalled() immediately after verifying the 400 response, then
replace the spy with a mockResolvedValue or set its resolved value for the
cleanup valid callback, and finally assert the spy was called exactly once after
shutdownServer and awaiting resultPromise; apply the same pattern to the other
failing-test block that uses
resolveFromOAuth/sendCallback/waitForServer/shutdownServer.
In `@packages/core/test/helpers/mock-oauth-server.ts`:
- Around line 9-53: Add clear JSDoc comments to the exported types and factory:
document DevicePollResponse variants, MockOAuthServerOptions fields (explicitly
note that deviceInterval is in RFC seconds), and the createMockOAuthServer
factory return (Promise<MockOAuthServer>), so test authors know units and
behavior; mention that runtime resolver uses millisecond pollInterval to avoid
confusion between deviceInterval (seconds) and pollInterval (ms). Ensure JSDoc
appears on MockOAuthServerOptions, DevicePollResponse, and createMockOAuthServer
to make the public test API self-describing.
In `@packages/core/test/integration/auth-http-chain.test.ts`:
- Around line 72-76: Remove the module-level "let fetchSpy" and the beforeEach
spy setup; instead add a small helper (e.g., createFetchSpy) that returns
vi.spyOn(globalThis, 'fetch').mockResolvedValue(createMockResponse()), and
update each test to call const fetchSpy = createFetchSpy() at the start of the
test; this avoids reassignment and eliminates use of let and the beforeEach hook
while keeping the same spy behavior (referencing fetchSpy, beforeEach, vi.spyOn,
and createMockResponse to locate the affected code).
In `@packages/core/test/integration/auth-oauth-pkce.test.ts`:
- Around line 36-46: The test helper extractRedirectUri currently returns a
silent placeholder URL when redirect_uri is missing, which can mask failures;
update extractRedirectUri (used with vi.mocked(execFile).mock.calls) to throw a
clear error (or return a Result/optional that the test asserts) when
authUrl.searchParams.get('redirect_uri') === null so tests fail fast and surface
the missing redirect URI instead of returning 'http://127.0.0.1:0'.
- Around line 97-107: The test uses a top-level mutable variable "mockServer"
declared with let; replace it with an immutable test context or factory to
comply with the "no let" guideline. For example, introduce a const testContext =
{ mockServer: null as MockOAuthServer | null } (or a createMockServer() factory)
and update all references in beforeEach/afterEach and tests to use
testContext.mockServer (or the factory return) and call
testContext.mockServer.close() in afterEach; ensure the afterEach still clears
and restores mocks (vi.clearAllMocks/vi.restoreAllMocks) and sets
testContext.mockServer = null instead of reassigning a top-level variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45545d11-4f00-47a3-9bb4-637a99a29f07
📒 Files selected for processing (25)
.changeset/config.json.changeset/oauth-pkce-device-code.mddocs/concepts/authentication.mddocs/guides/add-authentication.mdexamples/authenticated-service/cli/src/index.tspackages/core/src/middleware/auth/README.mdpackages/core/src/middleware/auth/constants.tspackages/core/src/middleware/auth/index.tspackages/core/src/middleware/auth/oauth-shared.test.tspackages/core/src/middleware/auth/oauth-shared.tspackages/core/src/middleware/auth/resolve-credentials.test.tspackages/core/src/middleware/auth/resolve-credentials.tspackages/core/src/middleware/auth/resolve-device-code.test.tspackages/core/src/middleware/auth/resolve-device-code.tspackages/core/src/middleware/auth/resolve-oauth.test.tspackages/core/src/middleware/auth/resolve-oauth.tspackages/core/src/middleware/auth/types.tspackages/core/test/helpers/mock-oauth-server.tspackages/core/test/integration/auth-device-code.test.tspackages/core/test/integration/auth-http-chain.test.tspackages/core/test/integration/auth-oauth-pkce.test.tsresearch/auth/clerk-pkce.mdresearch/auth/current-implementation.mdresearch/auth/device-code-overview.mdresearch/auth/pkce-overview.md
zrosenbauer
force-pushed
the
feat/oauth-pkce-device-code
branch
from
9d87c93 to
a33ecca
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
research/auth/device-code-overview.md (1)
13-41:⚠️ Potential issue | 🟡 MinorAdd a language tag to this flow fence.
Line 13 still uses an untyped fenced block, so markdownlint will keep flagging MD040.
textis enough here and preserves the pseudo-flow formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@research/auth/device-code-overview.md` around lines 13 - 41, The fenced code block illustrating the device-code flow is missing a language tag which triggers MD040; update the opening fence (the triple backticks that start the flow example in research/auth/device-code-overview.md) to include a language tag (use "text") so it becomes ```text and preserves the pseudo-flow formatting without markdownlint warnings.research/auth/current-implementation.md (1)
16-49:⚠️ Potential issue | 🟡 MinorType both diagram fences as
text.Lines 16 and 70 still trigger MD040. Adding
textkeeps the diagrams readable and clears the lint warning.Also applies to: 70-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@research/auth/current-implementation.md` around lines 16 - 49, The fenced ASCII diagram blocks in the markdown (the triple-backtick diagram shown between the diff markers) are currently untyped and trigger MD040; change both fences to use a language tag of "text" (i.e., replace ``` with ```text) for the diagram shown around the code snippet and do the same for the second diagram block referenced (lines 70-96) so the diagrams remain readable and the linter warning is cleared.packages/core/src/middleware/auth/README.md (1)
130-130:⚠️ Potential issue | 🟡 MinorBroaden this provider-support statement.
This allowlist is already narrower than
research/auth/device-code-overview.md, which also calls out Auth0 and Okta. Phrase it in terms of providers that expose RFC 8628 endpoints, and keep Clerk as the explicit limitation if that is still intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/README.md` at line 130, The current sentence "Supported by GitHub, Azure AD, and Google. Not supported by Clerk." is too narrow; update the README sentence to describe support generically (e.g., "Supported by providers that expose RFC 8628 device authorization endpoints (for example GitHub, Azure AD, Google, Auth0, Okta). Clerk is not supported.") — locate the sentence in packages/core/src/middleware/auth/README.md and replace it with a phrase referencing RFC 8628 compliance and explicitly call out Clerk as not supported so it aligns with research/auth/device-code-overview.md.
🧹 Nitpick comments (1)
packages/core/src/middleware/auth/oauth-shared.ts (1)
128-151: Refactor these exported helpers to use object parameters.
trackConnections()anddestroyServer()are exported functions with two positional parameters. Per the coding guidelines, functions with 2+ parameters should use object parameters instead:export function trackConnections({ server, sockets }: { server: Server, sockets: Set<Socket> }): void { // ... } export function destroyServer({ server, sockets }: { server: Server, sockets: Set<Socket> }): void { // ... }This will require updating call sites in
oauth-shared.ts,resolve-oauth.ts, and test files, but makes the API clearer and less prone to argument reordering errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/oauth-shared.ts` around lines 128 - 151, Change the two exported helpers trackConnections and destroyServer to accept a single object parameter (use destructuring: { server, sockets }: { server: Server; sockets: Set<Socket> }) instead of two positional args; update their internal references accordingly and update every call site that invokes trackConnections(...) or destroyServer(...) (including usages in resolve-oauth and related tests) to pass an object with keys server and sockets to match the new signatures so type checking and call semantics remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/concepts/authentication.md`:
- Line 181: Replace the hard-coded provider list line ("Supported by GitHub,
Azure AD, and Google. Not supported by Clerk.") with capability-based wording
that references RFC 8628 device-authorization support; e.g., state that this
flow is supported by "providers that expose RFC 8628 (device authorization)
endpoints" and optionally list examples (GitHub, Azure AD, Google, Auth0, Okta)
while noting Clerk does not support it, so update the sentence in
docs/concepts/authentication.md accordingly.
In `@packages/core/src/middleware/auth/oauth-shared.ts`:
- Around line 172-178: The openBrowser function calls execFile(command, args)
without handling the ChildProcess 'error' event which can crash Node in headless
environments; modify openBrowser to capture the returned ChildProcess from
execFile and attach an 'error' event handler that swallows or logs the error
(e.g., debug/info) so the auth flow can continue. Specifically update the
openBrowser function to assign const child = execFile(command, args) and then
child.on('error', err => { /* safe log or ignore */ }); to prevent unhandled
exceptions from execFile failing.
In `@packages/core/src/middleware/auth/resolve-device-code.ts`:
- Around line 58-60: The device-code resolver currently always calls
openBrowser(authResponse.verificationUri) after displayUserCode, causing
failures in non-interactive/CI/SSH/container environments; modify the logic in
the resolve-device-code path to call openBrowser only when it's safe/explicitly
requested (e.g., check an options flag like options.openBrowser === true or
detect interactive environment via options.prompts.isInteractive/TTY) and
otherwise skip the call, keeping displayUserCode(authResponse.verificationUri,
authResponse.userCode) and using authResponse as before.
- Around line 100-127: The flow can hang because requestDeviceAuth (and
similarly requestToken) use fetch without an AbortSignal, so the overall
deadline check around sleep() doesn't stop stuck HTTP calls; update
requestDeviceAuth and requestToken to create an AbortController tied to the same
timeout/deadline (or a per-call timer) and pass controller.signal into fetch,
and ensure the controller is aborted when the deadline expires or on function
exit; also clear any timers to avoid leaks and propagate fetch abort errors to
return null consistently.
- Around line 266-271: resolveServerInterval currently accepts 0, negative
numbers, NaN, and Infinity which can cause tight polling loops; update
resolveServerInterval to only return value * 1000 when the input is a finite
number greater than zero (use Number.isFinite(value) and value > 0), otherwise
return null so callers fall back to configured intervals; keep the function name
resolveServerInterval and its return behavior for valid inputs.
In `@packages/core/src/middleware/auth/resolve-oauth.ts`:
- Around line 177-194: The handler handleCallback should detect state-matched
OAuth error parameters (e.g., error=access_denied) from the incoming URL and
immediately resolve the deferred instead of returning 400 and letting the caller
time out; update logic in handleCallback (and the similar block around the later
callback handling at the other section) to parse error and state via
extractCodeFromUrl or a new extractOAuthErrorFromUrl helper, call
sendErrorPage(res) or writeHead(400)/end as appropriate, and then call resolve
with a distinct failure value (e.g., null or an error code) so the caller is
unblocked immediately rather than waiting for the timeout. Ensure the same
change is applied to the other callback-handling block referenced (lines
~209-232) so state-matched OAuth errors are resolved immediately.
- Around line 37-106: The current resolveFromOAuth implementation clears the
timeout before calling exchangeCodeForToken so the documented timeout only
covers the browser callback and not the token exchange; modify resolveFromOAuth
to keep the timeout active (or recreate/use a remaining-time Promise) and wrap
the call to exchangeCodeForToken in a race with the existing timeout.promise (or
a new timeout that represents the remaining budget) so that if the token
endpoint stalls the function returns/nulls per the timeout; refer to
createTimeout, timeout.promise, timeout.clear, codeDeferred, and
exchangeCodeForToken to locate where to start the race and ensure
timeout.clear() is only called after the token exchange completes or times out.
- Around line 280-284: The current block in resolve-oauth.ts returns a bearer
credential whenever record.access_token is a non-empty string; update it to also
validate record.token_type (the OAuth token_type field) before returning type:
'bearer': check that typeof record.token_type === 'string' and
record.token_type.toLowerCase() === 'bearer' (case-insensitive) and only then
return { token: record.access_token, type: 'bearer' }; if token_type is missing
or not "bearer", return null (or propagate the actual token_type if you intend
to support other schemes), so downstream code won’t apply the wrong auth scheme.
---
Duplicate comments:
In `@packages/core/src/middleware/auth/README.md`:
- Line 130: The current sentence "Supported by GitHub, Azure AD, and Google. Not
supported by Clerk." is too narrow; update the README sentence to describe
support generically (e.g., "Supported by providers that expose RFC 8628 device
authorization endpoints (for example GitHub, Azure AD, Google, Auth0, Okta).
Clerk is not supported.") — locate the sentence in
packages/core/src/middleware/auth/README.md and replace it with a phrase
referencing RFC 8628 compliance and explicitly call out Clerk as not supported
so it aligns with research/auth/device-code-overview.md.
In `@research/auth/current-implementation.md`:
- Around line 16-49: The fenced ASCII diagram blocks in the markdown (the
triple-backtick diagram shown between the diff markers) are currently untyped
and trigger MD040; change both fences to use a language tag of "text" (i.e.,
replace ``` with ```text) for the diagram shown around the code snippet and do
the same for the second diagram block referenced (lines 70-96) so the diagrams
remain readable and the linter warning is cleared.
In `@research/auth/device-code-overview.md`:
- Around line 13-41: The fenced code block illustrating the device-code flow is
missing a language tag which triggers MD040; update the opening fence (the
triple backticks that start the flow example in
research/auth/device-code-overview.md) to include a language tag (use "text") so
it becomes ```text and preserves the pseudo-flow formatting without markdownlint
warnings.
---
Nitpick comments:
In `@packages/core/src/middleware/auth/oauth-shared.ts`:
- Around line 128-151: Change the two exported helpers trackConnections and
destroyServer to accept a single object parameter (use destructuring: { server,
sockets }: { server: Server; sockets: Set<Socket> }) instead of two positional
args; update their internal references accordingly and update every call site
that invokes trackConnections(...) or destroyServer(...) (including usages in
resolve-oauth and related tests) to pass an object with keys server and sockets
to match the new signatures so type checking and call semantics remain correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a38f54e3-3365-44eb-a74e-c8fdcffec361
📒 Files selected for processing (25)
.changeset/config.json.changeset/oauth-pkce-device-code.mddocs/concepts/authentication.mddocs/guides/add-authentication.mdexamples/authenticated-service/cli/src/index.tspackages/core/src/middleware/auth/README.mdpackages/core/src/middleware/auth/constants.tspackages/core/src/middleware/auth/index.tspackages/core/src/middleware/auth/oauth-shared.test.tspackages/core/src/middleware/auth/oauth-shared.tspackages/core/src/middleware/auth/resolve-credentials.test.tspackages/core/src/middleware/auth/resolve-credentials.tspackages/core/src/middleware/auth/resolve-device-code.test.tspackages/core/src/middleware/auth/resolve-device-code.tspackages/core/src/middleware/auth/resolve-oauth.test.tspackages/core/src/middleware/auth/resolve-oauth.tspackages/core/src/middleware/auth/types.tspackages/core/test/helpers/mock-oauth-server.tspackages/core/test/integration/auth-device-code.test.tspackages/core/test/integration/auth-http-chain.test.tspackages/core/test/integration/auth-oauth-pkce.test.tsresearch/auth/clerk-pkce.mdresearch/auth/current-implementation.mdresearch/auth/device-code-overview.mdresearch/auth/pkce-overview.md
🚧 Files skipped from review as they are similar to previous changes (6)
- research/auth/clerk-pkce.md
- packages/core/src/middleware/auth/index.ts
- packages/core/test/integration/auth-http-chain.test.ts
- packages/core/src/middleware/auth/constants.ts
- .changeset/oauth-pkce-device-code.md
- packages/core/test/helpers/mock-oauth-server.ts
Replace the non-standard direct-token-POST OAuth flow with a spec-compliant Authorization Code + PKCE implementation (RFC 7636 + RFC 8252). Add a new device-code resolver implementing the Device Authorization Grant (RFC 8628) for headless environments. BREAKING CHANGE: OAuthSourceConfig now requires clientId and tokenUrl fields. The previous oauth resolver that accepted a direct token POST has been removed. Co-Authored-By: Claude <noreply@anthropic.com>
zrosenbauer
force-pushed
the
feat/oauth-pkce-device-code
branch
from
a33ecca to
9ee4996
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
research/auth/device-code-overview.md (1)
13-13:⚠️ Potential issue | 🟡 MinorAdd an info string to this fenced flow block.
The opening fence is still anonymous, so this doc keeps tripping MD040.
textis enough here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@research/auth/device-code-overview.md` at line 13, The fenced code block at the indicated spot is anonymous and triggers MD040; add an info string (e.g., change the opening fence from ``` to ```text) for the fenced flow block in research/auth/device-code-overview.md so the block is no longer anonymous and the linter stops flagging it.packages/core/src/middleware/auth/resolve-device-code.ts (3)
58-60:⚠️ Potential issue | 🟠 MajorKeep browser launch optional in the device-code flow.
This resolver is explicitly for SSH/CI/container-style environments, so always calling
openBrowser()adds a failing or noisy side effect in the environments that need this path most.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/resolve-device-code.ts` around lines 58 - 60, The device-code resolver is always calling openBrowser(), which causes failures in SSH/CI/container environments; update the resolve-device-code flow (around the displayUserCode(...) / openBrowser(...) calls in the resolveDeviceCode function) to only call openBrowser when a caller-controlled flag is true (e.g., options.openBrowser or options.launchBrowser) — default behavior should keep it disabled for non-interactive environments; check the flag (e.g., if (options.openBrowser) openBrowser(...)) and do not call openBrowser unconditionally.
48-52:⚠️ Potential issue | 🟠 MajorThe timeout budget still doesn't cover the full device-code flow.
deadlineis created only after the device-auth request and after waiting ondisplayUserCode(), and neither fetch call is cancelable. A slow endpoint or a prompt waiting on Enter can therefore outliveoptions.timeout.Also applies to: 58-63, 112-116, 323-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/resolve-device-code.ts` around lines 48 - 52, The timeout budget is being started too late and fetches are uncancelable; move creation of the deadline/AbortController to the very start of the device-code flow so it covers requestDeviceAuth, displayUserCode, and polling, pass the controller.signal into requestDeviceAuth and any internal fetches (and into pollForToken or equivalent), and modify displayUserCode to accept and respect an AbortSignal (or return early when aborted) so that slow endpoints or a waiting prompt are cancelled when options.timeout elapses; update calls to requestDeviceAuth, displayUserCode, and pollForToken to use the shared AbortSignal and ensure any pending timers/fetches are cleaned up on abort.
61-62:⚠️ Potential issue | 🟠 MajorValidate both server and configured poll intervals.
The server path currently accepts
0, negatives,NaN, andInfinity, and the configured fallback is passed through unchecked as well. Any of those values can collapse polling into an immediate retry loop instead of respecting RFC 8628 backoff.Also applies to: 200-206, 266-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/resolve-device-code.ts` around lines 61 - 62, The code uses resolveInterval(authResponse.interval, options.pollInterval) and then deadline = Date.now() + options.timeout without validating inputs, allowing 0, negative, NaN, or Infinity to collapse polling; update resolveInterval (and any other call sites using authResponse.interval or options.pollInterval) to sanitize both values: coerce to a finite number, reject NaN/Infinity, enforce a minimum positive integer (e.g., >= 1s) and an upper bound, and fall back to a safe default (or options.timeout) when invalid; ensure polling loop and deadline calculation use the validated interval and timeout so you never schedule immediate tight loops.packages/core/src/middleware/auth/resolve-oauth.ts (3)
86-103:⚠️ Potential issue | 🟠 MajorKeep the timeout budget through the token exchange.
timeout.clear()runs beforeexchangeCodeForToken(), so a stalled token endpoint can hang indefinitely even though this resolver documents a timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/resolve-oauth.ts` around lines 86 - 103, The timeout is cleared before calling exchangeCodeForToken, which allows a slow token endpoint to hang past the documented timeout; instead keep the timeout active through the token exchange by removing the early timeout.clear() and only clearing the timeout after the exchange completes (or by wrapping exchangeCodeForToken in a Promise.race with timeoutPromise and clearing in a finally block). Update the flow around codeDeferred/timeoutPromise and exchangeCodeForToken so timeout.clear() is invoked after the token exchange resolves/rejects, and ensure destroyServer(handle.server, handle.sockets) still runs on early return and after the exchange (use try/finally around exchangeCodeForToken if needed).
278-284:⚠️ Potential issue | 🟠 MajorValidate
token_typebefore returning a bearer credential.This path accepts any non-empty
access_tokenand always maps it to{ type: 'bearer' }. If the provider omitstoken_typeor returns a different scheme, downstream requests will send the wrongAuthorizationheader.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/resolve-oauth.ts` around lines 278 - 284, Validate the OAuth token_type before mapping to a bearer credential: in the resolve-oauth logic that inspects `record` (the block returning `{ token: record.access_token, type: 'bearer' }`), check `record.token_type` (case-insensitive) and only return `{ token: record.access_token, type: 'bearer' }` when `token_type` exists and equals "bearer" (or default to "bearer" only if the provider explicitly indicates it); otherwise return null or map to the appropriate scheme. Ensure you still verify `access_token` is a non-empty string and use the `token_type` check to avoid always assuming bearer.
184-189:⚠️ Potential issue | 🟠 MajorResolve provider error callbacks instead of timing out.
A state-matched callback like
?error=access_deniedstill returns400without resolving the deferred. Callers then sit until the timeout expires and report the wrong failure mode.Also applies to: 220-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/resolve-oauth.ts` around lines 184 - 189, When extractCodeFromUrl(req.url, callbackPath, expectedState) returns no code, also check for a provider error in the callback (e.g., parse req.url for an "error" param and ensure the state matches expectedState); if present, respond to the HTTP request (res.writeHead/res.end) and resolve the OAuth deferred/promise with a provider-error result (do not leave the deferred pending); apply the same fix to the other callback handling block later (the block around the handling at lines 220-230) so provider error callbacks like "?error=access_denied" immediately resolve the deferred instead of timing out.research/auth/pkce-overview.md (1)
28-28:⚠️ Potential issue | 🟡 MinorType the anonymous flow and request fences.
These three opening fences still trigger MD040.
textfits the ASCII flow, andhttportextfits the request snippets.Also applies to: 82-82, 94-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@research/auth/pkce-overview.md` at line 28, Update the three code fences that are currently anonymous (empty language) to include explicit language tags so they no longer trigger MD040: change the ASCII/anonymous flow fences to use "text" and change the request/HTTP snippet fences to use "http" (or "text" if preferred); locate the three opening fences in the pkce-overview.md that are currently empty and add the appropriate language tag for each.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/middleware/auth/resolve-device-code.ts`:
- Around line 337-339: The current success branch in resolve-device-code.ts
turns any non-empty record.access_token into a bearer credential without
checking record.token_type; update the logic in the block that checks
response.ok (the branch returning { credential: { token: record.access_token,
type: 'bearer' }, status: 'success' }) to validate that record.token_type exists
and equals 'bearer' (case-insensitive) before returning a bearer credential, and
if token_type is missing or not a bearer scheme, return an appropriate
non-success status (or error) instead so callers don't send the wrong
Authorization header.
In `@packages/core/src/middleware/auth/resolve-oauth.test.ts`:
- Around line 33-44: The test helper extractPort assumes the browser URL is at
call[1][0], which breaks on Windows where openBrowser() invokes cmd /c start ''
<url>; update extractPort to locate the real URL argument instead of using a
fixed index: inspect the mocked execFile call arguments
(vi.mocked(execFile).mock.calls -> call[1]) and search that array for the first
string that looks like the browser URL (e.g., starts with "http" or contains
"redirect_uri"), then parse that string with new URL(...) to extract
redirect_uri and its port; apply the same change to the other helper(s)
mentioned around lines 82-85 so both are platform-agnostic.
---
Duplicate comments:
In `@packages/core/src/middleware/auth/resolve-device-code.ts`:
- Around line 58-60: The device-code resolver is always calling openBrowser(),
which causes failures in SSH/CI/container environments; update the
resolve-device-code flow (around the displayUserCode(...) / openBrowser(...)
calls in the resolveDeviceCode function) to only call openBrowser when a
caller-controlled flag is true (e.g., options.openBrowser or
options.launchBrowser) — default behavior should keep it disabled for
non-interactive environments; check the flag (e.g., if (options.openBrowser)
openBrowser(...)) and do not call openBrowser unconditionally.
- Around line 48-52: The timeout budget is being started too late and fetches
are uncancelable; move creation of the deadline/AbortController to the very
start of the device-code flow so it covers requestDeviceAuth, displayUserCode,
and polling, pass the controller.signal into requestDeviceAuth and any internal
fetches (and into pollForToken or equivalent), and modify displayUserCode to
accept and respect an AbortSignal (or return early when aborted) so that slow
endpoints or a waiting prompt are cancelled when options.timeout elapses; update
calls to requestDeviceAuth, displayUserCode, and pollForToken to use the shared
AbortSignal and ensure any pending timers/fetches are cleaned up on abort.
- Around line 61-62: The code uses resolveInterval(authResponse.interval,
options.pollInterval) and then deadline = Date.now() + options.timeout without
validating inputs, allowing 0, negative, NaN, or Infinity to collapse polling;
update resolveInterval (and any other call sites using authResponse.interval or
options.pollInterval) to sanitize both values: coerce to a finite number, reject
NaN/Infinity, enforce a minimum positive integer (e.g., >= 1s) and an upper
bound, and fall back to a safe default (or options.timeout) when invalid; ensure
polling loop and deadline calculation use the validated interval and timeout so
you never schedule immediate tight loops.
In `@packages/core/src/middleware/auth/resolve-oauth.ts`:
- Around line 86-103: The timeout is cleared before calling
exchangeCodeForToken, which allows a slow token endpoint to hang past the
documented timeout; instead keep the timeout active through the token exchange
by removing the early timeout.clear() and only clearing the timeout after the
exchange completes (or by wrapping exchangeCodeForToken in a Promise.race with
timeoutPromise and clearing in a finally block). Update the flow around
codeDeferred/timeoutPromise and exchangeCodeForToken so timeout.clear() is
invoked after the token exchange resolves/rejects, and ensure
destroyServer(handle.server, handle.sockets) still runs on early return and
after the exchange (use try/finally around exchangeCodeForToken if needed).
- Around line 278-284: Validate the OAuth token_type before mapping to a bearer
credential: in the resolve-oauth logic that inspects `record` (the block
returning `{ token: record.access_token, type: 'bearer' }`), check
`record.token_type` (case-insensitive) and only return `{ token:
record.access_token, type: 'bearer' }` when `token_type` exists and equals
"bearer" (or default to "bearer" only if the provider explicitly indicates it);
otherwise return null or map to the appropriate scheme. Ensure you still verify
`access_token` is a non-empty string and use the `token_type` check to avoid
always assuming bearer.
- Around line 184-189: When extractCodeFromUrl(req.url, callbackPath,
expectedState) returns no code, also check for a provider error in the callback
(e.g., parse req.url for an "error" param and ensure the state matches
expectedState); if present, respond to the HTTP request (res.writeHead/res.end)
and resolve the OAuth deferred/promise with a provider-error result (do not
leave the deferred pending); apply the same fix to the other callback handling
block later (the block around the handling at lines 220-230) so provider error
callbacks like "?error=access_denied" immediately resolve the deferred instead
of timing out.
In `@research/auth/device-code-overview.md`:
- Line 13: The fenced code block at the indicated spot is anonymous and triggers
MD040; add an info string (e.g., change the opening fence from ``` to ```text)
for the fenced flow block in research/auth/device-code-overview.md so the block
is no longer anonymous and the linter stops flagging it.
In `@research/auth/pkce-overview.md`:
- Line 28: Update the three code fences that are currently anonymous (empty
language) to include explicit language tags so they no longer trigger MD040:
change the ASCII/anonymous flow fences to use "text" and change the request/HTTP
snippet fences to use "http" (or "text" if preferred); locate the three opening
fences in the pkce-overview.md that are currently empty and add the appropriate
language tag for each.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 490976d7-be5d-433a-94ea-bf32a416250d
📒 Files selected for processing (25)
.changeset/config.json.changeset/oauth-pkce-device-code.mddocs/concepts/authentication.mddocs/guides/add-authentication.mdexamples/authenticated-service/cli/src/index.tspackages/core/src/middleware/auth/README.mdpackages/core/src/middleware/auth/constants.tspackages/core/src/middleware/auth/index.tspackages/core/src/middleware/auth/oauth-shared.test.tspackages/core/src/middleware/auth/oauth-shared.tspackages/core/src/middleware/auth/resolve-credentials.test.tspackages/core/src/middleware/auth/resolve-credentials.tspackages/core/src/middleware/auth/resolve-device-code.test.tspackages/core/src/middleware/auth/resolve-device-code.tspackages/core/src/middleware/auth/resolve-oauth.test.tspackages/core/src/middleware/auth/resolve-oauth.tspackages/core/src/middleware/auth/types.tspackages/core/test/helpers/mock-oauth-server.tspackages/core/test/integration/auth-device-code.test.tspackages/core/test/integration/auth-http-chain.test.tspackages/core/test/integration/auth-oauth-pkce.test.tsresearch/auth/clerk-pkce.mdresearch/auth/current-implementation.mdresearch/auth/device-code-overview.mdresearch/auth/pkce-overview.md
✅ Files skipped from review due to trivial changes (1)
- .changeset/config.json
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/core/src/middleware/auth/index.ts
- packages/core/test/integration/auth-device-code.test.ts
- packages/core/src/middleware/auth/resolve-device-code.test.ts
- examples/authenticated-service/cli/src/index.ts
- packages/core/test/integration/auth-http-chain.test.ts
- packages/core/src/middleware/auth/resolve-credentials.ts
- packages/core/src/middleware/auth/oauth-shared.ts
- packages/core/src/middleware/auth/oauth-shared.test.ts
Add /authorize, /authorize/grant, and /token endpoints to the
authenticated-service API server to support the PKCE authorization
code flow. Add standalone http() middleware with dynamic headers
and a ping command to the advanced example. Update READMEs to
reflect the new auth({ http }) pattern and resolver builders.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/core/src/middleware/http/http.test.ts (1)
80-115: Consider verifying resolved headers are passed to the HTTP client.The tests verify that the headers function is called with the context, but don't assert that the returned headers are actually propagated to the HTTP client. This could be verified by mocking
createHttpClientand checking thedefaultHeadersargument.💡 Example test enhancement
import { createHttpClient } from './create-http-client.js' vi.mock('./create-http-client.js', () => ({ createHttpClient: vi.fn(() => ({ get: vi.fn(), post: vi.fn(), put: vi.fn(), patch: vi.fn(), delete: vi.fn(), })), })) it('should pass resolved headers to createHttpClient', async () => { const ctx = createMockCtx() const headers = { 'X-Custom': 'value' } const mw = http({ baseUrl: 'https://api.example.com', headers: () => headers, namespace: 'api', }) await mw.handler(ctx as never, vi.fn()) expect(createHttpClient).toHaveBeenCalledWith({ baseUrl: 'https://api.example.com', defaultHeaders: headers, }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/http/http.test.ts` around lines 80 - 115, The test should assert that resolved headers are actually passed into the HTTP client: mock createHttpClient (from create-http-client.js), call the middleware created by http(...) with a headers function (e.g., headersFn) that returns a headers object, invoke mw.handler(ctx, next), and assert createHttpClient was called with { baseUrl: 'https://api.example.com', defaultHeaders: <the-resolved-headers> } so the middleware's use of defaultHeaders is verified.examples/advanced/src/index.ts (1)
27-34: Reconcileconfig.apiUrlwith the newctx.apiclient.
http()now hardcodeshttps://api.acme.dev, so the requiredapiUrlsetting no longer affects wherectx.apisends requests. Either feed that config into client construction, or rename/remove the setting if it is no longer meant to control the bound client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/advanced/src/index.ts` around lines 27 - 34, The http client registration currently hardcodes 'https://api.acme.dev' which ignores the configured apiUrl; update the http(...) call that builds the ctx.api client to use the configured value (e.g. ctx.config.apiUrl or config.apiUrl) for baseUrl instead of the hardcoded string, or if apiUrl is no longer intended to control the client remove/rename the config setting and any references to config.apiUrl; ensure you update the http(...) call and any initialization logic that expects ctx.api to target the configured endpoint so behavior is consistent with the config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/authenticated-service/api/server.ts`:
- Around line 217-250: The HTML builder is directly interpolating
user-controlled values (clientId, redirectUri, codeChallenge, state) into
HTML/JS which allows XSS; update the code that constructs the HTML (the block
that defines html array and the inline authorize() script) to escape values
before embedding: use an HTML-escaping helper for values rendered into content
(e.g., the <p> with clientId) and use safe JS serialization (e.g.,
JSON.stringify or a JS-string-escaping helper) when inserting values into the
inline script and attributes (the authorize button onclick and
window.location.href construction) so that special characters cannot break out
of the string context.
In `@examples/authenticated-service/README.md`:
- Around line 7-12: Update the README.md fenced directory-tree block to specify
a language to satisfy markdownlint MD040: change the opening ``` to ```text (so
the block becomes ```text followed by the directory tree and closing ```).
Target the code fence in the authenticated-service README directory-tree block
(the triple-backtick fenced block that lists "authenticated-service/ api/ cli/
ui/") and only modify the opening fence to include "text"/"plaintext".
In `@packages/core/src/middleware/auth/auth.ts`:
- Around line 90-102: The current code captures authContext.credential() once
when creating clients (in the options.http handling around normalizeHttpOptions,
resolveCredentialHeaders, createHttpClient, decorateContext), causing stale
tokens after ctx.auth.authenticate(); change client creation to resolve headers
lazily per request instead of baking defaultHeaders: implement the HTTP client
with a request-time header resolver that calls authContext.credential() (or
resolveCredentialHeaders(credential())) each call, or rebind/decorate the client
after authenticate; ensure the code paths that call
createHttpClient/decorateContext use this lazy header resolution so
ctx[namespace] always sends the current token.
- Around line 148-150: resolvePassive currently only checks file/env and ignores
configured dotenv resolvers so auth.dotenv() never participates; update
resolvePassive to iterate over the configured resolvers array in order and
dispatch passive resolution for each (including dotenv) rather than hard-coding
file/env, and ensure buildDotenv/DotenvSourceConfig is treated the same way as
other resolvers when invoked passively; specifically, in the resolvePassive
implementation call each resolver's passive path (including the dotenv resolver
produced by buildDotenv) in order and merge/short-circuit results according to
existing passive semantics so dotenv can be preferred via passive ordering.
In `@packages/core/src/middleware/http/README.md`:
- Around line 40-47: The example introduces an undefined ctx.vault which will
not type-check; update the http(...) example so headers() uses a documented/real
context property or a static header. Replace the Authorization line to reference
an actual context field used in this README (e.g., ctx.state.token or
ctx.correlationId) or show a non-auth example like a static 'X-Request-ID'
header; ensure the example still uses the http() call and the headers: (ctx) =>
(...) function shape.
---
Nitpick comments:
In `@examples/advanced/src/index.ts`:
- Around line 27-34: The http client registration currently hardcodes
'https://api.acme.dev' which ignores the configured apiUrl; update the http(...)
call that builds the ctx.api client to use the configured value (e.g.
ctx.config.apiUrl or config.apiUrl) for baseUrl instead of the hardcoded string,
or if apiUrl is no longer intended to control the client remove/rename the
config setting and any references to config.apiUrl; ensure you update the
http(...) call and any initialization logic that expects ctx.api to target the
configured endpoint so behavior is consistent with the config.
In `@packages/core/src/middleware/http/http.test.ts`:
- Around line 80-115: The test should assert that resolved headers are actually
passed into the HTTP client: mock createHttpClient (from create-http-client.js),
call the middleware created by http(...) with a headers function (e.g.,
headersFn) that returns a headers object, invoke mw.handler(ctx, next), and
assert createHttpClient was called with { baseUrl: 'https://api.example.com',
defaultHeaders: <the-resolved-headers> } so the middleware's use of
defaultHeaders is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f339388a-09c2-4703-adeb-d2cdba6dab17
📒 Files selected for processing (19)
.changeset/auth-http-redesign.md.changeset/oauth-pkce-device-code.mddocs/concepts/authentication.mdexamples/advanced/README.mdexamples/advanced/src/commands/ping.tsexamples/advanced/src/index.tsexamples/authenticated-service/README.mdexamples/authenticated-service/api/server.tsexamples/authenticated-service/cli/src/index.tspackages/core/src/middleware/auth/README.mdpackages/core/src/middleware/auth/auth.test.tspackages/core/src/middleware/auth/auth.tspackages/core/src/middleware/auth/index.tspackages/core/src/middleware/auth/types.tspackages/core/src/middleware/http/README.mdpackages/core/src/middleware/http/http.test.tspackages/core/src/middleware/http/http.tspackages/core/src/middleware/http/types.tspackages/core/test/integration/auth-http-chain.test.ts
✅ Files skipped from review due to trivial changes (1)
- examples/advanced/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/test/integration/auth-http-chain.test.ts
- packages/core/src/middleware/auth/types.ts
- packages/core/src/middleware/auth/index.ts
- examples/authenticated-service/cli/src/index.ts
…iKey alias
Rename the prompt resolver builder from `auth.prompt()` to `auth.token()`
to better describe what is being requested rather than the mechanism.
Add `auth.apiKey()` as an alias that points to the same builder.
The source discriminator changes from `'prompt'` to `'token'`. Raw config
objects should use `{ source: 'token' }`.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/middleware/auth/auth.ts (1)
262-281:⚠️ Potential issue | 🟠 Major
auth.dotenv()never participates in passive resolution.
resolvePassive()only invokesresolveFromFileandresolveFromEnv, ignoring any configureddotenvresolvers. This means credentials from.envfiles viaauth.dotenv()will not be resolved when the middleware initializes, even thoughdotenvis non-interactive. Credentials will only be discovered ifauthenticate()is called explicitly.Consider iterating over the configured resolvers in order and dispatching passive resolution for each applicable source type (including
dotenv).🐛 Suggested approach
function resolvePassive( cliName: string, resolvers: readonly ResolverConfig[] ): AuthCredential | null { - const fileConfig = findResolverBySource(resolvers, 'file') - const envConfig = findResolverBySource(resolvers, 'env') - - const fromFile = resolveFromFile({ - dirName: resolveFileDir(fileConfig, cliName), - filename: resolveFileFilename(fileConfig), - }) - - if (fromFile) { - return fromFile - } - - return resolveFromEnv({ - tokenVar: resolveEnvTokenVar(envConfig, cliName), - }) + // Iterate resolvers in order, resolve passive sources + for (const resolver of resolvers) { + const credential = resolvePassiveSource(resolver, cliName) + if (credential !== null) { + return credential + } + } + return null }Note: This would require adding a
resolvePassiveSourcedispatcher that handlesfile,env, anddotenvsources, and aresolveFromDotenvimplementation (which already exists atresolve-dotenv.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/auth.ts` around lines 262 - 281, resolvePassive currently only checks file and env sources (resolveFromFile, resolveFromEnv) so auth.dotenv() never participates; update resolvePassive to iterate the provided resolvers in order and dispatch passive resolution per resolver.source (create a small dispatcher function resolvePassiveSource that calls resolveFromFile, resolveFromEnv, or resolveFromDotenv as appropriate), using existing helpers like findResolverBySource only if needed, and return the first non-null AuthCredential found; ensure you call the existing resolveFromDotenv implementation when the resolver source is 'dotenv'.
♻️ Duplicate comments (2)
examples/authenticated-service/README.md (1)
7-12:⚠️ Potential issue | 🟡 MinorAdd a language specifier to the directory-tree fence.
The code fence still lacks a language identifier, which triggers markdownlint MD040.
📝 Suggested fix
-``` +```text authenticated-service/ api/ # Faux API server (bearer token validation, PKCE OAuth) cli/ # kidd CLI with auth middleware (includes HTTP client) ui/ # Browser dashboard for testing the API🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/authenticated-service/README.md` around lines 7 - 12, The Markdown code fence in the README for the directory tree (the block that starts with ``` and lists "authenticated-service/ api/ cli/ ui/") lacks a language identifier and triggers markdownlint MD040; update that fence to include a language specifier (e.g., change ``` to ```text) so the fence is fenced with a language and linting passes while leaving the directory contents unchanged.packages/core/src/middleware/auth/README.md (1)
145-145:⚠️ Potential issue | 🟡 MinorPhrase provider support as capability, not a short allowlist.
The resolver is endpoint-driven, so this sentence may become stale. Consider capability-oriented wording.
📝 Suggested wording
-Supported by GitHub, Azure AD, and Google. Not supported by Clerk. +Works with providers that implement RFC 8628, including GitHub, Azure AD, Google, Auth0, and Okta. Clerk does not currently expose a device authorization endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/auth/README.md` at line 145, The README line "Supported by GitHub, Azure AD, and Google. Not supported by Clerk." is endpoint-oriented and may become stale; update the wording in packages/core/src/middleware/auth/README.md to describe capability support instead (e.g., "Supports OAuth/OIDC identity providers; consult provider docs for specific integrations" or similar), replacing the short allowlist sentence with capability-oriented phrasing so the docs remain accurate as endpoints change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/middleware/auth/auth.ts`:
- Around line 91-104: The HTTP clients created in the middleware use a one-time
resolved credential (authContext.credential() / resolveCredentialHeaders) and
bake those into createHttpClient defaultHeaders, causing stale tokens after
ctx.auth.authenticate(); change createHttpClient/CreateHttpClientOptions to
accept a header/credential factory (e.g. headers?: () =>
Readonly<Record<string,string>> or a credential resolver) and update
executeRequest to call that factory each request so decorateContext / the
clients returned for each namespace resolve headers lazily rather than using the
single authContext.credential() value captured at initialization.
---
Outside diff comments:
In `@packages/core/src/middleware/auth/auth.ts`:
- Around line 262-281: resolvePassive currently only checks file and env sources
(resolveFromFile, resolveFromEnv) so auth.dotenv() never participates; update
resolvePassive to iterate the provided resolvers in order and dispatch passive
resolution per resolver.source (create a small dispatcher function
resolvePassiveSource that calls resolveFromFile, resolveFromEnv, or
resolveFromDotenv as appropriate), using existing helpers like
findResolverBySource only if needed, and return the first non-null
AuthCredential found; ensure you call the existing resolveFromDotenv
implementation when the resolver source is 'dotenv'.
---
Duplicate comments:
In `@examples/authenticated-service/README.md`:
- Around line 7-12: The Markdown code fence in the README for the directory tree
(the block that starts with ``` and lists "authenticated-service/ api/ cli/
ui/") lacks a language identifier and triggers markdownlint MD040; update that
fence to include a language specifier (e.g., change ``` to ```text) so the fence
is fenced with a language and linting passes while leaving the directory
contents unchanged.
In `@packages/core/src/middleware/auth/README.md`:
- Line 145: The README line "Supported by GitHub, Azure AD, and Google. Not
supported by Clerk." is endpoint-oriented and may become stale; update the
wording in packages/core/src/middleware/auth/README.md to describe capability
support instead (e.g., "Supports OAuth/OIDC identity providers; consult provider
docs for specific integrations" or similar), replacing the short allowlist
sentence with capability-oriented phrasing so the docs remain accurate as
endpoints change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae04e420-5f8b-434b-b159-44ccc950ebbc
📒 Files selected for processing (13)
.changeset/auth-http-redesign.mddocs/concepts/authentication.mddocs/guides/add-authentication.mdexamples/authenticated-service/README.mdexamples/authenticated-service/cli/src/index.tspackages/core/src/middleware/auth/README.mdpackages/core/src/middleware/auth/auth.test.tspackages/core/src/middleware/auth/auth.tspackages/core/src/middleware/auth/create-auth-context.test.tspackages/core/src/middleware/auth/index.tspackages/core/src/middleware/auth/resolve-credentials.test.tspackages/core/src/middleware/auth/resolve-credentials.tspackages/core/src/middleware/auth/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/middleware/auth/index.ts
Fix broken lifecycle.md links in contributing/concepts/ that pointed to a non-existent file (should reference docs/concepts/lifecycle.md). Update CLAUDE.md test file glob to match actual colocated test pattern, add type-fest to tech stack. Align testing.md integration test directory example with real codebase (test/integration/ not __tests__/). Add undocumented Crust color to diagrams.md palette table and clarify classDef vs inline style guidance for subgraphs. Update changesets to cover output/prompts removal, utils fs export, and CLI command refactoring. Co-Authored-By: Claude <noreply@anthropic.com>
…ddleware - Include dotenv resolver in passive credential resolution alongside file and env resolvers (was silently ignored) - Detect OAuth error redirects (e.g. ?error=access_denied) and resolve immediately instead of waiting for timeout expiry - Add AbortSignal.timeout to device-code HTTP requests so hung endpoints cannot exceed the configured timeout - Add openBrowser option to device-code resolver (defaults true) for headless/CI/SSH environments - Validate client_id on device-code token polls in mock server - Add expectedVerifier option to mock server for PKCE verification - Replace ctx.vault reference with ctx.meta.name in HTTP README - Escape user-controlled HTML values in example server authorize page - Use platform-agnostic URL extraction in OAuth test helpers - Rephrase device-code provider support as RFC 8628 capability - Add language tag to example README directory tree fence Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Mar 10, 2026
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
1 participant
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
Replace the non-standard direct-token-POST OAuth flow with spec-compliant implementations:
oauthresolver with a standard PKCE flow that works with any OAuth 2.0 provider (including Clerk)device-coderesolver for headless/browserless environments (SSH, CI, containers)Changes
resolve-oauth.tswith a new PKCE implementation (GET redirect callback, code exchange at token endpoint withcode_verifier)resolve-device-code.tsimplementing RFC 8628 (device auth request, user code display, token endpoint polling withauthorization_pending/slow_down/expired_token/access_deniedhandling)oauth-shared.ts(deferred promises, timeouts, local server infra, browser launching)types.ts--OAuthSourceConfignow requiresclientIdandtokenUrl; addedDeviceCodeSourceConfigconstants.tswith defaults for both flowsresolve-credentials.tsdispatcher with new.with()branchesauthentication.md,add-authentication.md,README.md)Breaking Change
OAuthSourceConfignow requiresclientIdandtokenUrl:Testing
Key test files:
oauth-shared.test.ts(13 tests) -- shared utilitiesresolve-oauth.test.ts(13 tests) -- PKCE resolver unit testsresolve-device-code.test.ts(16 tests) -- device code resolver unit testsresolve-credentials.test.ts(21 tests) -- dispatcher integrationtest/integration/auth-oauth-pkce.test.ts(10 tests) -- PKCE e2e with real mock OAuth servertest/integration/auth-device-code.test.ts(11 tests) -- device code e2e with real mock servertest/integration/auth-http-chain.test.ts(8 tests) -- full auth → HTTP middleware chainSummary by CodeRabbit
New Features
Breaking / Configuration
Documentation
Tests