Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…E_VERSION__ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes behavioral gaps between the axios-based and fetch-based (feaxios) HTTP client implementations, specifically restoring enforcement of SSRF/DoS guardrails (maxRedirects, maxContentLength) on the no-axios path and adding a cross-build contract test suite to prevent regressions.
Changes:
- Add a bounded fetch adapter that enforces
maxRedirects(manual hop counting) andmaxContentLength(streamed read with early abort), plus cancellation tagging forisCancel. - Replace runtime
package.jsonimport with the build-time__PACKAGE_VERSION__define and fix browser DefinePlugin version injection. - Add a no-axios Vitest config + test harness routing (
USE_AXIOS=false) and introduce an HttpClient contract test suite run against both builds.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/http_client_contract.test.ts | New Node-only contract tests covering request/response shaping, errors, cancellation, redirect/size bounds, and interceptors. |
| test/tsconfig.json | Loosens TS strictness for tests and normalizes include/exclude formatting. |
| test/test-utils/stellar-sdk-import.ts | Routes Node tests to lib/no-axios when USE_AXIOS=false so tests exercise the fetch build output. |
| src/http-client/fetch-client.ts | Implements bounded fetch path for redirect/content-length enforcement and improves cancellation identification. |
| src/bindings/config.ts | Uses __PACKAGE_VERSION__ instead of importing package.json to support deeper build outputs. |
| package.json | Adds test:node:no-axios and runs it as part of test/test:all. |
| config/webpack.config.browser.js | Fixes __PACKAGE_VERSION__ injection by stringifying the version in DefinePlugin. |
| config/vitest.config.no-axios.ts | Adds a dedicated Vitest config for running unit tests against the no-axios build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Size Change: +397 kB (+0.87%) Total Size: 46.1 MB 📦 View Changed
|
…bortSignal fallback, harness error surface Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e under test Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| const controller = new AbortController(); | ||
| setTimeout(() => controller.abort(new Error("TimeoutError")), ms); | ||
| return controller.signal; | ||
| } |
There was a problem hiding this comment.
In the fallback path (when AbortSignal.timeout isn't available), controller.abort(new Error("TimeoutError")) usually makes fetch reject with an AbortError (or another error whose name isn't TimeoutError), so the boundedFetchAdapter catch block won't translate it into the axios-style timeout of Nms exceeded. Consider aborting with a DOMException named TimeoutError (or setting reason.name = "TimeoutError"), and/or checking signal.reason when mapping to a timeout error.
| "removeComments": false, | ||
| "strict": true, | ||
| "noImplicitAny": true, | ||
| "noImplicitAny": false, |
There was a problem hiding this comment.
Disabling noImplicitAny for the entire test project reduces type-safety and can mask mistakes across the suite. If this was done to accommodate a few tests, consider keeping it enabled and using local casts or targeted @ts-expect-error where needed.
| "noImplicitAny": false, | |
| "noImplicitAny": true, |
|
|
||
|
|
There was a problem hiding this comment.
There are multiple extra blank lines before the ### Deprecated section; consider removing them to keep the changelog formatting consistent.
…for cross-build coverage Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e, redirect auth, responseType, transforms Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… defaults merge, responseType, transforms, auth-strip Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…bounded options Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Why
feaxiosignores maxRedirects and maxContentLength, silently turning SDK-set SSRF and DoS guards into no-ops.What
timeout, cancellation, bounded-path redirect/size enforcement, validateStatus, fetchOptions override-resistance, interceptors, and create() defaults. Runs against both builds.