JavaScript SDK: add configurable request timeout, fix Node hash() tag handling (KSM-1209, KSM-1254)

[stas-schaller]

Summary

JavaScript SDK: bounds network request timeouts and fixes the Node platform's hash() ignoring its tag argument.

Changes

Fixed

• Node and browser network calls (main API POST, file upload, file download/thumbnail) had no request timeout and could hang indefinitely on a stalled or hostile server (KSM-1209). Node now sets a socket timeout and rejects with KeeperError if it fires; browser uses AbortSignal.timeout. Default 30s, configurable via SecretManagerOptions.requestTimeoutMs, or a direct timeoutMs argument on downloadFile/downloadThumbnail.
• The Node platform's hash() hardcoded its HMAC tag instead of using the caller-supplied tag argument, unlike the browser implementation and the documented Platform contract (KSM-1254). No behavior change for the SDK's existing caller, which already passed that same string.

Testing

cd sdk/javascript/packages/core && npm test

Security Impact

• KSM-1209 mitigates a denial-of-service condition (CWE-400): an unresponsive or hostile server previously had no bound on how long it could hold a caller's request open. Does not change certificate validation or what is transmitted.
• KSM-1254's hash() derives the client ID from the client key (getClientId, via CLIENT_ID_HASH_TAG). The fix makes the Node platform honor its tag argument instead of a hardcoded string, closing a latent wrong-tag risk for any future caller that passes a tag other than the one currently in use; no change to today's derived client ID.

Breaking Changes

None. New requestTimeoutMs/timeoutMs parameters and Platform contract additions are additive and optional; downloadFile/downloadThumbnail keep their existing call signature.

Related Issues

• Jira: KSM-1209, KSM-1254

[mgallego-keeper]

Summary

Reviewed the timeout bounding and the Node hash() tag fix in detail, including empirical reproduction of the Node vs. browser timeout semantics against live servers. The core mechanism is sound and the hash() fix is correct, but a few concrete defects and a contradicted "Breaking Changes: None" claim should be addressed before merge.

Correctness / security

1. Node's timeout is an idle timer, not a deadline (medium). nodePlatform.ts (lines 213, 235, 272) passes timeout to https.request, which resets on every byte of socket activity. browserPlatform.ts (lines 335, 360, 386) uses AbortSignal.timeout(), a fixed wall clock deadline. Reproduced with a server trickling 1 byte every 150ms for 3s at requestTimeoutMs=300: the Node request resolved successfully; the identical scenario under browser semantics rejected in ~300ms. A hostile server that stays just under the idle window can still hang a Node caller indefinitely, which is the exact scenario the changelog says this closes, on the SDK's primary server side target.

2. timeoutMs: 0 means opposite things on each platform, and nothing validates it (medium). timeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS only substitutes on null/undefined, so an explicit 0 passes through unchanged. On Node this fully disables the timeout (reopens the DoS this PR fixes); on browser, AbortSignal.timeout(0) aborts nearly every request instantly. Both reproduced live. Related, lower severity: negative values throw a raw RangeError instead of KeeperError on both platforms; values above 2^32-1 throw on browser but only warn and clamp on Node. Recommend validating/clamping timeoutMs once, in keeper.ts, before it reaches either platform.

3. downloadFile/downloadThumbnail cannot inherit SecretManagerOptions.requestTimeoutMs (medium). uploadFile forwards options.requestTimeoutMs to platform.fileUpload, but downloadFile/downloadThumbnail (keeper.ts:1398,1403) only accept an explicit timeoutMs argument, no options parameter. A caller who sets requestTimeoutMs once, expecting it to bound downloads too, silently gets the 30s default instead.

4. cachingPostFunction/createCachingFunction silently drop the new override (low). Both public exports (node/localConfigStorage.ts, browser/localConfigStorage.ts) declare only (url, transmissionKey, payload) and call platform.post with 3 args, so requestTimeoutMs never reaches platform.post for any consumer using the SDK's own offline cache helpers.

5. Browser timeout errors are a raw DOMException, never KeeperError (low/medium). Node wraps its timeout in new KeeperError(...); browserPlatform.ts's get/post have no try/catch at all, and fileUpload's catch re-throws unchanged. Code written against instanceof KeeperError (the pattern the CHANGELOG implies) will silently miss timeouts on browser.

Breaking change risk

6. AbortSignal.timeout() is called unconditionally in every browser network call, with no feature detection (medium/high). No typeof guard and no polyfill anywhere in the rollup browser build. On a runtime lacking this API (pre-2022 browsers, older embedded WebViews), every get/post/fileUpload call now fails immediately with a TypeError, a full break, not limited to calls that would have timed out. This contradicts "Breaking Changes: None": a runtime that previously worked fine with no timeout enforcement now hard fails on every call.

Test coverage

7. requestTimeoutMs propagation through the public API is completely untested (high). Zero occurrences of requestTimeoutMs/timeoutMs in keeper.test.ts or throttle.test.ts. Verified by mutation testing: dropping options.requestTimeoutMs from the call at keeper.ts:800 entirely still leaves the full suite at 73/73 passing.
8. downloadFile/downloadThumbnail/uploadFile are never invoked by any test.
9. No test proves a timeout rejection isn't retried forever by postQuery's retry loop (current behavior is correct, verified by repro, but it's unguarded and untested).
10. nodePlatform.test.ts's https mock never emits a response event; a coverage run confirms the success path lines (fetchData and the three callback bodies) are never executed.
11. DEFAULT_REQUEST_TIMEOUT_MS's value is only checked against itself; mutating 30000 to 30 left the full suite green.
12. hash()'s fix is only provable by the new isolated unit test; both real production call sites pass the same tag before and after the fix, so no integration test can distinguish buggy from fixed behavior.

Process note

13. This PR's target branch never triggers the JS test workflow. test.js.yml runs only on PRs into master; this PR targets release/sdk/javascript/core/v17.6.0. The only passing checks are Socket Security scans; no test execution ran in CI. Confirmed the suite does pass (73/73) by running it locally against this PR branch.

Not an issue

Confirmed the hash() fix itself is correct and non-breaking today (both call sites pass an identical tag), and no other package in the monorepo touches the changed surface.

Recommendation

Requesting changes on items 1, 2, 3, and 6: the Node/browser timeout semantics gap and the unvalidated 0 value both undercut the security rationale in the PR description, and the browser hard dependency contradicts the stated "no breaking changes." Items 7-12 would meaningfully reduce the chance of a silent regression in this exact area and are worth adding here or in a fast follow-up.

[stas-schaller]

Superseded: split into #1135 (KSM-1254) → #1136 (KSM-1209), one ticket per PR.