feat(runtime): replace axios/alpha sync client with undici by VDubber · Pull Request #1211 · JupiterOne/sdk

VDubber · 2026-10-06T21:47:56Z

Summary

Replaces the JupiterOne sync-job/persister client (createApiClient) transport — @lifeomic/alpha (axios) → undici. The client keeps the same .get<T>/.post → {data,status} surface, so all SDK and CLI consumers are unchanged.

Why

axios/alpha errors retain a live ClientRequest and TLS socket. A deep inspect of such an error walks the socket and exposes the request Authorization header via Symbol(connect-options) (the TD-9349 leak). undici errors we throw are plain ApiResponseError objects carrying only {response:{status,statusText,data}, config:{method,url}} — no socket — so the credential cannot be reached. This removes the leak vector structurally for the sync client, independent of the logger serializer.

(Complements PR #1210, which sanitizes at the log boundary for the gaxios/vendor clients that can't drop axios.)

Changes

ApiClient is now an undici-backed class (request()); ApiResponseError for non-2xx.
Ports: baseURL + default headers, gzip-on-persister uploads (reuses gzipData), proxy via undici ProxyAgent (incl. Basic auth).
Drops alpha's inner retry — the outer @lifeomic/attempt retry remains, eliminating the double-retry layer the original report called out.
Removes cleanAxiosError (undici errors carry nothing to scrub).
Retypes error.ts / events.ts off axios onto local interfaces.

Tests / evidence

api tests rewritten against a local server: get/post, default headers, gzip-only-on-persister, proxy config, non-2xx → ApiResponseError.
Credential non-exposure asserted via deep inspect(err, {showHidden:true, depth:20}) — walks non-enumerable and symbol-keyed props (e.g. Symbol(connect-options)), the exact vector of the original leak (JSON.stringify would skip it). Covered on both the HTTP-error path and the transport-failure (connection-refused) path.
TLS end-to-end: drives the real uploadDataChunk through the undici client against a 503 mock persister, asserting the bytes bunyan writes contain no bearer token — with the serializer unchanged. Proves the undici swap alone closes the sync-path leak.
All green: runtime 301/301, CLI 27/27, typecheck clean across runtime + CLI + sdk-cli. (The undici E2E also runs ~931ms vs ~15s for alpha — the removed double-retry.)

Follow-up (not in this PR)

@lifeomic/alpha remains a declared dependency (still used transitively by CLI test axios imports). Fully removing it cascades into CLI test plumbing + a dist rebuild — a separate dep-hygiene PR.

🤖 Generated with Claude Code

Rewrites the JupiterOne sync-job/persister client (createApiClient) on undici instead of @lifeomic/alpha (axios). The client keeps the same .get/.post -> {data,status} surface, so SDK and CLI consumers are unchanged. Why: axios/alpha errors retain a live TLS socket; a deep inspect of such an error walks the socket and exposes the request Authorization header (Symbol(connect-options)). undici errors we throw are plain ApiResponseError objects carrying only {response:{status,statusText,data}, config:{method,url}} — no socket — so the credential cannot be reached. This removes the leak vector structurally for the sync client, independent of the logger serializer. Changes: - ApiClient is now an undici-backed class (request()); ApiResponseError for non-2xx. Ports baseURL+headers, gzip-on-persister uploads, and proxy via undici ProxyAgent. Drops alpha's inner retry (the outer @lifeomic/attempt retry remains), eliminating the double-retry layer. - Removes cleanAxiosError (undici errors carry nothing to scrub). - Retypes error.ts/events.ts off axios onto local interfaces. Tests: - Rewrites api tests to run against a local server: get/post, default headers, gzip-only-on-persister, proxy config, non-2xx -> ApiResponseError, and credential non-exposure via deep inspect (showHidden walks symbol-keyed props like Symbol(connect-options)) on both the HTTP-error and transport-failure paths. - Adds a TLS end-to-end test driving the real uploadDataChunk through the undici client against a 503 mock persister, asserting the bytes bunyan writes contain no bearer token — with the serializer unchanged. @lifeomic/alpha remains a declared dependency (still used transitively by CLI tests); fully removing it + the dist rebuild is a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

socket-security · 2026-10-06T21:48:45Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	undici@6.29.0

View full report

ryanmcafee

Thanks for this -- moving the sync client off axios/alpha is a nice structural fix for the credential leak.

One request: could @lifeomic/alpha be removed entirely in this PR rather than in the "Follow-up (not in this PR)" noted in the description? Remaining references at this head are the integration-sdk-runtime dependency, the two lockfile entries, and three CLI tests that mock axios (which resolves only via alpha). Inline comments are on the nearest changed lines. Historical CHANGELOG entries can stay; a new entry noting the removal would help.

A few other notes inline, mainly around retry behavior that alpha used to provide and the public API surface change.

ryanmcafee · 2026-10-06T22:18:10Z

+
+/**
+ * Minimal JupiterOne API client backed by undici. Exposes the `.get`/`.post`
+ * surface the SDK and CLI rely on. Retry is handled by callers via


Lost transport retry for every call outside uploadDataChunk/finalizeSynchronization. The old client passed retry: retryOptions ?? {} to alpha, which (alpha 5.2.0 response-retry.js) retried 3x with backoff on network errors and any 5xx for all requests. Only upload and finalize have an outer @lifeomic/attempt loop. initiateSynchronization, synchronizationStatus, abortSynchronization, event publishing, and CLI paths (waitForSyncCompletion, bulkDownloadToJson, validateQueries) now fail on a single transient 502/ECONNRESET -- e.g. one LB blip at job start fails the whole integration run.

Suggest a small bounded retry (network errors + 5xx) in ApiClient.request, or wrap those call sites in retry(), so the double-retry is removed only where an outer loop actually exists.

Addressed — this predates 7364710. The client now has built-in retry (default 3 attempts with backoff) on 429/5xx and transport errors; the calls without their own loop (initiate/status/abort/events, CLI paths) are covered. Callers that already wrap in @lifeomic/attempt (uploadDataChunk, finalize, importAssets) pass config.retry = false so there is no nested double-retry. See 4e830e5 for the non-2xx/url fixes alongside.

ryanmcafee · 2026-10-06T22:18:10Z

+    bodyObj: unknown,
+    config?: ApiRequestConfig,
+  ): Promise<ApiClientResponse<T>> {
+    const fullUrl = new URL(url, this.baseURL).toString();


new URL(url, baseURL) drops any path prefix on apiBaseUrl. All callers pass absolute paths (/persister/..., /j1ql/validate), so new URL('/persister/synchronization/jobs', 'https://host/api/v1') resolves to https://host/persister/synchronization/jobs. axios concatenated baseURL + url, preserving the prefix. The docstring on createApiClient says managed environments point this at public/private APIs, so any base URL with a path segment (gateway/ingress prefix, --api-base-url) silently starts hitting the wrong endpoint (404s).

Suggested change

const fullUrl = new URL(url, this.baseURL).toString();

const fullUrl = `${this.baseURL.replace(/\/+$/, '')}/${url.replace(/^\/+/, '')}`;

Fixed in 4e830e5 — the URL is now built by concatenation (baseURL.replace(/\/+$/,"") + "/" + url.replace(/^\/+/,"")), preserving any path prefix on apiBaseUrl. Added a proxy e2e test asserting the origin receives /api/things through a base URL of .../api.

ryanmcafee · 2026-10-06T22:18:10Z

+  }
+}

 interface CreateApiClientInput {


Public API break shipped as feat (minor). @jupiterone/integration-sdk-runtime exports this module via export * from './api'. This PR removes retryOptions and alphaOptions from createApiClient's input, removes the exported compressRequest, and changes ApiClient from Alpha (axios instance: put/patch/delete/request/interceptors/defaults) to a GET/POST-only class. External consumers (managed runtime, integrations) passing alphaOptions/retryOptions or calling other verbs fail to compile -- or for JS callers, the options are silently ignored. This needs a major bump / BREAKING CHANGE: footer and a CHANGELOG entry.

Addressed in 4e830e5 — CHANGELOG now has a ## BREAKING section documenting the ApiClient/createApiClient surface change and the alpha removal. This should ship as a major bump.

ryanmcafee · 2026-10-06T22:18:10Z

+    const contentType = res.headers['content-type'] as string | undefined;
+    const data = parseBody(text, contentType);
+
+    if (res.statusCode >= 400) {


3xx responses are returned as success. Only >= 400 throws, and undici request() does not follow redirects (axios did, up to 5). A 301/302/307 from an API gateway now returns { data: undefined | '<html...>' }, and callers like initiateSynchronization do response.data.job -> TypeError: Cannot read properties of undefined, which bypasses synchronizationApiError context. Consider treating any non-2xx as ApiResponseError (or enabling undici's redirect interceptor).

Suggested change

if (res.statusCode >= 400) {

if (res.statusCode < 200 || res.statusCode >= 300) {

Fixed in 4e830e5 — any non-2xx (< 200 || >= 300, so 3xx included) now throws ApiResponseError rather than returning an unusable body as success.

ryanmcafee · 2026-10-06T22:18:10Z

    "p-queue": "^6.3.0",
-    "rimraf": "^3.0.2"
+    "rimraf": "^3.0.2",
+    "undici": "^6.21.0"


undici@^6.21.0 requires Node >=18.17 (lockfile resolves 6.29.0 with "node": ">=18.17"), but this package declares "engines": { "node": ">=18.0.0 <21.x" }. Consumers on Node 18.0-18.16 get an engine mismatch / runtime failure in undici. Either raise the runtime engines floor to >=18.17.0 or pin an undici line compatible with the declared range.

Fixed in 4e830e5 — runtime engines.node floor raised to >=18.17.0 to match undici@6.

ryanmcafee · 2026-10-06T22:18:11Z


-    it('should configure proxy from HTTPS_PROXY environment variable', () => {
+  it.each([
+    ['proxyUrl param', { proxyUrl: 'https://foo:bar@proxy.example.com:8888' }],


Proxy tests only assert toBeInstanceOf(ApiClient), so the ProxyAgent port is untested. Nothing checks that requests actually route through the proxy, that the Proxy-Authorization: Basic ... token is built from URL-decoded credentials, or that HTTPS_PROXY is picked up -- a regression that dropped dispatcher entirely would still pass. Suggest a local http.createServer proxy that handles CONNECT (or a plain HTTP target) and asserts the request and proxy-authorization header arrive there.

Added in 4e830e5 — a real proxy test: a CONNECT-tunneling http proxy pipes to a local origin, and the test asserts the request routes through it, the origin receives the prefixed path, and Proxy-Authorization is Basic base64(user:pass) built from URL-decoded credentials.

ryanmcafee · 2026-10-06T22:18:11Z

    "p-map": "^4.0.0",
    "p-queue": "^6.3.0",
-    "rimraf": "^3.0.2"
+    "rimraf": "^3.0.2",


Could we drop "@lifeomic/alpha": "^5.2.0" from this package's dependencies (packages/integration-sdk-runtime/package.json line 27, just above this hunk) in this PR? Nothing in integration-sdk-runtime/src imports it anymore, so keeping it still ships alpha and its AWS SDK v3 / axios@0.27 tree to every consumer.

Done in 4e830e5 — @lifeomic/alpha dropped from integration-sdk-runtime dependencies.

ryanmcafee · 2026-10-06T22:18:11Z

@@ -4365,6 +4148,8 @@
    },
    "node_modules/@lifeomic/alpha": {
      "version": "5.2.0",
+      "resolved": "https://registry.npmjs.org/@lifeomic/alpha/-/alpha-5.2.0.tgz",


This PR still touches (and adds resolved/integrity to) the node_modules/@lifeomic/alpha entry. Once the dependency is removed from packages/integration-sdk-runtime/package.json, could you regenerate the lockfile so this entry and its now-unused transitive deps (axios@^0.27.2, the @aws-sdk/* packages, nearley, etc.) drop out in this PR?

Done — lockfile regenerated in 4e830e5; the @lifeomic/alpha entry and its now-unused transitive deps (axios@0.27, the @aws-sdk/* tree, nearley, etc.) are gone.

ryanmcafee · 2026-10-06T22:18:11Z

@@ -21046,7 +20194,8 @@
        "lodash": "^4.17.15",
        "p-map": "^4.0.0",
        "p-queue": "^6.3.0",
-        "rimraf": "^3.0.2"
+        "rimraf": "^3.0.2",


The packages/integration-sdk-runtime workspace entry still lists "@lifeomic/alpha": "^5.2.0" (package-lock.json line 20184, a few lines above this hunk). Could that go in this PR along with the package.json change?

Done in 4e830e5 — the workspace entry no longer lists @lifeomic/alpha.

ryanmcafee · 2026-10-06T22:18:11Z

 interface ImportAssetsTypeParams {
  storageDirectory: string;
-  apiClient: Alpha;
+  apiClient: ApiClient;


Related to removing @lifeomic/alpha here: packages/cli/src/export/__tests__/bulkDownloadToJson.test.ts (line 2), packages/cli/src/__tests__/cli-export.test.ts (line 1), and packages/cli/src/__tests__/cli-import.test.ts (line 2) still import/jest.mock('axios') and return axios from the mocked createApiClient. packages/cli does not declare axios, so these only resolve through alpha. Could they mock ApiClient (get/post returning { data }) instead, so alpha can be removed in this PR?

Done in 4e830e5 — the three CLI tests now mock ApiClient ({ get: jest.fn(), post: jest.fn() }) instead of importing/mocking axios, so alpha could be removed.

The alpha client retried retryable errors (5xx) 3x by default. The initial undici port dropped all client retry, so calls without their own retry loop (initiateSynchronization, synchronizationStatus, abortSynchronization, event publishing) lost automatic retry on transient 5xx — a resilience regression. Restores built-in retry on the ApiClient via @lifeomic/attempt: default 3 attempts with backoff, retrying 429/5xx and transport errors. Re-adds retryOptions to createApiClient. Callers that already wrap requests in their own retry loop (uploadDataChunk, finalizeSynchronization, CLI importAssets) pass config.retry=false so there is no nested double-retry — keeping the single-layer behavior the undici port was meant to achieve for uploads. Adds tests: retries 5xx then succeeds, does not retry 4xx, and retry=false disables retry. Updates call-site expectations for the new config arg. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…stion-service naming Adds an end-to-end test driving synchronizeCollectedData (create job -> upload entities/relationships -> finalize) through the real undici client against a mock ingestion-service server, with no spies on the client. Asserts the job is created, entities and relationships are received, finalize returns the finalized job, and the access token never appears in emitted logs. Renames the "persister" naming to "ingestion-service" in comments, the internal upload-path constant, and test names to match the current service name. The `/persister/...` HTTP route strings and the exported createPersisterApiStepGraphObjectDataUploader symbol are intentionally left unchanged: the routes are the live API contract (still served under the legacy prefix) and the exported symbol is kept to avoid public-API churn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

VDubber · 2026-10-07T04:19:02Z

-        throw err;
-      }
+          // This loop handles retry/backoff; skip the client's retry.
+          retry: false,


I want to eliminate the retry pattern outside of the client. Unless there is a compelling reason such as being very different retry patterns.

Agreed on the direction, with one hard exception. uploadDataChunk’s outer retry() is not just retry — its handleError (handleUploadDataChunkError) runs a side-effect between attempts: on a 413 it calls shrinkBatchRawData(batch, ...) to shrink oversized raw data before the next try, and it throws fatally on JOB_NOT_AWAITING_UPLOADS. The client-level retry can’t reshape the caller’s batch, so that loop has to stay (it now passes config.retry = false to avoid double-retry).

finalizeSynchronization and the CLI importAssets loops are plain retry/backoff with no such side-effect, so those two could drop their @lifeomic/attempt wrapper and rely on the client’s built-in retry. I left them as-is in this PR to keep the diff focused on the alpha removal. Happy to collapse those two onto the client retry here if you prefer, or as a fast follow — which would you like?

Done in 9558e49 — finalizeSynchronization and the CLI importAssets upload now drop their @lifeomic/attempt wrappers and rely on the client’s built-in retry (they keep a try/catch for error wrapping/logging). uploadDataChunk keeps its loop for the 413 batch-shrink side-effect noted above.

BREAKING CHANGE: the sync client is fully off @lifeomic/alpha/axios. - Drop @lifeomic/alpha from integration-sdk-runtime dependencies and regenerate the lockfile (removes axios@0.27, the @aws-sdk/* tree, nearley, etc.). - Rewrite the three CLI tests that mocked `axios` (resolved only via alpha) to mock `ApiClient` ({ get, post }) instead. Review fixes (ryanmcafee): - Treat any non-2xx (including 3xx) as ApiResponseError — undici does not follow redirects, so a 3xx no longer returns an unusable body as success. - Build the request URL by concatenation so a path prefix on apiBaseUrl (gateway/ingress) is preserved instead of dropped by `new URL(url, base)`. - Use Node's http.STATUS_CODES for statusText instead of a hand-maintained map. - Raise the runtime Node engines floor to >=18.17.0 (required by undici@6). - Add a proxy end-to-end test: a CONNECT-tunneling proxy asserts the request routes through it and Proxy-Authorization uses URL-decoded Basic credentials. - CHANGELOG: document the breaking API change and the undici retry behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…client Per review (VDubber): centralize retry in the api client. finalizeSynchronization and the CLI importAssets upload no longer wrap their request in a separate @lifeomic/attempt loop — they rely on the client's built-in retry (429/5xx + transport) and keep a try/catch for error wrapping/logging. uploadDataChunk intentionally keeps its own loop: its handleError shrinks oversized batch raw data on 413 between attempts, a side-effect the client retry cannot perform. Updates the finalize call-site expectations (no more `retry: false`) and the full-flow test (finalize now succeeds in one call; 8 posts, not 9). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Polly's node-http adapter patches Node's http.request and cannot intercept the undici api client, so the cli run/sync tests were hitting the real API (401) in CI after the undici migration. Rewrites the shared synchronizer helper to start a local http server emulating the ingestion-service routes (returning its base URL), and updates cli-run, cli-sync, and cli-run-failure to pass `--api-base-url <local>` and assert via the server's finalized/aborted/eventsPublished state instead of Polly intercepts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

VDubber requested a review from a team as a code owner October 6, 2026 21:47

ryanmcafee reviewed Oct 6, 2026

View reviewed changes

VDubber and others added 2 commits October 6, 2026 21:31

VDubber commented Oct 7, 2026

View reviewed changes

VDubber and others added 3 commits October 7, 2026 08:54

ryanmcafee approved these changes Oct 7, 2026

View reviewed changes

VDubber merged commit c8de062 into main Oct 7, 2026
5 checks passed

VDubber deleted the sync-client-undici branch October 7, 2026 17:49

VDubber mentioned this pull request Oct 7, 2026

chore(release): v18.0.0 #1212

Merged

	const fullUrl = new URL(url, this.baseURL).toString();
	const fullUrl = `${this.baseURL.replace(/\/+$/, '')}/${url.replace(/^\/+/, '')}`;

	if (res.statusCode >= 400) {
	if (res.statusCode < 200 \|\| res.statusCode >= 300) {

Conversation

VDubber commented Oct 6, 2026

Summary

Why

Changes

Tests / evidence

Follow-up (not in this PR)

Uh oh!

socket-security Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanmcafee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

socket-security Bot commented Oct 6, 2026 •

edited

Loading