chore(node-client): harden existing platform implementation#1403
chore(node-client): harden existing platform implementation#1403joker23 wants to merge 7 commits into
Conversation
Improves robustness of the already-merged Node platform layer: - NodeStorage logs swallowed flush errors, validates and logs on malformed cache reads, writes the temp file with an exclusive (symlink-safe) open, and warns on localStoragePath mismatch across the process singleton. - NodeResponse caps the buffered response body to guard against memory exhaustion from an oversized or hostile response. - NodeRequests applies a default request timeout so a stuck server cannot hang polling or event delivery indefinitely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Pins the new hardening behavior: - NodeStorage: warn on malformed cache, non-string cache values ignored, symlink-safe temp write, localStoragePath-mismatch warning. - NodeResponse: rejects when the body exceeds the size cap (exports MAX_RESPONSE_BYTES so the test references the exact threshold). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@cursor review |
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/browser size report |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@cursor review |
|
@cursor review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| try { | ||
| await fs.writeFile(this._tempFile, content, { encoding: 'utf8', mode: 0o600 }); | ||
| try { | ||
| await fs.unlink(this._tempFile); |
There was a problem hiding this comment.
I assume the file being missing isn't the only reason it cannot be deleted. For example if it was created by a different user or with different permissions.
There was a problem hiding this comment.
yea that would also cause issues, but the exception will be caught further down.
This is a larger problem that needs to be resolved in a separate PR
this is not standard for our sdks
| await handle.close(); | ||
| handle = undefined; | ||
| await fs.rename(this._tempFile, this._storageFile); | ||
| } catch (error) { |
There was a problem hiding this comment.
It does feel like we probably should log something in this error handler.
Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
2 participants
Hardens the Node platform layer landed in #1393, ahead of the client-side SDK implementation that builds on it. First atomic in the node-client port stack.
wxopen so the write cannot be redirected through a symlink; warn whengetNodeStorageis called with alocalStoragePaththat differs from the process singleton's.Note
Medium Risk
Changes local filesystem cache read/write semantics and symlink-resistant atomic writes; impact is confined to on-disk flag cache, but misconfiguration or path warnings could surprise multi-instance setups in one process.
Overview
Hardens Node flag disk cache behavior in
NodeStorageandgetNodeStorage.Load path: Missing
ldcache.jsonno longer triggers a rewrite or warning. Invalid JSON or other read failures log a malformed cache warning and reset via an atomic write. Loaded JSON must be a plain object; only string values are kept (arrays, numbers, nested objects are dropped).Write path: Persists through an exclusive
wxopen on the temp file (after unlinking any existing temp path) instead of a plainwriteFile, so a symlink at the temp path cannot redirect writes to another file.Singleton:
getNodeStoragerecords the firstlocalStoragePathand warns if a later call passes a different path;resetNodeStorageclears that path as well.Tests cover first-run behavior, malformed cache warnings, non-string entries, symlink resistance, and the path mismatch warning.
Reviewed by Cursor Bugbot for commit a00473e. Bugbot is set up for automated code reviews on this repo. Configure here.