fix(node-client-sdk): better handling for bad filesystem states#1799
fix(node-client-sdk): better handling for bad filesystem states#1799joker23 wants to merge 1 commit into
Conversation
- refuse to load from symlinks - fallsback to inmemory store if filesystem loading fails - additionally, exposing `ConnectionMode` type for downstream reference
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/browser size report |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ced01fd. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // fs.mkdir succeeds silently if the path already exists as a symlink to a directory, so a | ||
| // pre-planted symlink would otherwise redirect where the cache is read from and written to | ||
| // without ever surfacing as an init failure. | ||
| if ((await fs.lstat(this._storageDir)).isSymbolicLink()) { | ||
| throw new Error(`Storage directory path is a symlink, not a real directory: ${this._storageDir}`); | ||
| } |
There was a problem hiding this comment.
I included this check in the default storage implementation to protect against all the funny business symlinks can introduce... after some discussion and thought, this might be too opinionated at this level? I am leaning towards removing this check.
One defense for keeping this is: if an application developer wants to support symlinks or anything, they can override the storage implementation.
1 participant
ConnectionModetype for downstream referenceNote
Medium Risk
Changes filesystem trust boundaries and persistence behavior for the local flag cache; misconfigured hosts may silently lose cross-restart persistence, but runtime flag access should remain available.
Overview
Hardens
NodeStoragelocal flag cache initialization so unsafe or broken filesystem layouts no longer redirect reads/writes or leave the SDK unusable.If the storage directory is a symlink, the cache file is not a regular file, or init otherwise fails (including when fixing a malformed cache cannot be written), persistence is disabled and the SDK warns once and keeps serving flags from an in-memory map.
get/set/clearno longer log errors on init failure; disk flushes are skipped when persistence is off. Symlinkedldcache.jsonpaths are treated as malformed and discarded without loading symlink targets.Also re-exports
ConnectionModefromLDCommonfor downstream typing. Tests were expanded for symlink, file-at-path, and fallback scenarios; platform tests expect the init-fallback warn instead of storage errors.Reviewed by Cursor Bugbot for commit ced01fd. Bugbot is set up for automated code reviews on this repo. Configure here.