Release Azure DevOps Pipeline Extension v1.2.0 - #983
Draft
stas-schaller wants to merge 2 commits into
Draft
stas-schaller wants to merge 2 commits into
stas-schaller wants to merge 2 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
KSM-861: Node.js 20 reaches EOL 2026-04-30. Azure DevOps skipped Node 22 and went directly to Node24. Add Node24 execution handler per Microsoft migrateNode24.md requirements. Supersedes PR #756. - Add Node24 execution handler to task.json (alongside Node10/16/20) - Bump azure-pipelines-task-lib ^4.17.3 -> ^5.2.4 (Node24 minimum) - Bump @keeper-security/secrets-manager-core ^16.6.3 -> ^17.2.0 - Bump typescript ^5.1.6 -> ^5.8.3, @types/node ^20.3.1 -> ^24.10.0 - Bump mocha, dotenv, @types/mocha dev dependencies - Add tsconfig.json for IDE support - Update Dockerfile from node:10 to node:20 - Update publish workflow: checkout@v4, setup-node@v4, node 24 - Mock KSM SDK in tests (no longer requires real vault credentials) - Increase test timeout for MockTestRunner Node binary download
stas-schaller
force-pushed
the
release/integration/azdevops/v1.2.0
branch
from
April 9, 2026 19:38
4e343cd to
d5cccda
Compare
stas-schaller
added a commit
that referenced
this pull request
Sep 3, 2026
Out of scope for a JS-core release: the extension has its own independent release track (currently mid-review as PR #983, v1.2.0) and its package.json still pins core ^16.6.3, whose published downloadFile only takes one argument, so this edit would break that extension's own build regardless of argument slot. Already tracked by KSM-1343; moved Triage to Backlog to pick up at the extension's next release. Reverts the downloadFile(file, undefined, options) call site, downloadSecretFile's options param, and the SecretManagerOptions import back to their pre-KSM-1209 state.
mgallego-keeper
added a commit
that referenced
this pull request
Sep 14, 2026
* fix(javascript): add configurable request timeout (KSM-1209) Both platforms enforce the deadline via AbortController rather than Node's socket timeout option, which is an idle timer that resets on socket activity and can be held open indefinitely by a slow trickle of data - not a fixed deadline. Node requests reject with a KeeperError when the deadline fires. Browser requests use the same AbortController-driven deadline, falling back to a plain setTimeout on runtimes that lack the AbortSignal.timeout() shorthand instead of failing every request outright. Defaults to 30s, overridable via SecretManagerOptions.requestTimeoutMs or a direct timeoutMs argument on downloadFile/downloadThumbnail. Previously a stalled or hostile server could hang the caller indefinitely (CWE-400), and on Node a slow trickle of bytes could keep resetting the old idle timer so it never fired at all. An invalid requestTimeoutMs/timeoutMs (zero, negative, or non-finite) now throws immediately instead of silently disabling the timeout on Node or firing almost instantly on both platforms. downloadFile and downloadThumbnail take options as an optional 3rd argument so they can inherit SecretManagerOptions.requestTimeoutMs instead of only accepting an explicit override; the options-first reorder consistent with the rest of this file is deferred to the next major version, logged in SDK-V18-BREAKING-CHANGES.html alongside KSM-1265's cachingPostFunction removal. Node's timeout error message no longer includes the request URL's query string. File download/thumbnail/upload URLs from the storage backend carry an AWS SigV4 signature there (an 8-hour bearer credential, confirmed against the backend's DownloadRequestFactory), which a timeout message would otherwise leak into whatever logs the caller's error handler writes to. * JavaScript SDK: bound the whole request and validate the timeout (KSM-1209 review fixes) Follow-up to the review on PR #1136. The AbortController deadline was cleared in the response callback, before fetchData read a byte, so nothing bounded the response body. A server that sent headers and then stalled hung the caller forever, which the previous socket-timeout version had caught. Hold the deadline until the body ends, and wire the response stream's error event so a mid-body socket failure rejects rather than leaving the promise pending. fileUpload had the same gap on its own response object; fixed the same way. Validate the timeout in one place. 0, negatives, NaN and Infinity all collapse to a near-instant setTimeout, so they now raise an Error instead of silently killing every request under a message naming a value that was never applied; values past setTimeout's 32-bit ceiling clamp rather than truncating to 1ms. Plain Error, not KeeperError, matching this file's existing convention for caller-input/config problems. Also from the review: - downloadFile/downloadThumbnail take options and inherit requestTimeoutMs, keeping timeoutMs as the second argument to avoid stacking a second breaking change onto KSM-1265's in the same minor - cachingPostFunction/createCachingFunction forward the trailing arguments - browser timeouts raise KeeperError instead of a raw DOMException - deadlineSignal moved to an internal module, off the public node surface - CHANGELOG corrected: it claimed the trickle case was closed when it was not truncateUrlForError (the CWE-532 query-string redaction on timeout error messages) is preserved and now covers both platforms uniformly via the shared timeoutError() helper. Tests: 73 to 164. The https mock now emits real response events, so the body path is executed; every wiring point from SecretManagerOptions to the platform call is asserted. Verified against a local HTTPS server: stall, trickle, mid-body reset and premature close are all bounded on both platforms. Co-authored-by: Stas Schaller <sschaller@keepersecurity.com> * fix(javascript): drain fileUpload responses and close the second review round's gaps (KSM-1209) Discovered while verifying the new file-upload example (KSM-1328): fileUpload() resolves off headers alone and never reads the response body. The comment already on this line (from the KSM-1209 review-fix round) correctly identifies that fact for the unhandled-'error' case, but the same unconsumed body also leaves the socket open, which keeps the event loop alive - a script with no other pending work never exits on its own after a successful upload. res.resume() discards the body without buffering it, since nothing here reads it anyway. Verified against Dev-CA: same script hangs (exit code 124) without this fix, exits cleanly (code 0) with it, no process.exit() needed on the caller's end. Second round of fixes to PR #1136's own review (the 36b73f1 commit above), addressing the follow-up CHANGES_REQUESTED pass plus the non-blocking items from that same review: - cachingPostFunction/createCachingFunction no longer treat a deliberate client-side timeout the same as a real network failure; a KeeperError from timeoutError() now propagates instead of returning a synthetic success built from stale cache - downloadFile's call sites in the Azure DevOps task and the hello-secret example were passing an options object into the timeoutMs slot, throwing on every call; restored the missing `undefined` placeholder - resolveTimeoutMs now rejects any value below 1, not just <= 0, so a fractional timeout like 0.5 can no longer floor to an instant abort - validateTimeoutMs now returns the resolved, clamped value instead of the raw input, so a custom queryFunction or the offline-cache helpers never see an over-max or fractional timeout unclamped - postQuery validates requestTimeoutMs once, up front, before any storage write or payload encryption, and reuses the resolved value across retries instead of re-validating it every iteration - extracted armRequest() in nodePlatform.ts so get/post/fileUpload share one abort/error wiring implementation instead of three copies; each now wraps its request()/https.request() call in try/catch so a synchronous throw clears the deadline timer instead of leaking it - uploadFile gains its own optional timeoutMs argument, matching downloadFile/downloadThumbnail - DEFAULT_REQUEST_TIMEOUT_MS is now exported from the browser entry point, not just the Node one - the custom-caching-function-support example now forwards timeoutMs to postFunction instead of dropping it - reworded the downloadFile comment: it cited KSM-1265 as "already-shipped" (it is not, that PR is still under review) and named a ticket in a source comment; also notes that allowUnverifiedCertificate isn't honored here since platform.get has no such parameter - CHANGELOG amended in place on the existing KSM-1209 entry to cover the behavior changes above Also fixed: the fileUpload tests' MockResponse had no resume() method, so they broke as soon as the drain fix above added that call; added a jest.fn() stub. Tests: 164 to 181. * fix(javascript): drain fileUpload's response body so the process can exit (KSM-1209) (#1148) Discovered while verifying the new file-upload example (KSM-1328): fileUpload() resolves off headers alone and never reads the response body. The comment already on this line (from the KSM-1209 review-fix round) correctly identifies that fact for the unhandled-'error' case, but the same unconsumed body also leaves the socket open, which keeps the event loop alive - a script with no other pending work never exits on its own after a successful upload. res.resume() discards the body without buffering it, since nothing here reads it anyway. Verified against Dev-CA: same script hangs (exit code 124) without this fix, exits cleanly (code 0) with it, no process.exit() needed on the caller's end. * revert(javascript): drop Azure DevOps integration edit from KSM-1209 Out of scope for a JS-core release: the extension has its own independent release track (currently mid-review as PR #983, v1.2.0) and its package.json still pins core ^16.6.3, whose published downloadFile only takes one argument, so this edit would break that extension's own build regardless of argument slot. Already tracked by KSM-1343; moved Triage to Backlog to pick up at the extension's next release. Reverts the downloadFile(file, undefined, options) call site, downloadSecretFile's options param, and the SecretManagerOptions import back to their pre-KSM-1209 state. * fix(javascript): O(n^2) response buffering, racy abort listener (KSM-1209) Round-4 review, blocking items 1 and 11. fetchData re-copied the whole accumulated response buffer on every 'data' event via Buffer.concat([retVal.data, data]), O(n^2) in body size. This PR's own deadline turns that from "slow" into "fails": a large-but-healthy download can now time out purely on the CPU cost of its own buffering. Chunks are collected in an array and concatenated once at 'end' instead; retVal.data still stays null for a zero-length body, matching downloadFile's no-null-check assumption. armRequest's own signal.addEventListener('abort', ...) was racing Node's internal handling of the same signal (request()/https.request() already destroys the request and emits 'error' on it when the passed-in signal aborts). Confirmed against real Node (not mocked) that this internal behavior fires with no application-level listener needed. Removed the redundant listener; the existing req.on('error', ...) handler now checks signal?.aborted to decide between our own timeoutError and the raw error, mirroring browserPlatform.ts's spec-guaranteed asTimeout pattern for the identical problem. nodePlatform.test.ts's https.request mock never simulated this real Node behavior (its MockRequest is a bare EventEmitter with no signal wiring), so the fix left every deadline-firing test hanging until Jest's own timeout. Fixed the mock to wire signal abort -> destroy + error, matching verified real Node behavior. Reverting just the armRequest change against the corrected mock reproduces the exact race Mateo described (rejects with a plain Error instead of KeeperError) before confirming the fix. Full suite 228/228, tsc clean. * fix(javascript): caching fallback mishandles validation and write failures (KSM-1209) Round-4 review, blocking items 2 and 3. Both cachingPostFunction (node) and createCachingFunction (browser) caught everything platform.post could throw and rethrew only KeeperError, falling back to stale cache for anything else. resolveTimeoutMs throws a plain Error, not a KeeperError, for an invalid timeoutMs, by design (deadline.ts) - so a caller-input mistake was falling through the same carve-out meant only for transport failures, getting misread as "the request failed, use stale cache" instead of surfacing the validation error. Both functions now resolve/validate the timeout eagerly, before the try/catch, via validateTimeoutMs (already exported, same pattern postQuery and downloadFile/downloadThumbnail already use). Separately, both functions wrapped the cache write for a *successful* response inside the same try as the request itself. A write failure (disk full on node; IndexedDB quota/private-browsing/blocked-upgrade on browser, wrapped as KeeperError per KSM-1332) fell into the outer catch and discarded the already-obtained fresh response, either silently downgrading it to stale cache or throwing "Cached value does not exist" - worse than just returning what was already fetched. The cache write is now isolated in its own try/catch on both platforms; a write failure no longer affects the response returned to the caller. New tests in cachingFunctions.test.ts, each confirmed failing against the pre-fix code first: an unusable timeoutMs is now rejected before platform.post is ever called (previously silently accepted, since the mocked platform.post in this test file bypasses the real internal validation entirely); a cache-write failure on either platform no longer discards a successful response. Full suite 232/228, tsc clean. * fix(javascript): uploadFile validates late, example missing timeout carve-out (KSM-1209) Round-4 review, blocking items 4 and 5. uploadFile validated its own upload timeoutMs only at the platform.fileUpload call, after prepareFileUploadPayload and postQuery('add_file', ...) had already run - the latter allocates an upload placeholder URL on the backend. An invalid value failed only after that allocation, leaving a fileRef pointing at content that was never uploaded. Now validated up front, before either side effect, independent of postQuery's own internal validation of options.requestTimeoutMs for the add_file call itself (a different timeout budget). Regression coverage for this ordering is added in the upcoming test-consolidation pass (test/timeout.test.ts already has a "rejects invalid timeoutMs before platform.fileUpload" case that needs strengthening to also prove add_file was never called). The shipped custom-caching-function-support example still had the exact bug this PR fixed in the real cachingPostFunction: its catch block never checked for KeeperError, so a deliberate timeout fell through to the stale-cache fallback like any other failure. Added the same carve-out, mirroring the real implementation 1:1. tsc --noEmit clean, full suite 232/232 (this example has no jest coverage - jest.config.js's roots excludes examples/ entirely, tracked separately since building test infra for one demo file is out of scope here). * test(javascript): de-flake the node cache-fallback test (KSM-1209) Round-4 review, non-blocking item 9. 'a non-timeout failure still falls back to cache' relied on fs.readFileSync('cache.dat') failing because that file happened not to exist in the working directory - no mock, no cleanup. Confirmed flaky: planting a stray cache.dat before running the suite flips the test from an expected rejection to a resolved 200. Mocked fs.readFileSync to throw deterministically instead; no-op for the browser variant of the same describe.each, which never touches fs. Re-verified with a stray cache.dat planted - test now passes either way. No assertion changed, full suite 232/232. * test(javascript): cover the AbortController-unavailable fallback (KSM-1209) Round-4 review, non-blocking item 7. deadline.ts's typeof AbortController === 'undefined' branch had zero references anywhere in test/ - confirmed deleting it entirely still left the full suite green (it crashes instead now: "AbortController is not defined"). Two tests: deadlineSignal returns {signal: undefined, timeoutMs: <resolved>, clear: <noop>} with AbortController stubbed out; get still resolves normally end-to-end through that same scenario, proving armRequest's signal?.aborted check (post KSM-1209's earlier round-4 fix) tolerates a genuinely undefined signal, not just one that hasn't aborted yet. Both confirmed failing (a ReferenceError, not a normal test failure) with the fallback branch temporarily removed, then restored. Full suite 234/234, tsc clean. * test(javascript): close two partial-coverage gaps, fix a write-ordering bug found along the way (KSM-1209) Round-4 review, non-blocking item 8. Extended the existing requestTimeoutMs:0 rejection test to also set options.serverPublicKey/serverPublicKeyId and assert storage stays untouched for both after rejection - the existing test only proved the network call was skipped, not the storage writes postQuery's comment claims are also guarded. That extension caught a real bug: fetchAndDecryptSecrets (getSecrets's own call path) writes serverPublicKey/serverPublicKeyId to storage unconditionally, before ever calling postQuery, so postQuery's own validate-before-write ordering (added by this same PR) never got a chance to guard this earlier, separate write. Confirmed via a pre-existing test ("IL5 dynamic key - Layer 3") that this early write is deliberate for a different reason - an IL5 dynamic key discovered via a one-time token has to persist even if the call later fails for an unrelated reason (that test's own scenario: missing clientId) - so removing the write outright broke that intentional behavior (caught immediately by the existing test failing). Fixed narrowly instead: validateTimeoutMs(options.requestTimeoutMs) now runs immediately before that write, so a caller-input mistake produces no side effects at all, while a valid-but-later-failing call still gets the early persist. Also added a case proving getSecrets forwards the clamped (not raw oversized) requestTimeoutMs to a custom queryFunction - previously only downloadFile/downloadThumbnail's direct platform.get path was proven clamped. Full suite 235/235 (dist rebuilt before this run - keeper.test.ts imports via '../', which resolves to dist, not src, so edits to keeper.ts need a rebuild to be reflected there). * fix(javascript): drain the browser fileUpload response body (KSM-1209) Round-4 review, non-blocking item 13. fileUpload resolves off headers alone and never reads the body, same gap the Node platform had (fixed earlier in this PR via res.resume()). Lower severity here - no demonstrated hang in a browser context - but the same class of leaving an unconsumed response stream dangling. res.body?.cancel() drains it, swallowing any cancellation error since nothing here needs the body anyway. New regression test confirmed failing against the unfixed code first: mocks a response whose body.cancel is a spy, asserts it was called. The existing tests' default fetch mock has no body property at all, so the fix's optional-chained call safely no-ops for them - full suite 236/236, tsc clean. * docs(javascript): amend the KSM-1209 CHANGELOG entry for round-4 fixes Adds the O(n^2) buffering fix, the caching-fallback validation-ordering and cache-write-isolation fixes, uploadFile's validate-before-side-effect fix, the getSecrets write-ordering fix, the browser fileUpload body drain, and the example fix to the existing entry rather than replacing it. * test(javascript): delete keeper.test.ts's duplicate timeout-propagation block (KSM-1209) Test-consolidation pass, found via an anti-pattern audit requested separately from Mateo's review: this "request timeout propagation" describe block (added within this same PR) almost entirely duplicated test/timeout.test.ts (also added within this same PR) - same layer, same import surface, same assertions differing only in magic numbers. Removing it here, first, as a pure deletion; the few cases it had that timeout.test.ts lacks (the MAX_REQUEST_TIMEOUT_MS-clamped case for downloadFile/downloadThumbnail, uploadFile's explicit-timeout-wins case, and the two round-8 gap-closing cases just added) get merged into timeout.test.ts next, strengthened where they didn't actually prove what their name claimed. Removed now-unused imports (downloadFile, downloadThumbnail, uploadFile, KeeperFile, KeeperRecord, MAX_REQUEST_TIMEOUT_MS) - DEFAULT_REQUEST_TIMEOUT_MS stays, its own standalone test is unrelated to the deleted block. tsc --noEmit clean, full suite 221/236 (15 tests removed, none of them irreplaceable - see the merge that follows). * test(javascript): merge unique cases into timeout.test.ts (KSM-1209) Test-consolidation pass, completing the split started in the previous commit. timeout.test.ts is now the sole home for request-timeout propagation tests, matching the deadline.test.ts precedent of one dedicated file per concern. Merged in, from the block deleted in the previous commit: - the two round-8 gap-closing cases (no side effects from an invalid requestTimeoutMs, clamped forwarding through a custom queryFunction) - downloadFile's MAX_REQUEST_TIMEOUT_MS-clamped case (not duplicated for downloadThumbnail, which already has its own single case proving it shares the same plumbing - re-testing every case on both would be the same anti-pattern this consolidation exists to fix) - uploadFile's "explicit timeoutMs wins" case Trimmed the invalid-timeout sweep from 5 values to 1 representative value (0) - the other 4 are already unit-tested at the resolveTimeoutMs level in deadline.test.ts and don't differentiate fixed/unfixed code at this integration layer. Strengthened uploadFile's invalid-timeout test, which didn't actually prove what it claimed: it only asserted platform.fileUpload wasn't called, true regardless of validation ordering since fileUpload is the last call in the function either way. Confirmed by reverting the ordering fix and finding the test still passed. Rewritten to use a valid options.requestTimeoutMs with an invalid explicit timeoutMs argument, isolating uploadFile's own validation from postQuery's separate, pre-existing validation of options.requestTimeoutMs, and to also assert the add_file network call was never made. Confirmed failing against the unfixed ordering, then restored. tsc --noEmit clean, full suite 220/220. * docs(javascript): clarify per-attempt vs per-call timeout budget (KSM-1209) Round-4 review, non-blocking item 12. requestTimeoutMs bounds each individual attempt inside postQuery's throttle/key-rotation retry loop, not the call as a whole, and the sleep between retries is itself unbounded - a call under sustained throttling can run longer in total than the configured value. Not a code change (reviewer flagged it as "worth a CHANGELOG line", not a defect), filed as follow-up tickets KSM-1364 (item 6, examples/integration outside jest's roots) and KSM-1365 (item 14, deferred dedup cleanup) rather than fixed inline. * fix(javascript): restore armRequest's settlement path when no socket is assigned (KSM-1209) Node only emits 'error' on a ClientRequest for an abort once a socket has been assigned, so removing the abort listener in a prior round left a request stalled on a proxy CONNECT or a saturated agent pool with no settlement path at all, regardless of the configured deadline. Both listeners now run, guarded against double-settlement. * test(javascript): guard round-5's regression findings in nodePlatform.test.ts (KSM-1209) Four fail-then-pass additions, each verified against the pre-fix code before landing: - a socketless-abort case for armRequest, driving the mock without simulating a socket assignment - a scaling assertion for the response-buffering fix, spying on Buffer.concat's call count rather than timing so it can't flake, plus a sibling empty-body case pinning data to null instead of a zero-length Buffer - a rejection handler attached before the AbortController-unavailable test drives its mock, so a removed guard reports a clean failure instead of crashing the Jest worker * test(javascript): restore downloadThumbnail coverage, cover postFunction forwarding (KSM-1209) downloadFile and downloadThumbnail are two hand-copied expressions of the same precedence rule with no shared helper, so they can drift independently; a prior consolidation left downloadThumbnail's own precedence unobservable (its one case passed undefined for timeoutMs). Converted the precedence/clamping suite into a describe.each over both functions and their own URL field, restoring full coverage on downloadThumbnail without duplicating test bodies. Also added a test for postFunction, the default queryFunction every consumer who supplies no custom one goes through - previously the only network call site with no forwarding coverage at all. * docs(javascript): document the file-transfer throughput floor, fix two inaccuracies (KSM-1209) The 30s default bounds the whole response/request body, so it also acts as a minimum-throughput requirement on a large file transfer, not just a liveness check on an API call - now stated in the CHANGELOG alongside the existing default/override documentation. Also corrects two factual errors from a prior round: the CHANGELOG said the sleep between throttle retries is unbounded, contradicting the KSM-1035 bullet two lines above it (it's capped at 176s plus up to 25% jitter); and a comment above downloadFile claimed timeoutMs had a prior published argument position, which never existed since neither downloadFile nor downloadThumbnail took more than one argument before this PR. Split the oversized KSM-1209 CHANGELOG bullet into three, keying the response-buffering fix to its own ticket (KSM-1342). --------- Co-authored-by: Mateo Gallego <mgallego@keepersecurity.com>
…-task-lib, js-yaml, minimatch, brace-expansion, serialize-javascript, uuid, follow-redirects, nodejs-file-downloader; force-bump mocha to 12.0.1 (dev-only) for diff/serialize-javascript (#1169)
mgallego-keeper
marked this pull request as draft
September 16, 2026 17:33
This branch has not been deployed
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
1 participant
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
Release branch for v1.2.0 — add Node24 execution handler support before Node 20 EOL (2026-04-30), upgrade all dependencies, and fix tests to run without real vault credentials.
Changes
New Features
Maintenance
azure-pipelines-task-lib^4.17.3 → ^5.2.4 (Node24 minimum requirement)@keeper-security/secrets-manager-core^16.6.3 → ^17.2.0typescript^5.1.6 → ^5.8.3,@types/node^20.3.1 → ^24.10.0mocha^10.7.3 → ^11.7.1,dotenv^16.4.5 → ^17.2.0,@types/mocha^10.0.9 → ^10.0.10tsconfig.jsonfor IDE supportnode:10tonode:20actions/checkout@v4,actions/setup-node@v4,node-version: '24'Breaking Changes
None. All existing execution handlers (Node10, Node16, Node20_1) are preserved. The agent automatically selects the highest compatible handler.
Testing
Related Issues