Remove axios dep#466
Conversation
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR successfully removes the axios dependency and replaces it with the built-in fetch API, but introduces several correctness issues: OAuth credentials are sent as URL query parameters instead of in the POST body (a spec violation that can also leak secrets into access logs), the response body is consumed before the status check (making error body capture unreliable), and _pollUntilDone can silently return undefined on timeout/failure causing a downstream null-dereference. Additionally, _fetch/_fetchWithAgent are copy-pasted into two files rather than shared, creating a maintenance burden.
PR Bot Information
Version: 1.22.9
- Event Trigger:
pull_request.opened - File Content Strategy: Full file content
- Correlation ID:
d2199f26-87b9-414b-a48f-468a1180a244 - LLM:
anthropic--claude-4.6-sonnet
| const params = new URLSearchParams({ | ||
| grant_type: "client_credentials", | ||
| client_id: clientid, | ||
| client_secret: clientsecret, | ||
| }) | ||
| const response = await _fetch(`${url}/oauth/token?${params}`, { | ||
| method: "POST", | ||
| headers, | ||
| params: { | ||
| grant_type: "client_credentials", | ||
| client_id: clientid, | ||
| client_secret: clientsecret, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Bug: OAuth token sent as query parameter instead of request body
The client credentials (grant_type, client_id, client_secret) are appended to the URL as query string parameters. The OAuth 2.0 spec (RFC 6749) requires these to be sent in the request body with Content-Type: application/x-www-form-urlencoded, not as URL query parameters. Some authorization servers will reject this or log credentials in access logs, creating a security concern.
The old axios implementation also used params (which axios sent as query string for POST), so this is a pre-existing behavioral issue—but it is explicitly worsened here by making it visible. The Content-Type header already declares application/x-www-form-urlencoded, so the body should carry the payload.
Consider sending the credentials in the request body instead:
| const params = new URLSearchParams({ | |
| grant_type: "client_credentials", | |
| client_id: clientid, | |
| client_secret: clientsecret, | |
| }) | |
| const response = await _fetch(`${url}/oauth/token?${params}`, { | |
| method: "POST", | |
| headers, | |
| params: { | |
| grant_type: "client_credentials", | |
| client_id: clientid, | |
| client_secret: clientsecret, | |
| }, | |
| }) | |
| const body = new URLSearchParams({ | |
| grant_type: "client_credentials", | |
| client_id: clientid, | |
| client_secret: clientsecret, | |
| }) | |
| const response = await _fetch(`${url}/oauth/token`, { | |
| method: "POST", | |
| headers, | |
| body: body.toString(), | |
| }) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const params = new URLSearchParams({ | ||
| grant_type: "client_credentials", | ||
| client_id: clientid, | ||
| client_secret: clientsecret, | ||
| }) | ||
| const { data: tokenData } = await _fetch(`${tokenUrl}?${params}`, { | ||
| method: "POST", | ||
| headers, | ||
| params: { | ||
| grant_type: "client_credentials", | ||
| client_id: clientid, | ||
| client_secret: clientsecret, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Bug: OAuth token sent as query parameter instead of request body
Same issue as in lib/helper.js: client_id, client_secret, and grant_type are appended to the URL as query string parameters for a POST request. The Content-Type: application/x-www-form-urlencoded header signals the server to expect these in the request body. Many OAuth providers will reject a POST with credentials only in the query string, and query params appear in server access logs.
Consider sending the credentials in the request body:
| const params = new URLSearchParams({ | |
| grant_type: "client_credentials", | |
| client_id: clientid, | |
| client_secret: clientsecret, | |
| }) | |
| const { data: tokenData } = await _fetch(`${tokenUrl}?${params}`, { | |
| method: "POST", | |
| headers, | |
| params: { | |
| grant_type: "client_credentials", | |
| client_id: clientid, | |
| client_secret: clientsecret, | |
| }, | |
| }) | |
| const params = new URLSearchParams({ | |
| grant_type: "client_credentials", | |
| client_id: clientid, | |
| client_secret: clientsecret, | |
| }) | |
| const { data: tokenData } = await _fetch(`${tokenUrl}`, { | |
| method: "POST", | |
| headers, | |
| body: params.toString(), | |
| }) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| }) | ||
| const instancePath = headers.get("location").substring(1) | ||
| const instanceId = await _pollUntilDone(sm_url, instancePath, token) | ||
| return instanceId.data.resource_id |
There was a problem hiding this comment.
Bug: _pollUntilDone returns { data, headers } but callers use .data.resource_id and .data.state directly
_fetch in server.js now returns { data, headers }. The _pollUntilDone function passes through the full _fetch result at line 342 (return instanceStatus) and line 306 uses instanceId.data.resource_id. However, at lines 340 and 352 within _pollUntilDone itself, instanceStatus.data.state is checked correctly. At line 306 in _createObjectStoreInstance, instanceId.data.resource_id is accessed on the value returned from _pollUntilDone, which itself returned the raw instanceStatus object from _fetch—so instanceId.data.resource_id is correct.
However, note that _pollUntilDone can return undefined when it times out (falls out of the while loop without isReady becoming true) or when STATE.FAILED is reached. In both those cases, execution continues looping without error or return, and _createObjectStoreInstance will receive undefined from _pollUntilDone, then crash with Cannot read properties of undefined (reading 'data') at line 306.
Should add a break or throw when the FAILED or timeout condition is detected, and guard against a null/undefined return from _pollUntilDone.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| async function _fetch(url, options = {}) { | ||
| const res = await fetch(url, options) | ||
| const data = await res.json().catch(() => null) | ||
| if (!res.ok) { | ||
| const err = new Error(`HTTP ${res.status} ${res.statusText}`) | ||
| err.response = { status: res.status, data } | ||
| throw err | ||
| } | ||
| return data | ||
| } |
There was a problem hiding this comment.
Bug: Duplicate code — _fetch and _fetchWithAgent are defined identically in both lib/helper.js and lib/mtx/server.js
Both files define their own private copies of _fetch and _fetchWithAgent with identical implementations (modulo the return shape of _fetch: helper.js returns data directly, while server.js returns { data, headers }). This duplication will lead to maintenance issues—any bug fix or change must be applied twice.
Consider extracting these helpers into a shared utility module (e.g., lib/http-utils.js) and importing from both files.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| async function _fetch(url, options = {}) { | ||
| const res = await fetch(url, options) | ||
| const data = await res.json().catch(() => null) | ||
| if (!res.ok) { | ||
| const err = new Error(`HTTP ${res.status} ${res.statusText}`) | ||
| err.response = { status: res.status, data } | ||
| throw err | ||
| } | ||
| return data |
There was a problem hiding this comment.
Bug: _fetch reads the response body before checking res.ok, discarding non-JSON error bodies silently
res.json() is called with .catch(() => null) before the !res.ok check. For error responses with a non-JSON body (e.g., plain-text 500 errors, HTML gateway errors), data will be null, which is fine. However, for successful responses with a non-JSON body (e.g., a 204 No Content, or a plain-text 200), calling .json() will throw and silently set data = null—the caller receives null and has no way to distinguish a successful empty response from a JSON parse failure.
More critically: calling res.json() consumes the response body stream. If ever needed for non-JSON content, the body is already gone.
Consider checking res.ok first and only parsing JSON when content is expected:
| async function _fetch(url, options = {}) { | |
| const res = await fetch(url, options) | |
| const data = await res.json().catch(() => null) | |
| if (!res.ok) { | |
| const err = new Error(`HTTP ${res.status} ${res.statusText}`) | |
| err.response = { status: res.status, data } | |
| throw err | |
| } | |
| return data | |
| async function _fetch(url, options = {}) { | |
| const res = await fetch(url, options) | |
| if (!res.ok) { | |
| const data = await res.json().catch(() => null) | |
| const err = new Error(`HTTP ${res.status} ${res.statusText}`) | |
| err.response = { status: res.status, data } | |
| throw err | |
| } | |
| const data = await res.json().catch(() => null) | |
| return data | |
| } |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
Remove
axiosDependency in Favor of Built-infetchAPIChore
♻️ Refactor: Replaced the
axiosexternal dependency with Node.js built-infetchandhttpsAPIs across the codebase, reducing the dependency footprint.Changes
Two internal helper functions were introduced to replace
axiosbehavior:_fetch(url, options)— wraps the globalfetchAPI with JSON parsing and error handling._fetchWithAgent(url, options)— uses Node.jshttps.requestfor requests requiring a custom TLS agent (e.g., MTLS flows).package.json: Removedaxiosfrom thedependencieslist.lib/helper.js: Replaced allaxioscalls (GET, POST with client credentials, POST with MTLS) with_fetchand_fetchWithAgent. Updated response data access fromresponse.data.*to direct response properties.lib/mtx/server.js: Replaced allaxioscalls (GET, POST, DELETE) in service manager operations with_fetchand_fetchWithAgent. Updated header access to useheaders.get("location")(Fetch APIHeadersobject). Removed theaxiosimport.tests/unit/unitTests.test.js: Removedaxiosmock and replaced allaxios.get/axios.postmock implementations withglobal.fetchmock chains. Updated theglobal.fetchdefault mock to includeok: truefor proper response simulation.CHANGELOG.md: Added changelog entry noting the replacement ofaxioswith the built-infetchAPI.PR Bot Information
Version:
1.22.9anthropic--claude-4.6-sonnetd2199f26-87b9-414b-a48f-468a1180a244pull_request.opened