From a62e24e934ead885854ea99d94d8cc33d4ba364e Mon Sep 17 00:00:00 2001 From: Stas Schaller Date: Fri, 4 Sep 2026 13:59:23 -0400 Subject: [PATCH 1/6] fix(javascript): secure the caching fallback (KSM-1265) Replaces cachingPostFunction, which kept the AES key in plaintext beside the ciphertext it protected in a fixed CWD-relative file with no integrity check, with createCachingFunction: an encrypted, integrity- checked, staleness-bounded cache derived from the app key, at a configurable non-CWD default path. Squashed from 3 review-round commits (a5c75e98, 7ca19729, 851d5f47) before rebasing onto KSM-1266's moved tip, per the standing one-commit- per-ticket convention and to avoid resolving the same rebase conflict three times against an intermediate, since-superseded design. --- .../custom-caching-function-support/hello.js | 50 +-- sdk/javascript/packages/core/CHANGELOG.md | 10 + .../packages/core/internal/quicktest.ts | 4 +- sdk/javascript/packages/core/package.json | 13 + .../core/src/browser/localConfigStorage.ts | 47 ++- sdk/javascript/packages/core/src/cache.ts | 67 ++++ sdk/javascript/packages/core/src/keeper.ts | 2 +- .../core/src/node/localConfigStorage.ts | 242 +++++++++---- .../test/browserLocalConfigStorage.test.ts | 183 ++++++++++ .../packages/core/test/cache.test.ts | 5 + .../test/localConfigStorage.homedir.test.ts | 23 ++ .../core/test/localConfigStorage.test.ts | 329 +++++++++++++++++- 12 files changed, 847 insertions(+), 128 deletions(-) create mode 100644 sdk/javascript/packages/core/src/cache.ts create mode 100644 sdk/javascript/packages/core/test/browserLocalConfigStorage.test.ts create mode 100644 sdk/javascript/packages/core/test/cache.test.ts create mode 100644 sdk/javascript/packages/core/test/localConfigStorage.homedir.test.ts diff --git a/examples/javascript/custom-caching-function-support/hello.js b/examples/javascript/custom-caching-function-support/hello.js index bfdea683d..8370061e6 100644 --- a/examples/javascript/custom-caching-function-support/hello.js +++ b/examples/javascript/custom-caching-function-support/hello.js @@ -1,58 +1,22 @@ -const fs = require('fs'); - const { getSecrets, initializeStorage, localConfigStorage, - postFunction + createCachingFunction } = require('@keeper-security/secrets-manager-core') -const CACHE_FILENAME = 'cache.dat'; - -// This is basic example of creating custom caching function -// ⓘ This will store only last request, however you can use any tool to extend this functionality -// ⓘ Stale cache entries can cause version mismatches if records are updated from other keepersecurity utils. Prefer fresh reads - -const cachingPostFunction = async (url, transmissionKey, payload, allowUnverifiedCertificate) => { - try { - const response = await postFunction( - url, - transmissionKey, - payload, - allowUnverifiedCertificate - ) - - if (response.statusCode == 200) { - fs.writeFileSync(CACHE_FILENAME, Buffer.concat([transmissionKey.key, response.data])) - } - - return response - } catch (e) { - console.error(e) - let cachedData - try { - cachedData = fs.readFileSync(CACHE_FILENAME) - } catch { - } - if (!cachedData) { - throw new Error('Cached value does not exist') - } - console.log('Using cached data') - transmissionKey.key = cachedData.slice(0, 32) - return { - statusCode: 200, - data: cachedData.slice(32), - headers: [] - } - } -} +// This is a basic example of using the SDK's built-in caching function. +// ⓘ createCachingFunction stores only the last successful request, but you can supply your own +// queryFunction to extend this behavior. +// ⓘ Stale cache entries can cause version mismatches if records are updated from other keepersecurity +// utils. createCachingFunction rejects cache entries older than its maxCacheAgeMs (default 24h). const getKeeperRecords = async () => { const storage = localConfigStorage("config.json") const options = { storage, - queryFunction: cachingPostFunction + queryFunction: createCachingFunction(storage) } // if your Keeper Account is in other region than US, update the hostname accordingly diff --git a/sdk/javascript/packages/core/CHANGELOG.md b/sdk/javascript/packages/core/CHANGELOG.md index ba8c28fb8..3a2b18516 100644 --- a/sdk/javascript/packages/core/CHANGELOG.md +++ b/sdk/javascript/packages/core/CHANGELOG.md @@ -13,6 +13,16 @@ - KSM-1263 - Fixed config and cache file permissions not being re-applied on every write. `fs.openSync`'s mode argument only takes effect when a file is created, so a config or cache file that already existed with looser permissions kept them; permissions are now explicitly reset to 0600 after every write. - KSM-1267 - `getFolders()` now classifies why an undecryptable folder was skipped (`integrity`, `format`, `missing-key`, or `malformed-data`) instead of logging an opaque, unclassified error, and logs one summary line naming every folder UID it had to omit. Added an optional `onDecryptionError` callback to `SecretManagerOptions`, invoked once per skipped folder, so a caller can react to or throw to fail closed on a partial result; existing callers that do not set it see no behavior change. Both the Node and browser platforms' `unwrap()` now reject an unwrapped key of the wrong length immediately (a corrupted-but-plausible 16- or 24-byte result was previously accepted by both platforms and cached, failing later at an unrelated call site with a much harder to diagnose error). The underlying finding (the shared-folder key wrap uses unauthenticated AES-256-CBC, a format fixed server-side that the SDK cannot change unilaterally) was reviewed and confirmed low-impact: a manipulated folder key is still caught by the existing AES-GCM authentication on the record keys inside that folder. - KSM-1266 - Fixed `localConfigStorage` treating every config-read failure as "no config yet." A missing file is still a legitimate fresh start, and so now is one left completely empty by a process killed mid-save (a partially-written file is not covered by this - there is no reliable way to distinguish a truncated write from genuine corruption, so it still throws). Permission errors, malformed JSON, invalid UTF-8 byte sequences, and JSON that parses but isn't an object (`null`, a number, an array) now throw a typed `KeeperError` instead of silently starting fresh or misbehaving on first use. A leading UTF-8 BOM (produced by tools like Windows Notepad or PowerShell's `Set-Content`) is stripped and the file is read normally, not treated as corruption. `saveStorage`'s write path now writes to a temporary file and renames it into place atomically, instead of truncating the destination before writing (which could previously leave a 0-byte file on disk after a failed write), and wraps its own failures (e.g. `EACCES`, `ENOSPC`) in the same `KeeperError` guarantee. Node validates config readability eagerly, at construction; the browser `localConfigStorage` (KSM-1332, same release) defers the equivalent check lazily to first storage access, since IndexedDB has no synchronous API to check eagerly against - this timing difference between the two platforms is expected and now documented in-code. The write path now resolves a symlinked config path and writes through the real file. It no longer replaces the symlink. Some deployments manage a "current config" symlink this way. This also covers a symlink whose target does not exist yet, for example a symlink an ops tool creates before the target file exists, including a chain of such symlinks up to 40 hops deep, the same bound the operating system's own resolution enforces. A resolution failure partway through that chain, for example a permissions error on an intermediate link, now fails the save instead of silently writing through the wrong path. A hard-linked config path now goes through the same atomic write as any other file. Only the resolved path gets the update; a second hard-linked name keeps its old content, because the atomic write always creates a new file at the resolved path. Before this fix, a hard-linked config path was written in place, so every hard link saw the update, but that write was not atomic: a failure partway through could corrupt the file with no recovery. An atomic write now needs write and execute permission on the config file's directory, not just the file itself. A directory locked down to file-only write access will fail every save from now on. If a crash happens between opening the temporary file and the rename, the SDK now removes the leftover file automatically on the next read. Before this fix, the file stayed on disk indefinitely. That cleanup resolves the config path the same way the write path does, so it also finds the leftover file when the config path is itself a symlink, including one whose target didn't exist at write time. A failed save no longer leaves the in-memory value ahead of the value on disk. `localConfigStorage` now throws the new `KeeperStorageError`, which extends `KeeperError`. Its `code` field carries the original filesystem error code, for example `EACCES` or `ENOSPC`, when one exists. Concurrent `saveString`/`saveBytes`/`delete` calls on the same `localConfigStorage` instance now run one at a time. Before this fix, two overlapping calls could interleave so that a failed save's rollback erased a different, already-successful call's data. +- KSM-1265 - **BREAKING (Node only):** Security fix (CWE-312, CWE-345): the Node `cachingPostFunction` stored its AES transmission key in plaintext next to the ciphertext it protected, in a fixed path relative to the process's working directory, and restored it with no integrity check. Replaced it with `createCachingFunction(storage, options?)`: + ``` + // before + queryFunction: cachingPostFunction + // after + queryFunction: createCachingFunction(storage) + // with a custom cache path or freshness window + queryFunction: createCachingFunction(storage, {cachePath, maxCacheAgeMs}) + ``` + The cache is now encrypted with a key derived from the app key already held in the config (so reading the cache requires the config, not just the cache file), authenticated so a tampered or corrupted file is rejected instead of silently trusted, bounded by a configurable freshness window (default 24h), and located at `~/.keeper/ksm-cache.dat` by default instead of the working directory. Usage was limited to the opt-in caching example, which has been updated to use the new function. If you called `cachingPostFunction` directly, delete the old cache file in your working directory after upgrading; it is not removed automatically. `cachePath` and `maxCacheAgeMs` are now named fields on an options object instead of positional arguments, since Node's and the browser's second positional argument meant different things; the browser signature (`createCachingFunction(storage, maxCacheAgeMs?)`) is unchanged and still non-breaking there, since the new `maxCacheAgeMs` parameter is optional and an old-format cached value is simply treated as a cache miss. The cache file and directory reject a symlink on write (replaced outright, never written through - there is no legitimate externally-managed symlink convention for a path the SDK itself names) and on read; the config file's own symlink handling is unchanged and intentionally different, see the KSM-1266 entry above. Both the config file and the cache file are now written atomically (to a temporary file, then renamed into place, via one shared primitive), so a write that fails partway through can no longer leave a corrupted or truncated file behind. Known limitation: the very first (bind) call's response is not cached, since caching requires an app key that the bind call itself establishes; every call after that caches normally. In the browser, when the app key is held as a non-extractable `CryptoKey` (`useObjects: true`), caching is a no-op rather than an error, the same graceful degradation already used for a network failure with no prior cache. A network failure served from cache now logs a warning, since the caller is getting a response that may be stale. This package now also declares an `exports` field so bundlers and modern TypeScript resolve the correct platform-specific type declarations for the browser bundle; a consumer still on TypeScript's legacy `moduleResolution: "node"` continues to see the Node type declarations regardless, unchanged from before. - Maintenance: Updated `minimatch`, `@babel/core`, and `handlebars` dev dependencies. ## 17.5.0 diff --git a/sdk/javascript/packages/core/internal/quicktest.ts b/sdk/javascript/packages/core/internal/quicktest.ts index 4b5cfb752..23f841bdf 100644 --- a/sdk/javascript/packages/core/internal/quicktest.ts +++ b/sdk/javascript/packages/core/internal/quicktest.ts @@ -10,7 +10,7 @@ import { import {nodePlatform} from '../src/node/nodePlatform'; import {connectPlatform} from '../src/platform'; import {inspect} from 'util'; -import {cachingPostFunction, localConfigStorage} from "../src/node"; +import {createCachingFunction, localConfigStorage} from "../src/node"; process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0' @@ -26,7 +26,7 @@ async function test() { await initializeStorage(kvs, oneTimeToken) const options: SecretManagerOptions = { storage: kvs, - // queryFunction: cachingPostFunction + // queryFunction: createCachingFunction(kvs) allowUnverifiedCertificate: true } const { records } = await getSecrets(options) diff --git a/sdk/javascript/packages/core/package.json b/sdk/javascript/packages/core/package.json index aea0d94d0..5bf22d42e 100644 --- a/sdk/javascript/packages/core/package.json +++ b/sdk/javascript/packages/core/package.json @@ -5,6 +5,19 @@ "browser": "dist/index.es.js", "main": "dist/index.cjs.js", "types": "dist/node/index.d.ts", + "exports": { + ".": { + "types": { + "browser": "./dist/browser/index.d.ts", + "default": "./dist/node/index.d.ts" + }, + "browser": "./dist/index.es.js", + "import": "./dist/index.es.js", + "require": "./dist/index.cjs.js", + "default": "./dist/index.cjs.js" + }, + "./package.json": "./package.json" + }, "repository": "https://github.com/Keeper-Security/secrets-manager", "author": "sm@keepersecurity.com", "license": "MIT", diff --git a/sdk/javascript/packages/core/src/browser/localConfigStorage.ts b/sdk/javascript/packages/core/src/browser/localConfigStorage.ts index c7d429def..b261724e4 100644 --- a/sdk/javascript/packages/core/src/browser/localConfigStorage.ts +++ b/sdk/javascript/packages/core/src/browser/localConfigStorage.ts @@ -1,5 +1,8 @@ import {EncryptedPayload, KeeperHttpResponse, KeyValueStorage, TransmissionKey, platform} from "../platform"; import {KeeperError} from "../errors"; +import {KEY_APP_KEY, deriveCacheKey, encodeCacheBlob, decodeCacheBlob, DEFAULT_MAX_CACHE_AGE_MS, isRawKeyBytes} from "../cache"; + +const CACHE_STORAGE_KEY = 'cache' type Reject = (reason: Error) => void @@ -202,24 +205,38 @@ export const secureStorage = async (dbName: string): Promise => } } -export function createCachingFunction(storage: KeyValueStorage): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload) => Promise { +// Same cache codec (../cache) as node/localConfigStorage.ts's createCachingFunction; only the +// storage medium differs (IndexedDB here, a file there). Replaces the old plaintext +// key-beside-data format (CWE-312, CWE-345) with one encrypted under a key derived from the app +// key, authenticated, and bounded by a freshness window. An old-format cached value simply fails +// the version check and is treated as a cache miss, the same graceful degradation the Node fix +// uses for its old-format files. +export function createCachingFunction(storage: KeyValueStorage, maxCacheAgeMs: number = DEFAULT_MAX_CACHE_AGE_MS): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload) => Promise { return async (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload): Promise => { + let response: KeeperHttpResponse try { - const response = await platform.post(url, payload.payload, { + response = await platform.post(url, payload.payload, { PublicKeyId: transmissionKey.publicKeyId.toString(), TransmissionKey: platform.bytesToBase64(transmissionKey.encryptedKey), Authorization: `Signature ${platform.bytesToBase64(payload.signature)}` }) - if (response.statusCode == 200) { - await storage.saveBytes('cache', new Uint8Array([...transmissionKey.key, ...response.data])) - } - return response } catch (e) { - const cachedData = await storage.getBytes('cache') - if (!cachedData) { - throw new Error('Cached value does not exist') + const appKey = await storage.getBytes(KEY_APP_KEY) + if (!appKey || !isRawKeyBytes(appKey)) { + throw new KeeperError('Cached value does not exist') + } + const raw = await storage.getBytes(CACHE_STORAGE_KEY) + if (!raw) { + throw new KeeperError('Cached value does not exist') + } + let cachedData: Uint8Array + try { + cachedData = await decodeCacheBlob(raw, await deriveCacheKey(appKey), maxCacheAgeMs) + } catch (e2: Error | any) { + throw new KeeperError(`Cached value is invalid: ${e2.message}`) } + console.error(`Network request failed (${describeCause(e)}); serving cached response, which may be stale`) transmissionKey.key = cachedData.slice(0, 32) return { statusCode: 200, @@ -227,5 +244,17 @@ export function createCachingFunction(storage: KeyValueStorage): (url: string, t headers: [] } } + if (response.statusCode == 200) { + try { + const appKey = await storage.getBytes(KEY_APP_KEY) + if (appKey && isRawKeyBytes(appKey)) { + const blob = await encodeCacheBlob(new Uint8Array([...transmissionKey.key, ...response.data]), await deriveCacheKey(appKey)) + await storage.saveBytes(CACHE_STORAGE_KEY, blob) + } + } catch (e: Error | any) { + console.error(`Failed to update cached response: ${e.message}`) + } + } + return response } } \ No newline at end of file diff --git a/sdk/javascript/packages/core/src/cache.ts b/sdk/javascript/packages/core/src/cache.ts new file mode 100644 index 000000000..754f060df --- /dev/null +++ b/sdk/javascript/packages/core/src/cache.ts @@ -0,0 +1,67 @@ +import {platform} from "./platform"; + +// Single source of truth for the app-key storage slot; keeper.ts imports this rather than +// declaring its own copy of the literal. +export const KEY_APP_KEY = 'appKey' + +const CACHE_KEY_LABEL = new TextEncoder().encode('KSM-cache-v1') +export const CACHE_FORMAT_VERSION = 0x02 +export const DEFAULT_MAX_CACHE_AGE_MS = 24 * 60 * 60 * 1000 + +// No Buffer here - this module is shared with the browser bundle, which has no Buffer global. +const concatBytes = (...parts: Uint8Array[]): Uint8Array => { + const length = parts.reduce((sum, part) => sum + part.length, 0) + const out = new Uint8Array(length) + let offset = 0 + for (const part of parts) { + out.set(part, offset) + offset += part.length + } + return out +} + +const readTimestamp = (data: Uint8Array): number => { + const view = new DataView(data.buffer, data.byteOffset, data.byteLength) + return Number(view.getBigUint64(0, false)) +} + +const writeTimestamp = (ms: number): Uint8Array => { + const out = new Uint8Array(8) + new DataView(out.buffer).setBigUint64(0, BigInt(ms), false) + return out +} + +// Under useObjects: true (browser), unwrap() stores the app key as a non-extractable CryptoKey +// rather than raw bytes, so it can never be handed to platform.getHmacDigest below. Exported so +// both platforms can pre-check appKey before ever calling deriveCacheKey, rather than relying on +// the TypeError below to propagate and be caught somewhere. +export const isRawKeyBytes = (value: unknown): value is Uint8Array => value instanceof Uint8Array + +export const deriveCacheKey = (appKey: Uint8Array): Promise => { + if (!isRawKeyBytes(appKey)) { + throw new TypeError('deriveCacheKey requires appKey to be raw key bytes (Uint8Array)') + } + return platform.getHmacDigest('SHA256', appKey, CACHE_KEY_LABEL) +} + +// The freshness timestamp goes inside the encrypted payload, not a cleartext header, so GCM's +// auth tag covers it too - otherwise the staleness check could be bypassed by editing the +// timestamp bytes without touching the ciphertext at all. +export const encodeCacheBlob = async (plaintext: Uint8Array, cacheKey: Uint8Array): Promise => { + const encrypted = await platform.encryptWithKey(concatBytes(writeTimestamp(Date.now()), plaintext), cacheKey) + return concatBytes(new Uint8Array([CACHE_FORMAT_VERSION]), encrypted) +} + +export const decodeCacheBlob = async (raw: Uint8Array, cacheKey: Uint8Array, maxCacheAgeMs: number): Promise => { + if (raw.length < 1 || raw[0] !== CACHE_FORMAT_VERSION) { + throw new Error('cache blob is not in a recognized format') + } + const decrypted = await platform.decryptWithKey(raw.subarray(1), cacheKey) + if (decrypted.length < 8) { + throw new Error('cache blob is not in a recognized format') + } + if (Date.now() - readTimestamp(decrypted.subarray(0, 8)) > maxCacheAgeMs) { + throw new Error(`cached value is stale (age exceeds ${maxCacheAgeMs}ms)`) + } + return decrypted.subarray(8) +} diff --git a/sdk/javascript/packages/core/src/keeper.ts b/sdk/javascript/packages/core/src/keeper.ts index 9f5f12382..660b5cee3 100644 --- a/sdk/javascript/packages/core/src/keeper.ts +++ b/sdk/javascript/packages/core/src/keeper.ts @@ -2,6 +2,7 @@ import {EncryptedPayload, KeeperHttpResponse, KeyValueStorage, platform, Transmi import {webSafe64FromBytes, webSafe64ToBytes, tryParseInt} from './utils' import {parseNotation} from './notation' import {KeeperError, KeeperThrottleError, KeeperCryptoError, KeeperCryptoFailureReason, KeeperDecryptionErrorInfo} from './errors' +import {KEY_APP_KEY} from './cache' export {KeyValueStorage} from './platform' @@ -11,7 +12,6 @@ const KEY_SERVER_PUBLIC_KEY_ID = 'serverPublicKeyId' const KEY_SERVER_PUBLIC_KEY = 'serverPublicKey' const KEY_CLIENT_ID = 'clientId' const KEY_CLIENT_KEY = 'clientKey' // The key that is used to identify the client before public key -const KEY_APP_KEY = 'appKey' // The application key with which all secrets are encrypted const KEY_OWNER_PUBLIC_KEY = 'appOwnerPublicKey' // The application owner public key, to create records const KEY_PRIVATE_KEY = 'privateKey' // The client's private key diff --git a/sdk/javascript/packages/core/src/node/localConfigStorage.ts b/sdk/javascript/packages/core/src/node/localConfigStorage.ts index dbcd5e427..72c83d2dc 100644 --- a/sdk/javascript/packages/core/src/node/localConfigStorage.ts +++ b/sdk/javascript/packages/core/src/node/localConfigStorage.ts @@ -1,7 +1,9 @@ import {EncryptedPayload, KeeperHttpResponse, KeyValueStorage, platform, TransmissionKey, inMemoryStorage} from "../platform"; -import {KeeperStorageError} from "../errors"; +import {KeeperError, KeeperStorageError} from "../errors"; +import {KEY_APP_KEY, deriveCacheKey, encodeCacheBlob, decodeCacheBlob, DEFAULT_MAX_CACHE_AGE_MS, isRawKeyBytes} from "../cache"; import * as fs from 'fs'; import * as path from 'path'; +import * as os from 'os'; import {randomBytes} from 'crypto'; // fs.openSync's mode argument is only honored when the file is created; it is a no-op on an @@ -34,7 +36,7 @@ const errorCode = (cause: unknown): string | undefined => { const isEnoent = (cause: unknown): boolean => typeof cause === 'object' && cause !== null && (cause as NodeJS.ErrnoException).code === 'ENOENT' -// Write-then-rename instead of truncate-then-write: fs.openSync(configName, 'w', ...) +// Write-then-rename instead of truncate-then-write: fs.openSync(finalPath, 'w', ...) // truncates the destination before a single byte of new content lands, so a write // failure used to leave a 0-byte file behind, which the empty-file self-heal in // readStorage then treated as a legitimate fresh start on the next read - silently @@ -44,30 +46,43 @@ const isEnoent = (cause: unknown): boolean => // too. fsyncSync before the rename means this survives a real power-loss event, not // just a killed process. // -// Resolving through realpathSync first (falling back to the literal path on ENOENT, -// i.e. a first-ever write) means a symlinked configName - an externally-managed -// "current config" convention, or the destination a caller's own tooling maintains - -// gets written through rather than replaced by renameSync, which operates on the link -// itself and never dereferences it (this is specified POSIX rename(2) behavior, not a -// bug to work around here). This is the same approach the write-file-atomic package -// (the de facto standard for this in the npm ecosystem) uses. realpathSync's ENOENT also -// covers a dangling symlink - configName IS a symlink, but its target doesn't exist yet -// (e.g. ops tooling pre-provisions config.json -> /secure/actual-config.json before -// actual-config.json exists) - and the catch below resolves that case through the link's -// own target rather than falling back to the literal symlink path, so the same -// written-through guarantee holds there too. +// Deliberately does NOT resolve a symlink at finalPath - it operates on the literal path +// given, and renameSync replaces whatever directory entry is there (a symlink included) +// rather than dereferencing it (specified POSIX rename(2) behavior, not a bug to work around +// here). This is the safe default: a caller-supplied or attacker-plantable symlink at the +// write destination is never followed. The config file's write-through-a-symlink behavior +// (an explicit, opt-in feature for an externally-managed "current config" convention, +// including a dangling/pre-provisioned symlink whose target doesn't exist yet) is the +// caller's own choice, made by saveStorage resolving configName via realpathSync *before* +// calling in here - see saveStorage below. The cache file (writeCacheFile) deliberately does +// not do that resolution: there is no legitimate externally-managed symlink convention for a +// path the SDK itself names and owns, so a symlink there is presumptively hostile and must be +// replaced, never written through (this is exactly the arbitrary-file-overwrite KSM-1265's +// security fix closes - regression test: "a symlink at the cache path is replaced by the +// write, never followed"). // -// A hard link is a second directory entry for the same inode, not something realpathSync -// resolves - renaming a temp file over one hard-linked path always creates a new inode -// there, leaving every other hard-linked path frozen at the old content. Accepted rather -// than worked around: write-file-atomic, npm and pip all make the same trade, since the -// alternative - writing in place to keep the shared inode - gives up the rename path's -// atomicity for that one file. +// A hard link is a second directory entry for the same inode, not something symlink +// resolution helps with either way - renaming a temp file over one hard-linked path always +// creates a new inode there, leaving every other hard-linked path frozen at the old content. +// Accepted rather than worked around, for both the config file and the cache file: write- +// file-atomic, npm and pip all make the same trade, since the alternative - writing in place +// to keep the shared inode - gives up this function's atomicity for that one file. // +// Shared by both the config file (saveStorage) and the cache file (writeCacheFile) - one +// atomic-write primitive, so a protection added for one (fsync-before-rename, the short-write +// guard below) isn't something the other has to reimplement or drift out of sync with. +// +// fs.writeSync's overloads don't distribute over a string | Uint8Array union, so the two data +// types need their own branch rather than one unbranched call. +const dataByteLength = (data: string | Uint8Array): number => + typeof data === 'string' ? Buffer.byteLength(data) : data.byteLength + // Same bound Linux's own symlink resolution enforces (SYMLOOP_MAX/MAXSYMLINKS is 40 on // every platform this package ships for), so a legitimate deep chain isn't cut short and a -// real loop still terminates. Shared with cleanupOrphanedTempFiles so the write path and the -// sweep always agree on where a dangling symlink's write actually landed. +// real loop still terminates. Shared by saveStorage's own opt-in symlink-following write (see +// writeFileAtomic's comment above for why the cache path deliberately does not opt in) and by +// cleanupOrphanedTempFiles, so the write path and the sweep always agree on where a dangling +// symlink's write actually landed. const MAX_SYMLINK_HOPS = 40 const resolveWriteTargetPath = (configName: string): string => { @@ -105,20 +120,18 @@ const resolveWriteTargetPath = (configName: string): string => { throw new Error(`Too many levels of symbolic links resolving ${configName}`) } -const writeConfigFile = (configName: string, data: string): void => { - const resolvedPath = resolveWriteTargetPath(configName) - - const tmpPath = `${resolvedPath}.${process.pid}.${randomBytes(6).toString('hex')}.tmp` +const writeFileAtomic = (finalPath: string, data: string | Uint8Array): void => { + const tmpPath = `${finalPath}.${process.pid}.${randomBytes(6).toString('hex')}.tmp` const fd = fs.openSync(tmpPath, 'w', 0o600) try { // A single fs.writeSync call is not guaranteed to write the whole buffer - POSIX // write(2) can return fewer bytes than requested. Comparing the result against the // intended length, rather than trusting it and fsync-ing whatever actually landed, is - // what keeps a rare short write from silently committing truncated JSON as the new - // config. Deliberately not a retry loop (one existed here before and was removed for + // what keeps a rare short write from silently committing truncated content as the new + // file. Deliberately not a retry loop (one existed here before and was removed for // hang risk) - a short write becomes an immediate failure instead of something to retry. - const bytesWritten = fs.writeSync(fd, data) - const expectedBytes = Buffer.byteLength(data) + const bytesWritten = typeof data === 'string' ? fs.writeSync(fd, data) : fs.writeSync(fd, data) + const expectedBytes = dataByteLength(data) if (bytesWritten !== expectedBytes) { throw new Error(`Short write: wrote ${bytesWritten} of ${expectedBytes} bytes to ${tmpPath}`) } @@ -145,7 +158,7 @@ const writeConfigFile = (configName: string, data: string): void => { // must already be 0600 before the rename. chmodSecure(tmpPath) try { - fs.renameSync(tmpPath, resolvedPath) + fs.renameSync(tmpPath, finalPath) } catch (e) { try { fs.unlinkSync(tmpPath) @@ -157,7 +170,7 @@ const writeConfigFile = (configName: string, data: string): void => { } } -// A SIGKILL/OOM between opening the temp file and the rename in writeConfigFile leaves the +// A SIGKILL/OOM between opening the temp file and the rename in writeFileAtomic leaves the // temp file behind permanently - it's never read back as config data, but it does hold a full // snapshot of every secret that was in storageData at that moment, so it shouldn't just sit on // disk forever. Swept here, on the next read, rather than at write time, since the crash that @@ -173,12 +186,12 @@ const writeConfigFile = (configName: string, data: string): void => { const ORPHANED_TEMP_FILE_MAX_AGE_MS = 60_000 const cleanupOrphanedTempFiles = (configName: string): void => { - // Shares writeConfigFile's own resolution rather than a simpler copy, so the sweep looks in - // the same directory the write actually landed in - including a dangling symlink whose - // target lives in a different directory than the link itself. Best-effort: any failure - // (including a readlink error, or a chain longer than resolveWriteTargetPath tolerates) - // just skips the sweep for this read, matching this function's existing contract that a - // sweep problem never fails a read. + // Shares the same resolveWriteTargetPath the config write path (saveStorage) uses, rather + // than a simpler copy, so the sweep looks in the same directory the write actually landed + // in - including a dangling symlink whose target lives in a different directory than the + // link itself. Best-effort: any failure (including a readlink error, or a chain longer + // than resolveWriteTargetPath tolerates) just skips the sweep for this read, matching this + // function's existing contract that a sweep problem never fails a read. let resolvedPath: string try { resolvedPath = resolveWriteTargetPath(configName) @@ -200,7 +213,7 @@ const cleanupOrphanedTempFiles = (configName: string): void => { if (!entry.startsWith(prefix) || !entry.endsWith(suffix)) { continue } - // Matches the exact shape writeConfigFile produces (.<12 hex chars>) so this never + // Matches the exact shape writeFileAtomic produces (.<12 hex chars>) so this never // sweeps an unrelated file that merely shares the config's name as a prefix. const middle = entry.slice(prefix.length, entry.length - suffix.length) if (!/^\d+\.[0-9a-f]{12}$/.test(middle)) { @@ -226,6 +239,12 @@ export const localConfigStorage = (configName?: string): KeyValueStorage => { // getString/saveString/delete, because IndexedDB has no synchronous API to check eagerly // against - a structural difference between the two platforms, not a // stylistic one. + // + // Deliberately does not reject a symlinked configName. Kubernetes always mounts a Secret or + // ConfigMap as a symlink chain (configName -> ..data/ -> a timestamped directory), so + // rejecting a read through a symlink here breaks every pod that mounts config.json this way. + // Reading through a symlink was never the vulnerability - only a write redirected through one + // is - so symlink protection lives on the write path (saveStorage) instead. const readStorage = (): any => { if (!configName) { return {} @@ -247,7 +266,7 @@ export const localConfigStorage = (configName?: string): KeyValueStorage => { throw new KeeperStorageError(`Unable to read local config ${configName}: ${describeCause(e)}`, errorCode(e)) } // An empty (or whitespace-only) file is a legitimate fresh start, not corruption. - // writeConfigFile's atomic rename path can no longer produce this itself - a kill + // writeFileAtomic's atomic rename path can no longer produce this by itself - a kill // mid-write only ever leaves a temp file behind, configName itself is untouched until // the rename completes - but some other writer entirely (a stray `echo -n > // config.json`, a pre-atomic-write version of this SDK) still can. The sibling KMS @@ -283,7 +302,14 @@ export const localConfigStorage = (configName?: string): KeyValueStorage => { return } try { - writeConfigFile(configName, JSON.stringify(storageData, null, 2)) + // Resolved here, not inside writeFileAtomic: this is the config file's own opt-in + // choice to write through a symlinked configName (an externally-managed "current + // config" convention some deployments use, including a chain of dangling symlinks + // up to MAX_SYMLINK_HOPS deep), not a default writeFileAtomic extends to every + // caller - the cache file (writeCacheFile) deliberately skips this resolution, see + // writeFileAtomic's own comment for why. + const resolvedPath = resolveWriteTargetPath(configName) + writeFileAtomic(resolvedPath, JSON.stringify(storageData, null, 2)) } catch (e) { throw new KeeperStorageError(`Unable to save local config ${configName}: ${describeCause(e)}`, errorCode(e)) } @@ -342,37 +368,117 @@ export const localConfigStorage = (configName?: string): KeyValueStorage => { } } -export const cachingPostFunction = async (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload): Promise => { +const writeCacheFile = async (cachePath: string, cacheKey: Uint8Array, plaintext: Uint8Array): Promise => { + const dir = path.dirname(cachePath) + // A relative cachePath with no directory component resolves dir to '.', the process's + // current working directory - not a directory this function created or owns. Hardening a + // directory the caller never named is a surprising side effect, so directory-level + // hardening only applies when cachePath actually names a directory component. The default + // path is always absolute, so the security-relevant case is unaffected. + if (dir !== '.') { + fs.mkdirSync(dir, {recursive: true, mode: 0o700}) + // O_DIRECTORY|O_NOFOLLOW makes "is this a real directory, not a symlink" and the open + // the same syscall, so there's no gap between a check and a separate mkdirSync/chmodSync + // for a symlink to be swapped into. fchmodSync operates on the fd this open returned, + // pinning the exact inode instead of re-resolving the path a second time. + const dfd = fs.openSync(dir, fs.constants.O_DIRECTORY | fs.constants.O_NOFOLLOW) + try { + fs.fchmodSync(dfd, 0o700) + } finally { + fs.closeSync(dfd) + } + } + const blob = await encodeCacheBlob(plaintext, cacheKey) + writeFileAtomic(cachePath, blob) +} + +const readCacheFile = async (cachePath: string, cacheKey: Uint8Array, maxCacheAgeMs: number): Promise => { + let raw: Buffer try { - const response = await platform.post(url, payload.payload, { - PublicKeyId: transmissionKey.publicKeyId.toString(), - TransmissionKey: platform.bytesToBase64(transmissionKey.encryptedKey), - Authorization: `Signature ${platform.bytesToBase64(payload.signature)}` - }) - if (response.statusCode == 200) { - // Create cache file with secure permissions (0600) - const cacheFd = fs.openSync('cache.dat', 'w', 0o600) - try { - fs.writeSync(cacheFd, Buffer.concat([transmissionKey.key, response.data])) - } finally { - fs.closeSync(cacheFd) - } + const dir = path.dirname(cachePath) + if (dir !== '.') { + // open-then-close is enough here (nothing needs to persist past the check), but it's + // the same O_DIRECTORY|O_NOFOLLOW atomic check-and-open writeCacheFile uses, so a + // symlinked cache directory is rejected on the read path too. + const dfd = fs.openSync(dir, fs.constants.O_DIRECTORY | fs.constants.O_NOFOLLOW) + fs.closeSync(dfd) } - return response - } catch (e) { - let cachedData + // O_NOFOLLOW folds the "not a symlink" check into the open itself, closing the gap a + // separate lstat-then-readFileSync would leave open. + const fd = fs.openSync(cachePath, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW) try { - cachedData = fs.readFileSync('cache.dat') - } catch { + raw = fs.readFileSync(fd) + } finally { + fs.closeSync(fd) } - if (!cachedData) { - throw new Error('Cached value does not exist') + } catch (e: Error | any) { + if (e.code === 'ENOENT') { + throw new KeeperError('Cached value does not exist') } - transmissionKey.key = cachedData.slice(0, 32) - return { - statusCode: 200, - data: cachedData.slice(32), - headers: [] + throw new KeeperError(`Unable to read cache file ${cachePath}: ${e.message}`) + } + try { + return await decodeCacheBlob(raw, cacheKey, maxCacheAgeMs) + } catch (e: Error | any) { + throw new KeeperError(`Cache file ${cachePath} is invalid: ${e.message}`) + } +} + +// Same closure shape and cache codec (../cache) as browser/localConfigStorage.ts's +// createCachingFunction; only the storage medium differs (a file here, IndexedDB there). +// Replaces the old standalone cachingPostFunction, which kept the AES key in plaintext beside +// the ciphertext it protected, in a fixed CWD-relative file, with no integrity check on restore. +// Fixing all three requires access to the config (to derive a cache key that isn't the +// transmission key itself) and a chosen cache location, so the factory shape - not the old +// zero-argument function - is what the fix needs. +// An options object, not positional args: browser's createCachingFunction takes +// (storage, maxCacheAgeMs?) - same position, different meaning than Node's second positional arg +// would otherwise be. A number intended for maxCacheAgeMs silently landing on cachePath (or vice +// versa) fails at runtime with a confusing path error; naming both fields turns that into a +// compile-time type error instead. +export const createCachingFunction = ( + storage: KeyValueStorage, + { + // Computed here, not at module scope: os.homedir() throws in an environment with no $HOME + // and no matching /etc/passwd entry for the current uid (some containers), and a default + // parameter only evaluates when the caller omits the field - so that failure now only + // reaches a caller who actually relies on the default, at call time, not every consumer + // who merely imports this module. + cachePath = path.join(os.homedir(), '.keeper', 'ksm-cache.dat'), + maxCacheAgeMs = DEFAULT_MAX_CACHE_AGE_MS + }: {cachePath?: string, maxCacheAgeMs?: number} = {} +): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload, allowUnverifiedCertificate?: boolean) => Promise => + async (url, transmissionKey, payload, allowUnverifiedCertificate) => { + let response: KeeperHttpResponse + try { + response = await platform.post(url, payload.payload, { + PublicKeyId: transmissionKey.publicKeyId.toString(), + TransmissionKey: platform.bytesToBase64(transmissionKey.encryptedKey), + Authorization: `Signature ${platform.bytesToBase64(payload.signature)}` + }, allowUnverifiedCertificate) + } catch (e) { + const appKey = await storage.getBytes(KEY_APP_KEY) + if (!appKey || !isRawKeyBytes(appKey)) { + throw new KeeperError('Cached value does not exist') + } + const cachedData = await readCacheFile(cachePath, await deriveCacheKey(appKey), maxCacheAgeMs) + console.error(`Network request failed (${describeCause(e)}); serving cached response, which may be stale`) + transmissionKey.key = cachedData.slice(0, 32) + return { + statusCode: 200, + data: cachedData.slice(32), + headers: [] + } } + if (response.statusCode == 200) { + try { + const appKey = await storage.getBytes(KEY_APP_KEY) + if (appKey) { + await writeCacheFile(cachePath, await deriveCacheKey(appKey), Buffer.concat([transmissionKey.key, response.data])) + } + } catch (e: Error | any) { + console.error(`Failed to update cached response: ${e.message}`) + } + } + return response } -} \ No newline at end of file diff --git a/sdk/javascript/packages/core/test/browserLocalConfigStorage.test.ts b/sdk/javascript/packages/core/test/browserLocalConfigStorage.test.ts new file mode 100644 index 000000000..7b9d01632 --- /dev/null +++ b/sdk/javascript/packages/core/test/browserLocalConfigStorage.test.ts @@ -0,0 +1,183 @@ +import {connectPlatform, inMemoryStorage, platform, KeyValueStorage, TransmissionKey, EncryptedPayload} from '../src/platform' +import {browserPlatform} from '../src/browser/browserPlatform' +import {createCachingFunction} from '../src/browser/localConfigStorage' +import {KeeperError} from '../src/errors' + +connectPlatform(browserPlatform) + +const enc = new TextEncoder() + +const makeStorageWithAppKey = async (): Promise => { + const storage = inMemoryStorage({}) + await storage.saveBytes('appKey', new Uint8Array(32).fill(7)) + return storage +} + +const fakeTransmissionKey = (): TransmissionKey => ({ + publicKeyId: 7, + key: platform.getRandomBytes(32), + encryptedKey: new Uint8Array(), +}) + +const fakePayload: EncryptedPayload = { payload: new Uint8Array(), signature: new Uint8Array() } +const networkFailure = () => Object.assign(new Error('connect ECONNREFUSED'), { code: 'ECONNREFUSED' }) +const originalPost = platform.post + +afterEach(() => { + platform.post = originalPost +}) + +describe('browser createCachingFunction (KSM-1265)', () => { + test('round-trip: caches a successful response, then serves it when the network fails', async () => { + const storage = await makeStorageWithAppKey() + const responseData = enc.encode('{"ok":true}') + const tk = fakeTransmissionKey() + const caching = createCachingFunction(storage) + + platform.post = async () => ({ statusCode: 200, data: responseData, headers: [] }) + const first = await caching('https://example.com', tk, fakePayload) + expect(first.statusCode).toBe(200) + expect(Buffer.from(first.data)).toEqual(Buffer.from(responseData)) + + platform.post = async () => { throw networkFailure() } + const tk2 = fakeTransmissionKey() + const second = await caching('https://example.com', tk2, fakePayload) + expect(second.statusCode).toBe(200) + expect(Buffer.from(second.data)).toEqual(Buffer.from(responseData)) + expect(Buffer.from(tk2.key)).toEqual(Buffer.from(tk.key)) + }) + + test('rejects a tampered cached blob instead of returning a synthetic 200', async () => { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + const raw = await storage.getBytes('cache') + const tampered = new Uint8Array(raw!) + tampered[tampered.length - 1] ^= 0xff + await storage.saveBytes('cache', tampered) + + platform.post = async () => { throw networkFailure() } + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)).rejects.toThrow() + }) + + test('rejects a cached blob older than maxCacheAgeMs', async () => { + jest.useFakeTimers() + try { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, 1000) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + jest.advanceTimersByTime(5000) + + platform.post = async () => { throw networkFailure() } + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)).rejects.toThrow() + } finally { + jest.useRealTimers() + } + }) + + test('treats an old-format (pre-fix, plaintext) cached value as a cache miss instead of misparsing it', async () => { + const storage = await makeStorageWithAppKey() + await storage.saveBytes('cache', new Uint8Array([...new Uint8Array(32).fill(1), ...enc.encode('legacy-plaintext-response')])) + + platform.post = async () => { throw networkFailure() } + const caching = createCachingFunction(storage) + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)).rejects.toThrow() + }) + + test('a cache write failure is swallowed so the caller still gets the successful response', async () => { + const storage = await makeStorageWithAppKey() + const originalSaveBytes = storage.saveBytes + storage.saveBytes = async () => { throw new Error('storage quota exceeded') } + const caching = createCachingFunction(storage) + + const responseData = enc.encode('{"ok":true}') + platform.post = async () => ({ statusCode: 200, data: responseData, headers: [] }) + const result = await caching('https://example.com', fakeTransmissionKey(), fakePayload) + expect(result.statusCode).toBe(200) + expect(Buffer.from(result.data)).toEqual(Buffer.from(responseData)) + + storage.saveBytes = originalSaveBytes + }) + + test('no appKey in storage: a successful response is not cached', async () => { + const storage = inMemoryStorage({}) + const caching = createCachingFunction(storage) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + expect(await storage.getBytes('cache')).toBeUndefined() + }) + + test('no appKey in storage: the fallback path throws instead of serving a nonexistent cache', async () => { + const storage = inMemoryStorage({}) + const caching = createCachingFunction(storage) + + platform.post = async () => { throw networkFailure() } + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)) + .rejects.toBeInstanceOf(KeeperError) + }) + + test('storage.getBytes throwing on the success path is not misrouted into serving stale cached data', async () => { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage) + + const staleData = enc.encode('{"stale":true}') + platform.post = async () => ({ statusCode: 200, data: staleData, headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + const originalGetBytes = storage.getBytes + let failNextAppKeyLookup = true + storage.getBytes = async (key: string) => { + if (key === 'appKey' && failNextAppKeyLookup) { + failNextAppKeyLookup = false + throw new Error('storage backend unavailable') + } + return originalGetBytes(key) + } + try { + const freshData = enc.encode('{"fresh":true}') + platform.post = async () => ({ statusCode: 200, data: freshData, headers: [] }) + const result = await caching('https://example.com', fakeTransmissionKey(), fakePayload) + expect(result.statusCode).toBe(200) + expect(Buffer.from(result.data)).toEqual(Buffer.from(freshData)) + } finally { + storage.getBytes = originalGetBytes + } + }) + + test('a CryptoKey appKey (useObjects: true) degrades caching to a no-op instead of throwing', async () => { + const storage = inMemoryStorage({}) + const cryptoKey = await crypto.subtle.generateKey({ name: 'AES-GCM', length: 256 }, false, ['encrypt', 'decrypt', 'unwrapKey']) + // A non-empty 'cache' entry too, so the fallback path actually reaches deriveCacheKey(appKey) + // instead of short-circuiting earlier on a missing cache entry - which would pass on both + // fixed and unfixed code for the wrong reason. + storage.getBytes = async (key: string) => { + if (key === 'appKey') return cryptoKey as unknown as Uint8Array + if (key === 'cache') return new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]) + return undefined + } + const saveBytesSpy = jest.fn() + storage.saveBytes = saveBytesSpy + const caching = createCachingFunction(storage) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{"ok":true}'), headers: [] }) + const first = await caching('https://example.com', fakeTransmissionKey(), fakePayload) + expect(first.statusCode).toBe(200) + expect(saveBytesSpy).not.toHaveBeenCalled() + + platform.post = async () => { throw networkFailure() } + // Exact message, not just KeeperError: pre-fix code also throws a KeeperError here, but + // a confusing one ("Cached value is invalid: Failed to execute 'importKey'...") leaking + // the underlying crypto TypeError instead of the clean "no cache available" message this + // fix produces. + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)) + .rejects.toThrow('Cached value does not exist') + }) +}) diff --git a/sdk/javascript/packages/core/test/cache.test.ts b/sdk/javascript/packages/core/test/cache.test.ts new file mode 100644 index 000000000..7f58b4bf7 --- /dev/null +++ b/sdk/javascript/packages/core/test/cache.test.ts @@ -0,0 +1,5 @@ +import {DEFAULT_MAX_CACHE_AGE_MS} from '../src/cache' + +test('DEFAULT_MAX_CACHE_AGE_MS is 24 hours', () => { + expect(DEFAULT_MAX_CACHE_AGE_MS).toBe(24 * 60 * 60 * 1000) +}) diff --git a/sdk/javascript/packages/core/test/localConfigStorage.homedir.test.ts b/sdk/javascript/packages/core/test/localConfigStorage.homedir.test.ts new file mode 100644 index 000000000..46cc57bad --- /dev/null +++ b/sdk/javascript/packages/core/test/localConfigStorage.homedir.test.ts @@ -0,0 +1,23 @@ +// A dedicated file so this mock doesn't leak into localConfigStorage.test.ts's os.tmpdir()-based +// beforeEach. os.homedir() throws in an environment with no $HOME and no matching /etc/passwd +// entry for the current uid (some containers); this proves that failure is deferred to the call +// that actually relies on the default cache path, not raised merely by importing the module. +jest.mock('os', () => ({ + homedir: () => { throw new Error('no matching entry in the password file was found for the current uid') } +})) + +import {createCachingFunction, inMemoryStorage} from '../' + +test('importing the module does not crash when os.homedir() is unusable', () => { + expect(() => require('../')).not.toThrow() +}) + +test('an explicit cachePath does not need os.homedir()', () => { + const storage = inMemoryStorage({}) + expect(() => createCachingFunction(storage, {cachePath: '/explicit/path/cache.dat'})).not.toThrow() +}) + +test('relying on the default cachePath surfaces the os.homedir() failure at call time', () => { + const storage = inMemoryStorage({}) + expect(() => createCachingFunction(storage)).toThrow(/password file/) +}) diff --git a/sdk/javascript/packages/core/test/localConfigStorage.test.ts b/sdk/javascript/packages/core/test/localConfigStorage.test.ts index e53e46704..8f20ecae4 100644 --- a/sdk/javascript/packages/core/test/localConfigStorage.test.ts +++ b/sdk/javascript/packages/core/test/localConfigStorage.test.ts @@ -1,17 +1,48 @@ -import {localConfigStorage, KeeperError, KeeperStorageError} from '../' +import { + createCachingFunction, + inMemoryStorage, + localConfigStorage, + platform, + KeeperError, + KeeperStorageError, + KeyValueStorage, + TransmissionKey, + EncryptedPayload, +} from '../' import * as fs from 'fs' import * as os from 'os' import * as path from 'path' import * as childProcess from 'child_process' +const enc = new TextEncoder() + +const makeStorageWithAppKey = async (): Promise => { + const storage = inMemoryStorage({}) + await storage.saveBytes('appKey', new Uint8Array(32).fill(7)) + return storage +} + +const fakeTransmissionKey = (): TransmissionKey => ({ + publicKeyId: 7, + key: platform.getRandomBytes(32), + encryptedKey: new Uint8Array(), +}) + +const fakePayload: EncryptedPayload = { payload: new Uint8Array(), signature: new Uint8Array() } +const networkFailure = () => Object.assign(new Error('connect ECONNREFUSED'), { code: 'ECONNREFUSED' }) + let tmpDir: string +let cachePath: string +const originalPost = platform.post beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ksm-cache-test-')) + cachePath = path.join(tmpDir, 'cache.dat') }) afterEach(() => { + platform.post = originalPost fs.rmSync(tmpDir, { recursive: true, force: true }) }) @@ -256,7 +287,7 @@ describe('localConfigStorage readStorage error handling (KSM-1266)', () => { const kvs = localConfigStorage(symlinkPath) // Simulates an attacker swapping the symlink's target immediately after - // writeConfigFile's one resolution call - the narrowest version of the residual + // writeFileAtomic's one resolution call - the narrowest version of the residual // window write-file-atomic itself also leaves open. Only the first realpathSync call // is intercepted (mockRestore right after), so this proves the resolved path is // cached and reused for the eventual rename, not looked up again right before it - @@ -386,7 +417,7 @@ describe('localConfigStorage readStorage error handling (KSM-1266)', () => { test('a file that merely looks tmp-like but does not match the exact naming shape is not swept', () => { const configPath = path.join(tmpDir, 'config.json') fs.writeFileSync(configPath, JSON.stringify({ original: 'data' })) - // Missing the segment writeConfigFile always adds - close enough to be a + // Missing the segment writeFileAtomic always adds - close enough to be a // plausible false match for a looser regex, not close enough to be one of ours. const lookalikePath = `${configPath}.12345.tmp` fs.writeFileSync(lookalikePath, 'not one of ours') @@ -427,9 +458,9 @@ describe('localConfigStorage readStorage error handling (KSM-1266)', () => { }) // The other orphan tests hand-write a fixture filename matching cleanupOrphanedTempFiles' - // regex - they'd stay green even if writeConfigFile's real naming and that regex silently + // regex - they'd stay green even if writeFileAtomic's real naming and that regex silently // drifted apart from each other, as long as each still matched its own fixture. This test - // closes that gap: it kills a real child process actually running writeConfigFile (via the + // closes that gap: it kills a real child process actually running writeFileAtomic (via the // built dist bundle, the same one the package ships), confirming the file it actually // leaves behind is one cleanupOrphanedTempFiles actually recognizes. test('a real SIGKILL between opening the temp file and the rename leaves an orphan the next read cleans up', async () => { @@ -544,4 +575,292 @@ describe('localConfigStorage readStorage error handling (KSM-1266)', () => { expect(await kvs.getString('b')).toBe('2') expect(JSON.parse(fs.readFileSync(configPath, 'utf8'))).toEqual({ b: '2' }) }) + + test('reads through a symlinked config path, matching the Kubernetes Secret/ConfigMap volume-mount layout', async () => { + const configPath = path.join(tmpDir, 'config.json') + const target = path.join(tmpDir, 'target-config.json') + fs.writeFileSync(target, JSON.stringify({foo: 'bar'})) + fs.symlinkSync(target, configPath) + const kvs = localConfigStorage(configPath) + expect(await kvs.getString('foo')).toBe('bar') + }) +}) + +describe('createCachingFunction (KSM-1265)', () => { + test('round-trip: caches a successful response, then serves it when the network fails', async () => { + const storage = await makeStorageWithAppKey() + const responseData = enc.encode('{"ok":true}') + const tk = fakeTransmissionKey() + const caching = createCachingFunction(storage, {cachePath}) + + platform.post = async () => ({ statusCode: 200, data: responseData, headers: [] }) + const first = await caching('https://example.com', tk, fakePayload) + expect(first.statusCode).toBe(200) + expect(Buffer.from(first.data)).toEqual(Buffer.from(responseData)) + + platform.post = async () => { throw networkFailure() } + const tk2 = fakeTransmissionKey() + const second = await caching('https://example.com', tk2, fakePayload) + expect(second.statusCode).toBe(200) + expect(Buffer.from(second.data)).toEqual(Buffer.from(responseData)) + expect(Buffer.from(tk2.key)).toEqual(Buffer.from(tk.key)) + }) + + test('rejects a tampered cache file instead of returning a synthetic 200', async () => { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, {cachePath}) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + const raw = fs.readFileSync(cachePath) + raw[raw.length - 1] ^= 0xff + fs.writeFileSync(cachePath, raw) + + platform.post = async () => { throw networkFailure() } + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)) + .rejects.toBeInstanceOf(KeeperError) + }) + + test('rejects a cache file older than maxCacheAgeMs', async () => { + jest.useFakeTimers() + try { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, {cachePath, maxCacheAgeMs: 1000}) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + jest.advanceTimersByTime(5000) + + platform.post = async () => { throw networkFailure() } + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)) + .rejects.toBeInstanceOf(KeeperError) + } finally { + jest.useRealTimers() + } + }) + + test('a forged freshness timestamp is rejected (the timestamp is now inside the AEAD boundary)', async () => { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, {cachePath}) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + const raw = fs.readFileSync(cachePath) + // Byte 1 falls inside the GCM ciphertext now (the format-version byte at 0 is the only + // cleartext byte left). Flipping it used to land on the cleartext timestamp header and + // silently pin a stale cache as fresh; now it corrupts the ciphertext and fails the tag check. + raw[1] ^= 0xff + fs.writeFileSync(cachePath, raw) + + platform.post = async () => { throw networkFailure() } + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)) + .rejects.toBeInstanceOf(KeeperError) + }) + + test('rejects an old-format cache file instead of misparsing it', async () => { + const storage = await makeStorageWithAppKey() + fs.mkdirSync(path.dirname(cachePath), { recursive: true }) + fs.writeFileSync(cachePath, Buffer.concat([Buffer.alloc(32, 1), Buffer.from('legacy-plaintext-response')])) + + platform.post = async () => { throw networkFailure() } + const caching = createCachingFunction(storage, {cachePath}) + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)) + .rejects.toBeInstanceOf(KeeperError) + }) + + test('a write failure after a successful response is swallowed, so the caller still gets the successful response', async () => { + const storage = await makeStorageWithAppKey() + const blockerFile = path.join(tmpDir, 'blocker') + fs.writeFileSync(blockerFile, '') + const badCachePath = path.join(blockerFile, 'cache.dat') + const caching = createCachingFunction(storage, {cachePath: badCachePath}) + + const responseData = enc.encode('{"ok":true}') + platform.post = async () => ({ statusCode: 200, data: responseData, headers: [] }) + const result = await caching('https://example.com', fakeTransmissionKey(), fakePayload) + expect(result.statusCode).toBe(200) + expect(Buffer.from(result.data)).toEqual(Buffer.from(responseData)) + }) + + test('no appKey in storage: a successful response is not cached', async () => { + const storage = inMemoryStorage({}) + const caching = createCachingFunction(storage, {cachePath}) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + expect(fs.existsSync(cachePath)).toBe(false) + }) + + test('no appKey in storage: the fallback path throws instead of serving a nonexistent cache', async () => { + const storage = inMemoryStorage({}) + const caching = createCachingFunction(storage, {cachePath}) + + platform.post = async () => { throw networkFailure() } + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)) + .rejects.toBeInstanceOf(KeeperError) + }) + + test('re-asserts 0700 on the cache directory even if it started more permissive', async () => { + const storage = await makeStorageWithAppKey() + const cacheDir = path.join(tmpDir, 'loose-dir') + fs.mkdirSync(cacheDir, { mode: 0o755 }) + const loosePath = path.join(cacheDir, 'cache.dat') + const caching = createCachingFunction(storage, {cachePath: loosePath}) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + expect(fs.statSync(cacheDir).mode & 0o777).toBe(0o700) + }) + + test('a write replaces the cache file via rename (new inode), not an in-place truncate', async () => { + const storage = await makeStorageWithAppKey() + fs.mkdirSync(path.dirname(cachePath), { recursive: true }) + fs.writeFileSync(cachePath, 'stale-loose-file') + fs.chmodSync(cachePath, 0o644) + const originalInode = fs.statSync(cachePath).ino + const caching = createCachingFunction(storage, {cachePath}) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + // An in-place truncate-then-chmod would end up at 0600 too - the inode staying the same + // is what actually distinguishes a real rename from that, so mode alone isn't enough. + const stat = fs.statSync(cachePath) + expect(stat.ino).not.toBe(originalInode) + expect(stat.mode & 0o777).toBe(0o600) + }) + + test('a symlink at the cache path is replaced by the write, never followed; the target stays untouched and the fallback reads the healed cache', async () => { + const storage = await makeStorageWithAppKey() + const target = path.join(tmpDir, 'target.dat') + fs.writeFileSync(target, 'do-not-touch') + fs.symlinkSync(target, cachePath) + const caching = createCachingFunction(storage, {cachePath}) + + const responseData = enc.encode('{"ok":true}') + platform.post = async () => ({ statusCode: 200, data: responseData, headers: [] }) + const tk = fakeTransmissionKey() + await caching('https://example.com', tk, fakePayload) + // renameSync replaces the directory entry rather than following it, so the symlink's + // target is never written through - the arbitrary-file-overwrite this test guards + // against stays closed - but the symlink itself is gone, replaced by a real cache file. + expect(fs.readFileSync(target, 'utf8')).toBe('do-not-touch') + expect(fs.lstatSync(cachePath).isSymbolicLink()).toBe(false) + + platform.post = async () => { throw networkFailure() } + const tk2 = fakeTransmissionKey() + const result = await caching('https://example.com', tk2, fakePayload) + expect(result.statusCode).toBe(200) + expect(Buffer.from(result.data)).toEqual(Buffer.from(responseData)) + expect(fs.readFileSync(target, 'utf8')).toBe('do-not-touch') + }) + + test('a relative cachePath with no directory component does not touch the current working directory', async () => { + const storage = await makeStorageWithAppKey() + const originalCwd = process.cwd() + process.chdir(tmpDir) + fs.chmodSync(tmpDir, 0o755) + try { + const caching = createCachingFunction(storage, {cachePath: 'cache.dat'}) + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + expect(fs.statSync(tmpDir).mode & 0o777).toBe(0o755) + } finally { + process.chdir(originalCwd) + } + }) + + test('refuses to write when the cache directory itself is a symlink', async () => { + const storage = await makeStorageWithAppKey() + const realDir = path.join(tmpDir, 'real-dir') + fs.mkdirSync(realDir) + const symlinkedDir = path.join(tmpDir, 'symlinked-dir') + fs.symlinkSync(realDir, symlinkedDir) + const pathThroughSymlink = path.join(symlinkedDir, 'cache.dat') + const caching = createCachingFunction(storage, {cachePath: pathThroughSymlink}) + + const responseData = enc.encode('{"ok":true}') + platform.post = async () => ({ statusCode: 200, data: responseData, headers: [] }) + const result = await caching('https://example.com', fakeTransmissionKey(), fakePayload) + // The write is swallowed, same convention as any other write failure, but nothing lands + // under the real directory the symlink points to. + expect(result.statusCode).toBe(200) + expect(Buffer.from(result.data)).toEqual(Buffer.from(responseData)) + expect(fs.readdirSync(realDir)).toEqual([]) + }) + + test('storage.getBytes throwing after a successful response does not propagate; the fresh response still wins', async () => { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, {cachePath}) + const originalGetBytes = storage.getBytes + storage.getBytes = async () => { throw new Error('storage backend unavailable') } + try { + const responseData = enc.encode('{"ok":true}') + platform.post = async () => ({ statusCode: 200, data: responseData, headers: [] }) + const result = await caching('https://example.com', fakeTransmissionKey(), fakePayload) + expect(result.statusCode).toBe(200) + expect(Buffer.from(result.data)).toEqual(Buffer.from(responseData)) + } finally { + storage.getBytes = originalGetBytes + } + }) + + test('a successful write leaves no leftover temp file behind', async () => { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, {cachePath}) + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + expect(fs.readdirSync(tmpDir)).toEqual(['cache.dat']) + }) + + test('a write that fails before the rename leaves a pre-existing valid cache file intact', async () => { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, {cachePath}) + + const responseData = enc.encode('{"first":true}') + platform.post = async () => ({ statusCode: 200, data: responseData, headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + const before = fs.readFileSync(cachePath) + + // Forces the temp-file create to fail (simulating disk-full, a permission race, etc.) + // deterministically, rather than relying on OS-level permission bits, which some + // filesystems/ACL setups don't enforce the same way for every test runner. + const originalOpenSync = fs.openSync + const openSyncSpy = jest.spyOn(fs, 'openSync').mockImplementation((p: any, flags: any, mode?: any) => { + if (typeof p === 'string' && p.endsWith('.tmp')) { + throw Object.assign(new Error('ENOSPC: no space left on device'), { code: 'ENOSPC' }) + } + return originalOpenSync(p, flags, mode) + }) + try { + platform.post = async () => ({ statusCode: 200, data: enc.encode('{"second":true}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + } finally { + openSyncSpy.mockRestore() + } + + expect(fs.readFileSync(cachePath)).toEqual(before) + }) + + test('uses ~/.keeper/ksm-cache.dat and creates + hardens the directory on first write', async () => { + const storage = await makeStorageWithAppKey() + const originalHomedir = os.homedir + ;(os as any).homedir = () => tmpDir + try { + const caching = createCachingFunction(storage) + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + const expectedPath = path.join(tmpDir, '.keeper', 'ksm-cache.dat') + expect(fs.existsSync(expectedPath)).toBe(true) + expect(fs.statSync(path.dirname(expectedPath)).mode & 0o777).toBe(0o700) + } finally { + (os as any).homedir = originalHomedir + } + }) }) From 20078caaebd85d445533023e1f84e527bac293b1 Mon Sep 17 00:00:00 2001 From: Stas Schaller Date: Fri, 4 Sep 2026 15:28:59 -0400 Subject: [PATCH 2/6] fix(javascript): close out the caching review's round-4 findings (KSM-1265) --- sdk/javascript/packages/core/CHANGELOG.md | 2 +- .../core/src/browser/localConfigStorage.ts | 16 ++- sdk/javascript/packages/core/src/cache.ts | 12 +- .../core/src/node/localConfigStorage.ts | 106 +++++++++++--- .../test/browserLocalConfigStorage.test.ts | 6 + .../core/test/localConfigStorage.test.ts | 135 +++++++++++++++++- 6 files changed, 247 insertions(+), 30 deletions(-) diff --git a/sdk/javascript/packages/core/CHANGELOG.md b/sdk/javascript/packages/core/CHANGELOG.md index 3a2b18516..8e8172a8e 100644 --- a/sdk/javascript/packages/core/CHANGELOG.md +++ b/sdk/javascript/packages/core/CHANGELOG.md @@ -22,7 +22,7 @@ // with a custom cache path or freshness window queryFunction: createCachingFunction(storage, {cachePath, maxCacheAgeMs}) ``` - The cache is now encrypted with a key derived from the app key already held in the config (so reading the cache requires the config, not just the cache file), authenticated so a tampered or corrupted file is rejected instead of silently trusted, bounded by a configurable freshness window (default 24h), and located at `~/.keeper/ksm-cache.dat` by default instead of the working directory. Usage was limited to the opt-in caching example, which has been updated to use the new function. If you called `cachingPostFunction` directly, delete the old cache file in your working directory after upgrading; it is not removed automatically. `cachePath` and `maxCacheAgeMs` are now named fields on an options object instead of positional arguments, since Node's and the browser's second positional argument meant different things; the browser signature (`createCachingFunction(storage, maxCacheAgeMs?)`) is unchanged and still non-breaking there, since the new `maxCacheAgeMs` parameter is optional and an old-format cached value is simply treated as a cache miss. The cache file and directory reject a symlink on write (replaced outright, never written through - there is no legitimate externally-managed symlink convention for a path the SDK itself names) and on read; the config file's own symlink handling is unchanged and intentionally different, see the KSM-1266 entry above. Both the config file and the cache file are now written atomically (to a temporary file, then renamed into place, via one shared primitive), so a write that fails partway through can no longer leave a corrupted or truncated file behind. Known limitation: the very first (bind) call's response is not cached, since caching requires an app key that the bind call itself establishes; every call after that caches normally. In the browser, when the app key is held as a non-extractable `CryptoKey` (`useObjects: true`), caching is a no-op rather than an error, the same graceful degradation already used for a network failure with no prior cache. A network failure served from cache now logs a warning, since the caller is getting a response that may be stale. This package now also declares an `exports` field so bundlers and modern TypeScript resolve the correct platform-specific type declarations for the browser bundle; a consumer still on TypeScript's legacy `moduleResolution: "node"` continues to see the Node type declarations regardless, unchanged from before. + The cache is now encrypted with a key derived from the app key already held in the config (so reading the cache requires the config, not just the cache file), authenticated so a tampered or corrupted file is rejected instead of silently trusted, bounded by a configurable freshness window (default 24h), and located at `~/.keeper/ksm-cache.dat` by default instead of the working directory. Usage was limited to the opt-in caching example, which has been updated to use the new function. If you called `cachingPostFunction` directly, delete the old cache file in your working directory after upgrading; it is not removed automatically. `cachePath` and `maxCacheAgeMs` are now named fields on an options object instead of positional arguments, since Node's and the browser's second positional argument meant different things; the browser signature (`createCachingFunction(storage, maxCacheAgeMs?)`) is unchanged and still non-breaking there, since the new `maxCacheAgeMs` parameter is optional and an old-format cached value is simply treated as a cache miss. A symlinked cache directory is rejected outright (throws) on both read and write. A symlinked cache file itself is not rejected with an error - the write silently replaces it with a real file instead of following it, and the read still requires the actual content underneath to pass its own integrity check - there is no legitimate externally-managed symlink convention for a path the SDK itself names, unlike the config file's own symlink handling, which is unchanged and intentionally different (see the KSM-1266 entry above). Both the config file and the cache file are now written atomically (to a temporary file, then renamed into place, via one shared primitive), so a write that fails partway through can no longer leave a corrupted or truncated file behind. The cache directory is created at `0700` and re-hardened to `0700` if this call is the one that creates it; a caller-supplied `cachePath` pointing at a directory that already exists (for example, a file directly inside `$HOME`) keeps whatever permissions it already had - the SDK does not narrow permissions on a directory it doesn't own. Known limitation: the very first (bind) call's response is not cached, since caching requires an app key that the bind call itself establishes; every call after that caches normally. In the browser, when the app key is held as a non-extractable `CryptoKey` (`useObjects: true`), caching is a no-op rather than an error, the same graceful degradation already used for a network failure with no prior cache. A network failure served from cache now logs a warning, since the caller is getting a response that may be stale. This package now also declares an `exports` field so bundlers and modern TypeScript resolve the correct platform-specific type declarations for the browser bundle; a consumer still on TypeScript's legacy `moduleResolution: "node"` continues to see the Node type declarations regardless, unchanged from before. - Maintenance: Updated `minimatch`, `@babel/core`, and `handlebars` dev dependencies. ## 17.5.0 diff --git a/sdk/javascript/packages/core/src/browser/localConfigStorage.ts b/sdk/javascript/packages/core/src/browser/localConfigStorage.ts index b261724e4..82b59b76f 100644 --- a/sdk/javascript/packages/core/src/browser/localConfigStorage.ts +++ b/sdk/javascript/packages/core/src/browser/localConfigStorage.ts @@ -1,6 +1,6 @@ import {EncryptedPayload, KeeperHttpResponse, KeyValueStorage, TransmissionKey, platform} from "../platform"; import {KeeperError} from "../errors"; -import {KEY_APP_KEY, deriveCacheKey, encodeCacheBlob, decodeCacheBlob, DEFAULT_MAX_CACHE_AGE_MS, isRawKeyBytes} from "../cache"; +import {KEY_APP_KEY, deriveCacheKey, encodeCacheBlob, decodeCacheBlob, DEFAULT_MAX_CACHE_AGE_MS, isRawKeyBytes, concatBytes} from "../cache"; const CACHE_STORAGE_KEY = 'cache' @@ -248,11 +248,19 @@ export function createCachingFunction(storage: KeyValueStorage, maxCacheAgeMs: n try { const appKey = await storage.getBytes(KEY_APP_KEY) if (appKey && isRawKeyBytes(appKey)) { - const blob = await encodeCacheBlob(new Uint8Array([...transmissionKey.key, ...response.data]), await deriveCacheKey(appKey)) + const blob = await encodeCacheBlob(concatBytes(transmissionKey.key, response.data), await deriveCacheKey(appKey)) await storage.saveBytes(CACHE_STORAGE_KEY, blob) + } else if (appKey) { + // appKey exists but isn't raw bytes - useObjects: true wraps it as a + // non-extractable CryptoKey, so caching is a deliberate no-op here (matches + // the identical guard in the fallback branch above), not a failure. Logged + // once per call, same as the fallback branch's own log a few lines up, so a + // caller who opted into useObjects: true has some signal that caching isn't + // doing anything for them before their first real outage. + console.error('Caching is a no-op with useObjects: true - the app key is not available as raw bytes') } - } catch (e: Error | any) { - console.error(`Failed to update cached response: ${e.message}`) + } catch (e) { + console.error(`Failed to update cached response: ${describeCause(e)}`) } } return response diff --git a/sdk/javascript/packages/core/src/cache.ts b/sdk/javascript/packages/core/src/cache.ts index 754f060df..2a3c7877d 100644 --- a/sdk/javascript/packages/core/src/cache.ts +++ b/sdk/javascript/packages/core/src/cache.ts @@ -9,7 +9,11 @@ export const CACHE_FORMAT_VERSION = 0x02 export const DEFAULT_MAX_CACHE_AGE_MS = 24 * 60 * 60 * 1000 // No Buffer here - this module is shared with the browser bundle, which has no Buffer global. -const concatBytes = (...parts: Uint8Array[]): Uint8Array => { +// Exported so callers building the plaintext to cache (transmission key + response body) get +// the same non-boxing copy this module already uses internally, instead of a slower +// spread-into-array (measured ~440x slower for a 50MB payload, since that boxes every byte +// through an intermediate JS array). +export const concatBytes = (...parts: Uint8Array[]): Uint8Array => { const length = parts.reduce((sum, part) => sum + part.length, 0) const out = new Uint8Array(length) let offset = 0 @@ -60,7 +64,11 @@ export const decodeCacheBlob = async (raw: Uint8Array, cacheKey: Uint8Array, max if (decrypted.length < 8) { throw new Error('cache blob is not in a recognized format') } - if (Date.now() - readTimestamp(decrypted.subarray(0, 8)) > maxCacheAgeMs) { + // Math.abs, not a plain subtraction: if the wall clock has moved backward relative to when + // this entry was encrypted (common on a VM/container before its first NTP sync), a plain + // `Date.now() - written` goes negative and this check never trips, letting an entry written + // during a period of clock skew stay "fresh" indefinitely regardless of maxCacheAgeMs. + if (Math.abs(Date.now() - readTimestamp(decrypted.subarray(0, 8))) > maxCacheAgeMs) { throw new Error(`cached value is stale (age exceeds ${maxCacheAgeMs}ms)`) } return decrypted.subarray(8) diff --git a/sdk/javascript/packages/core/src/node/localConfigStorage.ts b/sdk/javascript/packages/core/src/node/localConfigStorage.ts index 72c83d2dc..9a35a3d7e 100644 --- a/sdk/javascript/packages/core/src/node/localConfigStorage.ts +++ b/sdk/javascript/packages/core/src/node/localConfigStorage.ts @@ -368,7 +368,22 @@ export const localConfigStorage = (configName?: string): KeyValueStorage => { } } +// fs.constants.O_DIRECTORY/O_NOFOLLOW are undefined on Windows (same gap already tracked for +// writeFileAtomic's hard-link branch). There, `x | undefined` coerces to plain `x` and the two +// checks below silently stop verifying anything - not a crash, just a directory/file open with +// no symlink protection at all. Unlike writeFileAtomic's hard-link branch (where O_NOFOLLOW is +// one layer of defense-in-depth on top of an already-safe rename), this is the *only* +// protection the cache directory/file has, so a silent no-op here is a bigger gap. There is no +// good fallback available without a real openat()-style relative-to-fd primitive, which Node's +// public fs API doesn't expose - accepted as a documented, POSIX-only limitation rather than +// building a weaker check-then-open substitute, matching how KSM-1266's own review already +// decided the identical Windows gap for O_NOFOLLOW. +const hasDirectorySymlinkProtection = typeof fs.constants.O_DIRECTORY === 'number' && typeof fs.constants.O_NOFOLLOW === 'number' +const cacheDirOpenFlags = hasDirectorySymlinkProtection ? fs.constants.O_DIRECTORY | fs.constants.O_NOFOLLOW : fs.constants.O_DIRECTORY +const cacheFileReadFlags = hasDirectorySymlinkProtection ? fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW : fs.constants.O_RDONLY + const writeCacheFile = async (cachePath: string, cacheKey: Uint8Array, plaintext: Uint8Array): Promise => { + cleanupOrphanedTempFiles(cachePath) const dir = path.dirname(cachePath) // A relative cachePath with no directory component resolves dir to '.', the process's // current working directory - not a directory this function created or owns. Hardening a @@ -376,14 +391,43 @@ const writeCacheFile = async (cachePath: string, cacheKey: Uint8Array, plaintext // hardening only applies when cachePath actually names a directory component. The default // path is always absolute, so the security-relevant case is unaffected. if (dir !== '.') { + // mkdirSync(recursive: true) follows a symlink in any existing ancestor segment of a + // custom, multi-segment cachePath - only the leaf gets an O_NOFOLLOW check below. A + // pre-planted symlink ancestor (e.g. an attacker who can write to /shared pre-creates + // /shared/ksm as a symlink before this code ever runs, given cachePath under + // /shared/ksm/nested/cache.dat) silently relocates the whole cache, no race required. + // Accepted as a structurally-unclosable limitation without a native openat2()-style + // RESOLVE_NO_SYMLINKS binding (Linux-only even then, and its own author documents that + // blanket rejection breaks legitimate ancestor symlinks - confirmed directly: a first + // attempt at a plain lstat-every-segment check broke on macOS's own /tmp -> /private/tmp + // convention, indistinguishable by inspection from an attacker-planted one). No portable + // fix exists in Node's plain fs API; none of mkdirp/make-dir/write-file-atomic attempt + // one either. Same shape as the Windows O_NOFOLLOW gap already documented in this file. + // Unlike KSM-1263's file-level re-assertion (which runs on every write, since the SDK + // exclusively owns that one file it names), this only re-chmods the directory when this + // call is the one that just created it. A directory that already existed - most + // plausibly a caller-supplied cachePath pointing somewhere the caller manages, e.g. a + // file directly inside $HOME - keeps whatever permissions its actual owner set; the SDK + // doesn't forcibly narrow permissions on a resource it doesn't own. This matches the + // pattern documented for npm's own cache/config directory ("there are times when you do + // not want to change ownership of the default directory... configure a different + // directory altogether") and avoids the exact bug shape filed against another CLI's + // config-dir hardening (unconditional per-run chmod silently overriding a user's own + // chosen mode on a directory they own) - researched this session, not just inferred. + const dirExistedBefore = fs.existsSync(dir) fs.mkdirSync(dir, {recursive: true, mode: 0o700}) // O_DIRECTORY|O_NOFOLLOW makes "is this a real directory, not a symlink" and the open // the same syscall, so there's no gap between a check and a separate mkdirSync/chmodSync - // for a symlink to be swapped into. fchmodSync operates on the fd this open returned, - // pinning the exact inode instead of re-resolving the path a second time. - const dfd = fs.openSync(dir, fs.constants.O_DIRECTORY | fs.constants.O_NOFOLLOW) + // for a symlink to be swapped into (Windows caveat above). fchmodSync operates on the fd + // this open returned, pinning the exact inode instead of re-resolving the path a second + // time - though re-resolution still happens at the later file open below, since Node + // has no relative-to-this-fd open; that residual window is narrow (already-checked + // directory to already-checked file, both immediately adjacent to their use) but real. + const dfd = fs.openSync(dir, cacheDirOpenFlags) try { - fs.fchmodSync(dfd, 0o700) + if (!dirExistedBefore) { + fs.fchmodSync(dfd, 0o700) + } } finally { fs.closeSync(dfd) } @@ -392,35 +436,50 @@ const writeCacheFile = async (cachePath: string, cacheKey: Uint8Array, plaintext writeFileAtomic(cachePath, blob) } +// Bounds how much a corrupted, misconfigured, or maliciously-placed file at the cache path can +// force this to allocate before decodeCacheBlob gets any chance to reject it. Real cache blobs +// are tiny (a version byte, an 8-byte timestamp, and the encrypted response); generous headroom +// over any real response, same shape as MAX_ERROR_BODY_DECODE_BYTES in keeper.ts. +const MAX_CACHE_FILE_BYTES = 10 * 1024 * 1024 + const readCacheFile = async (cachePath: string, cacheKey: Uint8Array, maxCacheAgeMs: number): Promise => { + cleanupOrphanedTempFiles(cachePath) let raw: Buffer try { const dir = path.dirname(cachePath) if (dir !== '.') { // open-then-close is enough here (nothing needs to persist past the check), but it's // the same O_DIRECTORY|O_NOFOLLOW atomic check-and-open writeCacheFile uses, so a - // symlinked cache directory is rejected on the read path too. - const dfd = fs.openSync(dir, fs.constants.O_DIRECTORY | fs.constants.O_NOFOLLOW) + // symlinked cache directory is rejected on the read path too. Same residual + // re-resolution window as writeCacheFile's comment above. + const dfd = fs.openSync(dir, cacheDirOpenFlags) fs.closeSync(dfd) } // O_NOFOLLOW folds the "not a symlink" check into the open itself, closing the gap a - // separate lstat-then-readFileSync would leave open. - const fd = fs.openSync(cachePath, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW) + // separate lstat-then-readFileSync would leave open (Windows caveat above). + const fd = fs.openSync(cachePath, cacheFileReadFlags) try { + const size = fs.fstatSync(fd).size + if (size > MAX_CACHE_FILE_BYTES) { + throw new KeeperError(`Cache file ${cachePath} exceeds the maximum expected size`) + } raw = fs.readFileSync(fd) } finally { fs.closeSync(fd) } - } catch (e: Error | any) { - if (e.code === 'ENOENT') { + } catch (e) { + if (e instanceof KeeperError) { + throw e + } + if (isEnoent(e)) { throw new KeeperError('Cached value does not exist') } - throw new KeeperError(`Unable to read cache file ${cachePath}: ${e.message}`) + throw new KeeperError(`Unable to read cache file ${cachePath}: ${describeCause(e)}`) } try { return await decodeCacheBlob(raw, cacheKey, maxCacheAgeMs) - } catch (e: Error | any) { - throw new KeeperError(`Cache file ${cachePath} is invalid: ${e.message}`) + } catch (e) { + throw new KeeperError(`Cache file ${cachePath} is invalid: ${describeCause(e)}`) } } @@ -457,7 +516,16 @@ export const createCachingFunction = ( Authorization: `Signature ${platform.bytesToBase64(payload.signature)}` }, allowUnverifiedCertificate) } catch (e) { - const appKey = await storage.getBytes(KEY_APP_KEY) + // A storage failure here (plausible during the same outage that took the network + // down, for a KMS-backed KeyValueStorage) is treated the same as "no usable app key + // yet" - both mean the cache can't be read, not a reason to let a different, + // unrelated exception replace the original network error's context. + let appKey: Uint8Array | undefined + try { + appKey = await storage.getBytes(KEY_APP_KEY) + } catch { + appKey = undefined + } if (!appKey || !isRawKeyBytes(appKey)) { throw new KeeperError('Cached value does not exist') } @@ -473,11 +541,15 @@ export const createCachingFunction = ( if (response.statusCode == 200) { try { const appKey = await storage.getBytes(KEY_APP_KEY) - if (appKey) { + // Same isRawKeyBytes guard as the fallback branch above - without it, a + // non-raw-bytes value (e.g. a wrapped CryptoKey under a hypothetical Node + // useObjects mode) reaches deriveCacheKey unchecked and logs a confusing + // internal TypeError instead of just skipping the cache write cleanly. + if (appKey && isRawKeyBytes(appKey)) { await writeCacheFile(cachePath, await deriveCacheKey(appKey), Buffer.concat([transmissionKey.key, response.data])) } - } catch (e: Error | any) { - console.error(`Failed to update cached response: ${e.message}`) + } catch (e) { + console.error(`Failed to update cached response: ${describeCause(e)}`) } } return response diff --git a/sdk/javascript/packages/core/test/browserLocalConfigStorage.test.ts b/sdk/javascript/packages/core/test/browserLocalConfigStorage.test.ts index 7b9d01632..0ed41e8c8 100644 --- a/sdk/javascript/packages/core/test/browserLocalConfigStorage.test.ts +++ b/sdk/javascript/packages/core/test/browserLocalConfigStorage.test.ts @@ -167,10 +167,16 @@ describe('browser createCachingFunction (KSM-1265)', () => { storage.saveBytes = saveBytesSpy const caching = createCachingFunction(storage) + // Confirms the no-op is signaled, not silent: before this fix, a caller who opted into + // useObjects: true had no way to know caching was doing nothing for them until their + // first real outage hit the exact-message assertion below. + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}) platform.post = async () => ({ statusCode: 200, data: enc.encode('{"ok":true}'), headers: [] }) const first = await caching('https://example.com', fakeTransmissionKey(), fakePayload) expect(first.statusCode).toBe(200) expect(saveBytesSpy).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('useObjects: true')) + consoleErrorSpy.mockRestore() platform.post = async () => { throw networkFailure() } // Exact message, not just KeeperError: pre-fix code also throws a KeeperError here, but diff --git a/sdk/javascript/packages/core/test/localConfigStorage.test.ts b/sdk/javascript/packages/core/test/localConfigStorage.test.ts index 8f20ecae4..9d2fc7488 100644 --- a/sdk/javascript/packages/core/test/localConfigStorage.test.ts +++ b/sdk/javascript/packages/core/test/localConfigStorage.test.ts @@ -606,6 +606,23 @@ describe('createCachingFunction (KSM-1265)', () => { expect(Buffer.from(tk2.key)).toEqual(Buffer.from(tk.key)) }) + test('serving a stale-on-network-failure cache logs a warning, the only signal a caller gets that the data may not be fresh', async () => { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, {cachePath}) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}) + try { + platform.post = async () => { throw networkFailure() } + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('Network request failed')) + } finally { + consoleErrorSpy.mockRestore() + } + }) + test('rejects a tampered cache file instead of returning a synthetic 200', async () => { const storage = await makeStorageWithAppKey() const caching = createCachingFunction(storage, {cachePath}) @@ -704,17 +721,123 @@ describe('createCachingFunction (KSM-1265)', () => { .rejects.toBeInstanceOf(KeeperError) }) - test('re-asserts 0700 on the cache directory even if it started more permissive', async () => { + test('appKey present but not raw bytes: skipped cleanly, not logged as a confusing internal error', async () => { + // inMemoryStorage round-trips every saveBytes value through base64 encode/decode, so it + // always hands back a genuine Uint8Array on read regardless of what was stored - it + // can't simulate this. A custom KeyValueStorage could plausibly return a truthy + // non-Uint8Array directly (this SDK's own bundled storages never do). + // + // Either way the cache file ends up unwritten - deriveCacheKey's own internal + // isRawKeyBytes check throws even without this guard, and the outer try/catch already + // swallows that. What the guard on this call site actually controls is whether that + // throw ever happens: with it, the write is skipped before deriveCacheKey runs, no log + // at all; without it, deriveCacheKey's TypeError reaches the outer catch and gets logged + // as "Failed to update cached response: ...", indistinguishable from a real I/O failure. + const storage: KeyValueStorage = { + getString: async () => undefined, + saveString: async () => {}, + getBytes: async () => 'not-actually-bytes' as unknown as Uint8Array, + saveBytes: async () => {}, + delete: async () => {} + } + const caching = createCachingFunction(storage, {cachePath}) + + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}) + try { + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + expect(fs.existsSync(cachePath)).toBe(false) + expect(consoleErrorSpy).not.toHaveBeenCalled() + } finally { + consoleErrorSpy.mockRestore() + } + }) + + test('storage.getBytes throwing in the fallback path throws the documented "does not exist" error, not the raw storage error', async () => { const storage = await makeStorageWithAppKey() - const cacheDir = path.join(tmpDir, 'loose-dir') - fs.mkdirSync(cacheDir, { mode: 0o755 }) - const loosePath = path.join(cacheDir, 'cache.dat') - const caching = createCachingFunction(storage, {cachePath: loosePath}) + const caching = createCachingFunction(storage, {cachePath}) + const originalGetBytes = storage.getBytes + storage.getBytes = async () => { throw new Error('storage backend unavailable') } + try { + platform.post = async () => { throw networkFailure() } + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)) + .rejects.toThrow('Cached value does not exist') + } finally { + storage.getBytes = originalGetBytes + } + }) + test('a cache entry written during backward clock skew is still treated as stale, not fresh forever', async () => { + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, {cachePath, maxCacheAgeMs: 1000}) + + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + const realNow = Date.now + try { + // Simulates the clock moving backward relative to when the entry was written (e.g. + // a VM/container before its first NTP sync) - a plain `Date.now() - written` would + // go negative and never exceed maxCacheAgeMs, letting this entry read as fresh + // indefinitely regardless of the configured age limit. + Date.now = () => realNow() - 10 * 60 * 1000 + platform.post = async () => { throw networkFailure() } + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)) + .rejects.toBeInstanceOf(KeeperError) + } finally { + Date.now = realNow + } + }) + + test('a cache file over the size cap is rejected before any decode attempt', async () => { + fs.mkdirSync(path.dirname(cachePath), { recursive: true }) + fs.writeFileSync(cachePath, Buffer.alloc(11 * 1024 * 1024)) + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, {cachePath}) + + platform.post = async () => { throw networkFailure() } + await expect(caching('https://example.com', fakeTransmissionKey(), fakePayload)) + .rejects.toThrow('exceeds the maximum expected size') + }) + + test('a stale orphaned temp file next to the cache path is removed on the next read or write', async () => { + fs.mkdirSync(path.dirname(cachePath), { recursive: true }) + const orphanPath = `${cachePath}.99999.aabbccddeeff.tmp` + fs.writeFileSync(orphanPath, 'stale-leftover-cache-write') + const old = new Date(Date.now() - 5 * 60_000) + fs.utimesSync(orphanPath, old, old) + + const storage = await makeStorageWithAppKey() + const caching = createCachingFunction(storage, {cachePath}) platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) await caching('https://example.com', fakeTransmissionKey(), fakePayload) - expect(fs.statSync(cacheDir).mode & 0o777).toBe(0o700) + expect(fs.existsSync(orphanPath)).toBe(false) + }) + + test('hardens a cache directory this call creates, but leaves a pre-existing directory\'s permissions alone', async () => { + const storage = await makeStorageWithAppKey() + + // A directory this call creates fresh - the SDK owns it, so it gets locked to 0700. + const freshDir = path.join(tmpDir, 'fresh-dir') + const freshPath = path.join(freshDir, 'cache.dat') + const cachingFresh = createCachingFunction(storage, {cachePath: freshPath}) + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await cachingFresh('https://example.com', fakeTransmissionKey(), fakePayload) + expect(fs.statSync(freshDir).mode & 0o777).toBe(0o700) + + // A directory that already existed before this call - a caller-supplied cachePath can + // point at a directory the caller manages for other things (e.g. a file directly inside + // $HOME); the SDK must not narrow permissions on a directory it doesn't own. Matches + // established practice for this exact caller-owned-vs-SDK-owned distinction (npm's own + // cache-directory guidance; the same "unconditional per-run chmod on an existing + // directory" pattern is a filed, disputed bug in another CLI's own config-dir hardening). + const looseDir = path.join(tmpDir, 'loose-dir') + fs.mkdirSync(looseDir, { mode: 0o755 }) + const loosePath = path.join(looseDir, 'cache.dat') + const cachingLoose = createCachingFunction(storage, {cachePath: loosePath}) + await cachingLoose('https://example.com', fakeTransmissionKey(), fakePayload) + expect(fs.statSync(looseDir).mode & 0o777).toBe(0o755) }) test('a write replaces the cache file via rename (new inode), not an in-place truncate', async () => { From 802371b3cc09e5e34d41020a5ffa4d852786858c Mon Sep 17 00:00:00 2001 From: Stas Schaller Date: Fri, 4 Sep 2026 15:52:21 -0400 Subject: [PATCH 3/6] fix(javascript): keep the cache directory's own default path self-healing (KSM-1265) --- sdk/javascript/packages/core/CHANGELOG.md | 2 +- .../core/src/node/localConfigStorage.ts | 76 ++++++++----------- .../core/test/localConfigStorage.test.ts | 23 ++++++ 3 files changed, 56 insertions(+), 45 deletions(-) diff --git a/sdk/javascript/packages/core/CHANGELOG.md b/sdk/javascript/packages/core/CHANGELOG.md index 8e8172a8e..f74dc32fb 100644 --- a/sdk/javascript/packages/core/CHANGELOG.md +++ b/sdk/javascript/packages/core/CHANGELOG.md @@ -22,7 +22,7 @@ // with a custom cache path or freshness window queryFunction: createCachingFunction(storage, {cachePath, maxCacheAgeMs}) ``` - The cache is now encrypted with a key derived from the app key already held in the config (so reading the cache requires the config, not just the cache file), authenticated so a tampered or corrupted file is rejected instead of silently trusted, bounded by a configurable freshness window (default 24h), and located at `~/.keeper/ksm-cache.dat` by default instead of the working directory. Usage was limited to the opt-in caching example, which has been updated to use the new function. If you called `cachingPostFunction` directly, delete the old cache file in your working directory after upgrading; it is not removed automatically. `cachePath` and `maxCacheAgeMs` are now named fields on an options object instead of positional arguments, since Node's and the browser's second positional argument meant different things; the browser signature (`createCachingFunction(storage, maxCacheAgeMs?)`) is unchanged and still non-breaking there, since the new `maxCacheAgeMs` parameter is optional and an old-format cached value is simply treated as a cache miss. A symlinked cache directory is rejected outright (throws) on both read and write. A symlinked cache file itself is not rejected with an error - the write silently replaces it with a real file instead of following it, and the read still requires the actual content underneath to pass its own integrity check - there is no legitimate externally-managed symlink convention for a path the SDK itself names, unlike the config file's own symlink handling, which is unchanged and intentionally different (see the KSM-1266 entry above). Both the config file and the cache file are now written atomically (to a temporary file, then renamed into place, via one shared primitive), so a write that fails partway through can no longer leave a corrupted or truncated file behind. The cache directory is created at `0700` and re-hardened to `0700` if this call is the one that creates it; a caller-supplied `cachePath` pointing at a directory that already exists (for example, a file directly inside `$HOME`) keeps whatever permissions it already had - the SDK does not narrow permissions on a directory it doesn't own. Known limitation: the very first (bind) call's response is not cached, since caching requires an app key that the bind call itself establishes; every call after that caches normally. In the browser, when the app key is held as a non-extractable `CryptoKey` (`useObjects: true`), caching is a no-op rather than an error, the same graceful degradation already used for a network failure with no prior cache. A network failure served from cache now logs a warning, since the caller is getting a response that may be stale. This package now also declares an `exports` field so bundlers and modern TypeScript resolve the correct platform-specific type declarations for the browser bundle; a consumer still on TypeScript's legacy `moduleResolution: "node"` continues to see the Node type declarations regardless, unchanged from before. + The cache is now encrypted with a key derived from the app key already held in the config (so reading the cache requires the config, not just the cache file), authenticated so a tampered or corrupted file is rejected instead of silently trusted, bounded by a configurable freshness window (default 24h), and located at `~/.keeper/ksm-cache.dat` by default instead of the working directory. Usage was limited to the opt-in caching example, which has been updated to use the new function. If you called `cachingPostFunction` directly, delete the old cache file in your working directory after upgrading; it is not removed automatically. `cachePath` and `maxCacheAgeMs` are now named fields on an options object instead of positional arguments, since Node's and the browser's second positional argument meant different things; the browser signature (`createCachingFunction(storage, maxCacheAgeMs?)`) is unchanged and still non-breaking there, since the new `maxCacheAgeMs` parameter is optional and an old-format cached value is simply treated as a cache miss. A symlinked cache directory is rejected outright (throws) on both read and write. A symlinked cache file itself is not rejected with an error - the write silently replaces it with a real file instead of following it, and the read still requires the actual content underneath to pass its own integrity check - there is no legitimate externally-managed symlink convention for a path the SDK itself names, unlike the config file's own symlink handling, which is unchanged and intentionally different (see the KSM-1266 entry above). Both the config file and the cache file are now written atomically (to a temporary file, then renamed into place, via one shared primitive), so a write that fails partway through can no longer leave a corrupted or truncated file behind. The cache directory is created at `0700`. On the default path (`~/.keeper`), it's re-hardened to `0700` on every write, matching how the cache file itself already self-heals; on a caller-supplied `cachePath` pointing at a directory that already exists (for example, a file directly inside `$HOME`), its permissions are left alone - the SDK does not narrow permissions on a directory it doesn't own. Known limitation: the very first (bind) call's response is not cached, since caching requires an app key that the bind call itself establishes; every call after that caches normally. In the browser, when the app key is held as a non-extractable `CryptoKey` (`useObjects: true`), caching is a no-op rather than an error, the same graceful degradation already used for a network failure with no prior cache. A network failure served from cache now logs a warning, since the caller is getting a response that may be stale. This package now also declares an `exports` field so bundlers and modern TypeScript resolve the correct platform-specific type declarations for the browser bundle; a consumer still on TypeScript's legacy `moduleResolution: "node"` continues to see the Node type declarations regardless, unchanged from before. - Maintenance: Updated `minimatch`, `@babel/core`, and `handlebars` dev dependencies. ## 17.5.0 diff --git a/sdk/javascript/packages/core/src/node/localConfigStorage.ts b/sdk/javascript/packages/core/src/node/localConfigStorage.ts index 9a35a3d7e..70dc0e469 100644 --- a/sdk/javascript/packages/core/src/node/localConfigStorage.ts +++ b/sdk/javascript/packages/core/src/node/localConfigStorage.ts @@ -382,7 +382,7 @@ const hasDirectorySymlinkProtection = typeof fs.constants.O_DIRECTORY === 'numbe const cacheDirOpenFlags = hasDirectorySymlinkProtection ? fs.constants.O_DIRECTORY | fs.constants.O_NOFOLLOW : fs.constants.O_DIRECTORY const cacheFileReadFlags = hasDirectorySymlinkProtection ? fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW : fs.constants.O_RDONLY -const writeCacheFile = async (cachePath: string, cacheKey: Uint8Array, plaintext: Uint8Array): Promise => { +const writeCacheFile = async (cachePath: string, cacheKey: Uint8Array, plaintext: Uint8Array, isDefaultCachePath: boolean): Promise => { cleanupOrphanedTempFiles(cachePath) const dir = path.dirname(cachePath) // A relative cachePath with no directory component resolves dir to '.', the process's @@ -391,41 +391,25 @@ const writeCacheFile = async (cachePath: string, cacheKey: Uint8Array, plaintext // hardening only applies when cachePath actually names a directory component. The default // path is always absolute, so the security-relevant case is unaffected. if (dir !== '.') { - // mkdirSync(recursive: true) follows a symlink in any existing ancestor segment of a - // custom, multi-segment cachePath - only the leaf gets an O_NOFOLLOW check below. A - // pre-planted symlink ancestor (e.g. an attacker who can write to /shared pre-creates - // /shared/ksm as a symlink before this code ever runs, given cachePath under - // /shared/ksm/nested/cache.dat) silently relocates the whole cache, no race required. - // Accepted as a structurally-unclosable limitation without a native openat2()-style - // RESOLVE_NO_SYMLINKS binding (Linux-only even then, and its own author documents that - // blanket rejection breaks legitimate ancestor symlinks - confirmed directly: a first - // attempt at a plain lstat-every-segment check broke on macOS's own /tmp -> /private/tmp - // convention, indistinguishable by inspection from an attacker-planted one). No portable - // fix exists in Node's plain fs API; none of mkdirp/make-dir/write-file-atomic attempt - // one either. Same shape as the Windows O_NOFOLLOW gap already documented in this file. - // Unlike KSM-1263's file-level re-assertion (which runs on every write, since the SDK - // exclusively owns that one file it names), this only re-chmods the directory when this - // call is the one that just created it. A directory that already existed - most - // plausibly a caller-supplied cachePath pointing somewhere the caller manages, e.g. a - // file directly inside $HOME - keeps whatever permissions its actual owner set; the SDK - // doesn't forcibly narrow permissions on a resource it doesn't own. This matches the - // pattern documented for npm's own cache/config directory ("there are times when you do - // not want to change ownership of the default directory... configure a different - // directory altogether") and avoids the exact bug shape filed against another CLI's - // config-dir hardening (unconditional per-run chmod silently overriding a user's own - // chosen mode on a directory they own) - researched this session, not just inferred. + // mkdirSync(recursive: true) follows a symlink in any existing ancestor segment - only + // the leaf gets an O_NOFOLLOW check below. Accepted as unclosable without a native + // openat2()-style RESOLVE_NO_SYMLINKS binding (Linux-only, and blanket rejection breaks + // legitimate ancestor symlinks like macOS's own /tmp -> /private/tmp); no fs library + // attempts this either. + // + // Permissions are only force-re-asserted on the SDK's own default path (~/.keeper), + // matching KSM-1263's file-level self-healing. A caller-supplied cachePath pointing at a + // pre-existing directory they own (e.g. a file directly inside $HOME) keeps its own + // permissions - narrowing a directory the SDK doesn't own is a bigger side effect than + // this fix should have, but the one directory it does own should still self-heal. const dirExistedBefore = fs.existsSync(dir) fs.mkdirSync(dir, {recursive: true, mode: 0o700}) - // O_DIRECTORY|O_NOFOLLOW makes "is this a real directory, not a symlink" and the open - // the same syscall, so there's no gap between a check and a separate mkdirSync/chmodSync - // for a symlink to be swapped into (Windows caveat above). fchmodSync operates on the fd - // this open returned, pinning the exact inode instead of re-resolving the path a second - // time - though re-resolution still happens at the later file open below, since Node - // has no relative-to-this-fd open; that residual window is narrow (already-checked - // directory to already-checked file, both immediately adjacent to their use) but real. + // fchmodSync on the open fd, not chmodSync by path, pins the exact inode instead of + // re-resolving; O_DIRECTORY|O_NOFOLLOW on the open itself closes the mkdir-to-open gap + // (Windows caveat above). const dfd = fs.openSync(dir, cacheDirOpenFlags) try { - if (!dirExistedBefore) { + if (!dirExistedBefore || isDefaultCachePath) { fs.fchmodSync(dfd, 0o700) } } finally { @@ -497,17 +481,20 @@ const readCacheFile = async (cachePath: string, cacheKey: Uint8Array, maxCacheAg // compile-time type error instead. export const createCachingFunction = ( storage: KeyValueStorage, - { - // Computed here, not at module scope: os.homedir() throws in an environment with no $HOME - // and no matching /etc/passwd entry for the current uid (some containers), and a default - // parameter only evaluates when the caller omits the field - so that failure now only - // reaches a caller who actually relies on the default, at call time, not every consumer - // who merely imports this module. - cachePath = path.join(os.homedir(), '.keeper', 'ksm-cache.dat'), - maxCacheAgeMs = DEFAULT_MAX_CACHE_AGE_MS - }: {cachePath?: string, maxCacheAgeMs?: number} = {} -): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload, allowUnverifiedCertificate?: boolean) => Promise => - async (url, transmissionKey, payload, allowUnverifiedCertificate) => { + options: {cachePath?: string, maxCacheAgeMs?: number} = {} +): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload, allowUnverifiedCertificate?: boolean) => Promise => { + // Captured before defaulting, and threaded down to writeCacheFile: whether the SDK's own + // default directory (~/.keeper) always gets its permissions re-asserted, or a caller-owned + // directory is left alone once it exists - see writeCacheFile's own comment on this. + const isDefaultCachePath = options.cachePath === undefined + // Computed here, not at module scope: os.homedir() throws in an environment with no $HOME + // and no matching /etc/passwd entry for the current uid (some containers), and this only + // evaluates when the caller omits the field - so that failure now only reaches a caller who + // actually relies on the default, at call time, not every consumer who merely imports this + // module. + const cachePath = options.cachePath ?? path.join(os.homedir(), '.keeper', 'ksm-cache.dat') + const maxCacheAgeMs = options.maxCacheAgeMs ?? DEFAULT_MAX_CACHE_AGE_MS + return async (url, transmissionKey, payload, allowUnverifiedCertificate) => { let response: KeeperHttpResponse try { response = await platform.post(url, payload.payload, { @@ -546,7 +533,7 @@ export const createCachingFunction = ( // useObjects mode) reaches deriveCacheKey unchecked and logs a confusing // internal TypeError instead of just skipping the cache write cleanly. if (appKey && isRawKeyBytes(appKey)) { - await writeCacheFile(cachePath, await deriveCacheKey(appKey), Buffer.concat([transmissionKey.key, response.data])) + await writeCacheFile(cachePath, await deriveCacheKey(appKey), Buffer.concat([transmissionKey.key, response.data]), isDefaultCachePath) } } catch (e) { console.error(`Failed to update cached response: ${describeCause(e)}`) @@ -554,3 +541,4 @@ export const createCachingFunction = ( } return response } +} diff --git a/sdk/javascript/packages/core/test/localConfigStorage.test.ts b/sdk/javascript/packages/core/test/localConfigStorage.test.ts index 9d2fc7488..2b7ec8e43 100644 --- a/sdk/javascript/packages/core/test/localConfigStorage.test.ts +++ b/sdk/javascript/packages/core/test/localConfigStorage.test.ts @@ -986,4 +986,27 @@ describe('createCachingFunction (KSM-1265)', () => { (os as any).homedir = originalHomedir } }) + + test('the default ~/.keeper directory keeps self-healing even once it already exists, unlike a caller-supplied one', async () => { + const storage = await makeStorageWithAppKey() + const originalHomedir = os.homedir + ;(os as any).homedir = () => tmpDir + try { + const defaultDir = path.join(tmpDir, '.keeper') + fs.mkdirSync(defaultDir, { mode: 0o755 }) + + const caching = createCachingFunction(storage) + platform.post = async () => ({ statusCode: 200, data: enc.encode('{}'), headers: [] }) + await caching('https://example.com', fakeTransmissionKey(), fakePayload) + + // Unlike a caller-supplied cachePath (see the test above this one), the SDK's own + // default directory is force-reasserted to 0700 even though it already existed - + // it's the one directory the SDK unambiguously owns, so it should keep self-healing + // the same way KSM-1263 already does for the file, not silently trust whatever + // permissions it happens to find on a second or later run. + expect(fs.statSync(defaultDir).mode & 0o777).toBe(0o700) + } finally { + (os as any).homedir = originalHomedir + } + }) }) From 80e82d6b9023b304ff2e96a0000270f9271347f8 Mon Sep 17 00:00:00 2001 From: Stas Schaller Date: Tue, 8 Sep 2026 17:13:20 -0400 Subject: [PATCH 4/6] fix(javascript): route native-ESM Node consumers to the Node build, not the browser bundle (KSM-1265) --- sdk/javascript/packages/core/package.json | 4 ++ .../packages/core/test/exports.test.ts | 43 +++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 sdk/javascript/packages/core/test/exports.test.ts diff --git a/sdk/javascript/packages/core/package.json b/sdk/javascript/packages/core/package.json index 5bf22d42e..fbf8d06a3 100644 --- a/sdk/javascript/packages/core/package.json +++ b/sdk/javascript/packages/core/package.json @@ -11,6 +11,10 @@ "browser": "./dist/browser/index.d.ts", "default": "./dist/node/index.d.ts" }, + "node": { + "import": "./dist/index.cjs.js", + "require": "./dist/index.cjs.js" + }, "browser": "./dist/index.es.js", "import": "./dist/index.es.js", "require": "./dist/index.cjs.js", diff --git a/sdk/javascript/packages/core/test/exports.test.ts b/sdk/javascript/packages/core/test/exports.test.ts new file mode 100644 index 000000000..37b65491d --- /dev/null +++ b/sdk/javascript/packages/core/test/exports.test.ts @@ -0,0 +1,43 @@ +import * as fs from 'fs' +import * as os from 'os' +import * as path from 'path' +import * as childProcess from 'child_process' + +// Jest's own module resolution never goes through package.json's `exports` conditions the way a +// real consumer's `import`/`require` does - every other test in this suite imports via `from +// '../'`, a relative path that bypasses conditional exports entirely. That gap is exactly how a +// broken `exports` block (condition order matters, and Node has no built-in `browser` condition, +// so an unguarded native-ESM import here falls through to the browser bundle) shipped with a +// fully green suite and a clean `tsc --noEmit`. This spawns a real child Node process, in its own +// package with its own `type: module`, to exercise the actual resolution algorithm. +test('an ESM Node consumer resolves the Node build, not the browser bundle', () => { + const scratchDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ksm-exports-test-')) + try { + fs.writeFileSync(path.join(scratchDir, 'package.json'), JSON.stringify({ name: 'exports-test', type: 'module' })) + const scopeDir = path.join(scratchDir, 'node_modules', '@keeper-security') + fs.mkdirSync(scopeDir, { recursive: true }) + fs.symlinkSync(path.resolve(__dirname, '..'), path.join(scopeDir, 'secrets-manager-core'), 'dir') + + const configPath = path.join(scratchDir, 'config.json') + const probeScript = ` + import { localConfigStorage } from '@keeper-security/secrets-manager-core' + const kvs = localConfigStorage(${JSON.stringify(configPath)}) + await kvs.saveString('foo', 'bar') + console.log('OK') + ` + const probePath = path.join(scratchDir, 'probe.mjs') + fs.writeFileSync(probePath, probeScript) + + const result = childProcess.spawnSync(process.execPath, [probePath], { encoding: 'utf8' }) + + // The browser bundle's localConfigStorage resolves in Node too (it's the same exported + // name), but it's backed by IndexedDB, which doesn't exist outside a browser - so it + // fails at the first storage call, not at import time. That's the actual failure mode a + // wrong `exports` resolution produces, and what this asserts against. + expect(result.stderr).not.toContain('indexedDB is not defined') + expect(result.stdout).toContain('OK') + expect(result.status).toBe(0) + } finally { + fs.rmSync(scratchDir, { recursive: true, force: true }) + } +}) From 92358fb7dec64c36405dab17a40154d9cd6939c7 Mon Sep 17 00:00:00 2001 From: Stas Schaller Date: Wed, 9 Sep 2026 11:18:35 -0400 Subject: [PATCH 5/6] fix(javascript): qualify the cache symlink-rejection CHANGELOG claim for Windows (KSM-1265) The KSM-1265 entry said a symlinked cache directory is rejected outright on both read and write with no platform qualifier, which is false on Windows (fs.constants.O_DIRECTORY/O_NOFOLLOW don't exist there, so the check is a silent no-op). The PR description already carried this qualifier; the CHANGELOG didn't. Also drops a review-round reference from a shipped code comment in the same area. --- sdk/javascript/packages/core/CHANGELOG.md | 2 +- sdk/javascript/packages/core/src/node/localConfigStorage.ts | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/sdk/javascript/packages/core/CHANGELOG.md b/sdk/javascript/packages/core/CHANGELOG.md index f74dc32fb..8a5cce44d 100644 --- a/sdk/javascript/packages/core/CHANGELOG.md +++ b/sdk/javascript/packages/core/CHANGELOG.md @@ -22,7 +22,7 @@ // with a custom cache path or freshness window queryFunction: createCachingFunction(storage, {cachePath, maxCacheAgeMs}) ``` - The cache is now encrypted with a key derived from the app key already held in the config (so reading the cache requires the config, not just the cache file), authenticated so a tampered or corrupted file is rejected instead of silently trusted, bounded by a configurable freshness window (default 24h), and located at `~/.keeper/ksm-cache.dat` by default instead of the working directory. Usage was limited to the opt-in caching example, which has been updated to use the new function. If you called `cachingPostFunction` directly, delete the old cache file in your working directory after upgrading; it is not removed automatically. `cachePath` and `maxCacheAgeMs` are now named fields on an options object instead of positional arguments, since Node's and the browser's second positional argument meant different things; the browser signature (`createCachingFunction(storage, maxCacheAgeMs?)`) is unchanged and still non-breaking there, since the new `maxCacheAgeMs` parameter is optional and an old-format cached value is simply treated as a cache miss. A symlinked cache directory is rejected outright (throws) on both read and write. A symlinked cache file itself is not rejected with an error - the write silently replaces it with a real file instead of following it, and the read still requires the actual content underneath to pass its own integrity check - there is no legitimate externally-managed symlink convention for a path the SDK itself names, unlike the config file's own symlink handling, which is unchanged and intentionally different (see the KSM-1266 entry above). Both the config file and the cache file are now written atomically (to a temporary file, then renamed into place, via one shared primitive), so a write that fails partway through can no longer leave a corrupted or truncated file behind. The cache directory is created at `0700`. On the default path (`~/.keeper`), it's re-hardened to `0700` on every write, matching how the cache file itself already self-heals; on a caller-supplied `cachePath` pointing at a directory that already exists (for example, a file directly inside `$HOME`), its permissions are left alone - the SDK does not narrow permissions on a directory it doesn't own. Known limitation: the very first (bind) call's response is not cached, since caching requires an app key that the bind call itself establishes; every call after that caches normally. In the browser, when the app key is held as a non-extractable `CryptoKey` (`useObjects: true`), caching is a no-op rather than an error, the same graceful degradation already used for a network failure with no prior cache. A network failure served from cache now logs a warning, since the caller is getting a response that may be stale. This package now also declares an `exports` field so bundlers and modern TypeScript resolve the correct platform-specific type declarations for the browser bundle; a consumer still on TypeScript's legacy `moduleResolution: "node"` continues to see the Node type declarations regardless, unchanged from before. + The cache is now encrypted with a key derived from the app key already held in the config (so reading the cache requires the config, not just the cache file), authenticated so a tampered or corrupted file is rejected instead of silently trusted, bounded by a configurable freshness window (default 24h), and located at `~/.keeper/ksm-cache.dat` by default instead of the working directory. Usage was limited to the opt-in caching example, which has been updated to use the new function. If you called `cachingPostFunction` directly, delete the old cache file in your working directory after upgrading; it is not removed automatically. `cachePath` and `maxCacheAgeMs` are now named fields on an options object instead of positional arguments, since Node's and the browser's second positional argument meant different things; the browser signature (`createCachingFunction(storage, maxCacheAgeMs?)`) is unchanged and still non-breaking there, since the new `maxCacheAgeMs` parameter is optional and an old-format cached value is simply treated as a cache miss. A symlinked cache directory is rejected outright (throws) on both read and write, on POSIX platforms; Windows has no equivalent `O_NOFOLLOW`/`O_DIRECTORY` protection, so on that platform the cache's protection is the encryption and authentication alone. A symlinked cache file itself is not rejected with an error - the write silently replaces it with a real file instead of following it, and the read still requires the actual content underneath to pass its own integrity check - there is no legitimate externally-managed symlink convention for a path the SDK itself names, unlike the config file's own symlink handling, which is unchanged and intentionally different (see the KSM-1266 entry above). Both the config file and the cache file are now written atomically (to a temporary file, then renamed into place, via one shared primitive), so a write that fails partway through can no longer leave a corrupted or truncated file behind. The cache directory is created at `0700`. On the default path (`~/.keeper`), it's re-hardened to `0700` on every write, matching how the cache file itself already self-heals; on a caller-supplied `cachePath` pointing at a directory that already exists (for example, a file directly inside `$HOME`), its permissions are left alone - the SDK does not narrow permissions on a directory it doesn't own. Known limitation: the very first (bind) call's response is not cached, since caching requires an app key that the bind call itself establishes; every call after that caches normally. In the browser, when the app key is held as a non-extractable `CryptoKey` (`useObjects: true`), caching is a no-op rather than an error, the same graceful degradation already used for a network failure with no prior cache. A network failure served from cache now logs a warning, since the caller is getting a response that may be stale. This package now also declares an `exports` field so bundlers and modern TypeScript resolve the correct platform-specific type declarations for the browser bundle; a consumer still on TypeScript's legacy `moduleResolution: "node"` continues to see the Node type declarations regardless, unchanged from before. - Maintenance: Updated `minimatch`, `@babel/core`, and `handlebars` dev dependencies. ## 17.5.0 diff --git a/sdk/javascript/packages/core/src/node/localConfigStorage.ts b/sdk/javascript/packages/core/src/node/localConfigStorage.ts index 70dc0e469..d064c40e1 100644 --- a/sdk/javascript/packages/core/src/node/localConfigStorage.ts +++ b/sdk/javascript/packages/core/src/node/localConfigStorage.ts @@ -376,8 +376,7 @@ export const localConfigStorage = (configName?: string): KeyValueStorage => { // protection the cache directory/file has, so a silent no-op here is a bigger gap. There is no // good fallback available without a real openat()-style relative-to-fd primitive, which Node's // public fs API doesn't expose - accepted as a documented, POSIX-only limitation rather than -// building a weaker check-then-open substitute, matching how KSM-1266's own review already -// decided the identical Windows gap for O_NOFOLLOW. +// building a weaker check-then-open substitute. const hasDirectorySymlinkProtection = typeof fs.constants.O_DIRECTORY === 'number' && typeof fs.constants.O_NOFOLLOW === 'number' const cacheDirOpenFlags = hasDirectorySymlinkProtection ? fs.constants.O_DIRECTORY | fs.constants.O_NOFOLLOW : fs.constants.O_DIRECTORY const cacheFileReadFlags = hasDirectorySymlinkProtection ? fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW : fs.constants.O_RDONLY From 51a77bc2fed0039750a414194e38afb4d838f8b8 Mon Sep 17 00:00:00 2001 From: Mateo Gallego Date: Mon, 14 Sep 2026 11:42:15 -0400 Subject: [PATCH 6/6] fix(javascript): route a node-less, browser-less import condition to the Node build (KSM-1265) package.json's exports map left the generic import condition pointing at the browser bundle, so a bundler resolving with a condition set that omits both node and browser (for example @rollup/plugin-node-resolve at its documented defaults) fell through to a build backed by IndexedDB and crashed at the first storage call outside an actual browser. Points import at the same dist/index.cjs.js the node condition already resolves to. test/exports.test.ts gains a package.json exports condition resolution suite that walks the same package-exports algorithm Node itself implements, parametrized by condition set, since the existing test spawns a real Node process and Node always adds its own node condition, so it cannot reach this case. --- sdk/javascript/packages/core/package.json | 2 +- .../packages/core/test/exports.test.ts | 52 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/sdk/javascript/packages/core/package.json b/sdk/javascript/packages/core/package.json index fbf8d06a3..26e1e361e 100644 --- a/sdk/javascript/packages/core/package.json +++ b/sdk/javascript/packages/core/package.json @@ -16,7 +16,7 @@ "require": "./dist/index.cjs.js" }, "browser": "./dist/index.es.js", - "import": "./dist/index.es.js", + "import": "./dist/index.cjs.js", "require": "./dist/index.cjs.js", "default": "./dist/index.cjs.js" }, diff --git a/sdk/javascript/packages/core/test/exports.test.ts b/sdk/javascript/packages/core/test/exports.test.ts index 37b65491d..35da67af8 100644 --- a/sdk/javascript/packages/core/test/exports.test.ts +++ b/sdk/javascript/packages/core/test/exports.test.ts @@ -3,6 +3,58 @@ import * as os from 'os' import * as path from 'path' import * as childProcess from 'child_process' +// A minimal, faithful implementation of Node's own package-exports condition matching +// (https://nodejs.org/api/packages.html#conditional-exports): walk an exports sub-object's own +// keys in the order they're written, take the first key that is 'default' or appears in the +// caller's condition set, recursing into nested objects. This is the only way to test a +// condition set that omits 'node' - a real Node.js process always adds that condition itself and +// cannot be made to drop it, so childProcess.spawnSync (used above) can prove the fix works for +// Node but not that the fix's target scenario, a bundler with no 'node' condition, is covered. +const resolveCondition = (node: unknown, conditions: string[]): string | undefined => { + if (typeof node === 'string') return node + if (Array.isArray(node)) { + for (const item of node) { + const resolved = resolveCondition(item, conditions) + if (resolved !== undefined) return resolved + } + return undefined + } + if (typeof node === 'object' && node !== null) { + for (const key of Object.keys(node)) { + if (key === 'default' || conditions.includes(key)) { + const resolved = resolveCondition((node as Record)[key], conditions) + if (resolved !== undefined) return resolved + } + } + } + return undefined +} + +describe('package.json exports condition resolution', () => { + const exportsNode = JSON.parse(fs.readFileSync(path.join(__dirname, '..', 'package.json'), 'utf8')).exports['.'] + + // A bundler with no 'node' and no 'browser' in its condition set (for example + // @rollup/plugin-node-resolve at its documented defaults, ['default', 'module', 'import']) + // is not declaring a target platform at all, just an ESM preference. Without this fix, it + // fell through to the 'import' key, which pointed at the browser bundle - the same + // ReferenceError: indexedDB is not defined failure the Node-specific test above guards + // against, just for a resolver the previous fix's real-Node-process test cannot reach. + test('an import-only bundler condition set (no node, no browser) resolves the Node build', () => { + expect(resolveCondition(exportsNode, ['import', 'default'])).toBe('./dist/index.cjs.js') + expect(resolveCondition(exportsNode, ['module', 'import', 'default'])).toBe('./dist/index.cjs.js') + }) + + test('a real Node ESM or CJS consumer still resolves the Node build', () => { + expect(resolveCondition(exportsNode, ['node', 'import', 'default'])).toBe('./dist/index.cjs.js') + expect(resolveCondition(exportsNode, ['node', 'require', 'default'])).toBe('./dist/index.cjs.js') + }) + + test('a browser-targeting bundler still resolves the browser build', () => { + expect(resolveCondition(exportsNode, ['browser', 'import', 'default'])).toBe('./dist/index.es.js') + expect(resolveCondition(exportsNode, ['browser', 'require', 'default'])).toBe('./dist/index.es.js') + }) +}) + // Jest's own module resolution never goes through package.json's `exports` conditions the way a // real consumer's `import`/`require` does - every other test in this suite imports via `from // '../'`, a relative path that bypasses conditional exports entirely. That gap is exactly how a