refactor: migrate axios → Node built-in fetch#26
Merged
Conversation
Drop the axios runtime dependency in favor of the standard library.
Node 24 supplies fetch, Response, Headers, and AbortSignal.timeout — no
behavior change, smaller surface.
- session-grant-client: throw-on-non-2xx replaced with explicit resp.ok
check. Network errors (TypeError, AbortError) map to provider_unavailable
in a single catch. Retry-After lookup now uses Headers.get() which is
case-insensitive, so the Title-case test still passes without extra code.
- validation/introspect: added IntrospectHttpError so the router can branch
on HTTP status without peeking at a framework-specific error shape.
- validation/router: replaced axios.isAxiosError with instanceof
IntrospectHttpError.
- Tests: replaced vi.mock("axios") with vi.stubGlobal("fetch", vi.fn())
across the three affected suites. Two new tests added for the new error
paths (non-JSON 200 body, AbortError timeout).
tests: 86 → 88 passing. typecheck / lint / build clean.
Obsoletes dependabot PR #19 (axios 1.8 → 1.13 bump).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses Codex review finding on the axios→fetch migration. Before migration, axios 1.x with default silentJSONParsing=true swallowed JSON.parse SyntaxError on a 200 response with a malformed body, returning the raw string. The router's `if (!result.active)` then evaluated `!"...".active` → truthy, mapping the case to 401 Invalid Token — an accidental side effect, not an intentional contract. After migration, `resp.json()` throws on malformed bodies, which the router caught but mapped to 500 (fall-through path). Explicitly codify the new contract: a 200 with a non-JSON body is a provider bug, not an auth decision. Wrap the json() call and rethrow as IntrospectHttpError(502). This mirrors session-grant-client's provider_invalid_response (502) handling and makes provider bugs observable rather than silently degrading to an auth-layer response. Also add validation tests that were missing for the new fetch error paths: - non-JSON 200 → IntrospectHttpError(502) - non-2xx → IntrospectHttpError with matching status - fetch rejection (AbortError/network) propagates to caller tests: 88 → 91 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Refactors the auth proxy’s HTTP client code to drop the axios runtime dependency and use Node’s built-in fetch/Response/Headers APIs, while codifying stricter handling for malformed 200 responses from the introspection provider.
Changes:
- Migrates validation introspection and session-grant exchange flows from axios to
fetch, including timeout behavior viaAbortSignal.timeout. - Introduces
IntrospectHttpErrorand updates the validation router to interpret introspection failures without relying on axios-specific error typing. - Rewrites unit/integration tests to stub
globalThis.fetchinstead of mocking the axios module; removesaxiosfrom dependencies.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/modes/validation/router.mts | Switches error handling from axios.isAxiosError to IntrospectHttpError checks. |
| src/modes/validation/introspect.mts | Reimplements introspection HTTP call using fetch, adds IntrospectHttpError, and codifies “200 non-JSON body” as an error. |
| src/modes/validation/tests/introspect.test.mts | Updates tests to stub fetch and adds coverage for non-JSON 200 and non-2xx statuses. |
| src/modes/injection/session-grant-client.mts | Replaces axios with fetch, unifies network/timeout handling, and parses JSON responses explicitly. |
| src/modes/injection/tests/session-grant-client.test.mts | Updates tests to stub fetch and adds cases for non-JSON 200 and AbortError timeout. |
| src/modes/injection/tests/router.test.mts | Updates injection router integration tests to use fetch stubbing for provider calls. |
| package.json | Removes the axios dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes an auth bypass reported by Copilot on PR #26. resp.json() successfully parses non-object JSON (strings, numbers, arrays, null) and objects where `active` is not a boolean. The router only checked `if (!result.active)`, so a provider returning `{"active":"false"}` would be truthy → bypass the 401 branch → forward an invalid token to upstream. RFC 7662 §2.2 defines `active` as a boolean. Reject any deviation at introspect boundary by throwing IntrospectHttpError(502), which the router already maps to 500. This closes the bypass regardless of the parsed shape: - JSON primitives (string / number / null) → 502 - JSON arrays → 502 - Objects missing `active` → 502 - Objects where `active` is not a boolean (e.g. "false", 1, null) → 502 Only `{"active": <boolean>, ...}` is accepted as a valid introspection response. The `[key: string]: unknown` extra fields remain permissive. The bug existed under axios too (same falsy-check pattern), but this PR touches the introspection path, so per CLAUDE.md it is in scope. tests: 91 → 99 passing. 8 new parameterized cases covering the Critical shapes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Three fixes surfaced by the second Copilot review after the RFC 7662 validation commit: 1. session-grant-client: error message for malformed 200 said "non-JSON body", but parseJsonBody() also returns null for valid JSON that is not a plain object (string / number / array). Update to "non-JSON or non-object JSON body" so provider debugging is accurate. 2. AbortSignal.timeout coverage (session-grant-client + introspect): the timeout tests only asserted that init.signal was an AbortSignal instance, which would hold even if the code silently ignored the configured timeoutMs. Wrap both tests with vi.spyOn(AbortSignal, "timeout") and assert the call received 1234. vi.restoreAllMocks() added to each afterEach to clean up the spy. This point converged with Codex Minor 3 from the original review round (CLAUDE.md multi-reviewer convergence rule), auto-promoting from Minor to Important. tests: 99 passing (no new tests; two existing tests strengthened). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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
Drop the axios runtime dependency in favor of Node 24's built-in
fetch,Response,Headers, andAbortSignal.timeout. No new features — smaller surface, one less third-party dependency.Three production files migrated:
src/modes/injection/session-grant-client.mtssrc/modes/validation/introspect.mtssrc/modes/validation/router.mtsThree test files rewritten to stub
globalThis.fetchinstead of mocking the axios module.Semantic change (codified in this PR)
axios 1.x defaulted
silentJSONParsing: true, which silently swallowedJSON.parseerrors on 200 responses with malformed bodies and returned the raw string asdata. For the introspection path this accidentally routed "provider returned 200 with a non-JSON body" →401 Invalid Token(because!"...".activeis truthy).This PR codifies the new, intentional behavior: a 200 with a non-JSON body is a provider bug, not an auth decision. It now raises
IntrospectHttpError(502), mirroringsession-grant-client'sprovider_invalid_response (502)handling. This makes provider bugs observable instead of silently degrading them to a 401.If any deployment was relying on the accidental "malformed 200 → 401" behavior, this will surface as a 500 from the proxy instead. The old behavior was not documented and is not a contract we want to keep.
Error-handling parity
AxiosErrorwith noresponseTypeErrorcode: ECONNABORTEDDOMException("AbortError")AxiosErrorwithresponse.statusResponsewithresp.ok === falseh["retry-after"] ?? h["Retry-After"]Headers.get()handles casesession-grant-client's single
catchnow handles both network errors and timeouts uniformly (both map toprovider_unavailable), which is an improvement over theisAxiosErrorbranching.Test changes
Replaced
vi.mock("axios")withvi.stubGlobal("fetch", vi.fn())across:src/modes/injection/__tests__/session-grant-client.test.mtssrc/modes/validation/__tests__/introspect.test.mtssrc/modes/injection/__tests__/router.test.mtsAdded tests for the new/codified error paths:
IntrospectHttpError(502), non-2xx →IntrospectHttpError, fetch rejection propagationexpress-http-proxyuses Node'shttpmodule directly (not fetch), so stubbingglobalThis.fetchin the injection router integration test does not interfere with the real upstream recorder.Test count: 86 → 91 passing.
Follow-ups
Obsoletes dependabot #19 (axios 1.8 → 1.13). Will close after merge.
Test plan
pnpm typecheckcleanpnpm lintcleanpnpm test— 91/91 passingpnpm buildclean864a0b8)🤖 Generated with Claude Code