Add bounded debugger storage inspection - #159
Conversation
📊 PR Size: size/XLTotal changes: 1289 lines (3 files) Top files changed:
Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+) |
|
| Test Suite | Result |
|---|---|
| valdi_web Integration Test | ✅ success |
| Valdi Smoke Tests | ✅ success |
| Linux: Module Tests | ✅ success |
| Snapshot Tests | ✅ success |
| Linux: C++ Tests | ❌ failure |
| API Surface Check | ✅ success |
| Linux: Build & Export | ✅ success |
| macOS: C++ & Platform Tests | ❌ failure |
| Test Coverage Delta | ✅ success |
| Linux: Registry Validation | ✅ success |
| Linux: Build Compiler | ✅ success |
| Linux: Hotreload Smoke | ✅ success |
Some tests failed. Please check the workflow logs for details.
🚀 Bazel remote cache is now enabled - future builds will be faster!
Workflow: Valdi CI
clholgat
left a comment
There was a problem hiding this comment.
Storage-inspection review. One medium item inline below.
Note: this stack is being squashed to ~3 PRs — please carry this feedback into whichever squashed PR this change lands in.
| } | ||
|
|
||
| /** Return a bounded, read-only view over current and persisted legacy web stores. */ | ||
| export function getPersistentStoreSnapshot(): WebPersistentStoreSnapshot { |
There was a problem hiding this comment.
🟠 Med — getPersistentStoreSnapshot enforces thorough size/entry/store/aggregate caps, but it performs no redaction and has no isDebugEnabled/opt-in gate: it returns the raw persisted values (auth tokens, session data, PII), truncated only by length. The "bounded" guarantee here is about DoS, not confidentiality.
Failure scenario: once a later PR wires this over the __VALDI_WEB_DEBUGGER__ bridge, any attached client (or any same-runtime script that can call the export) can read every persisted secret verbatim.
Suggested fix: gate the snapshot behind the same debug-enabled + nonce/loopback transport the bridge PR introduces, and redact/hash values for keys matching common secret patterns (token/auth/session/key), surfacing only length + presence by default. At minimum, add a doc comment stating this returns unredacted values and must never be exposed on an ungated transport.
clholgat
left a comment
There was a problem hiding this comment.
Storage-inspection review. One medium item inline below.
Note: this stack is being squashed to ~3 PRs — please carry this feedback into whichever squashed PR this change lands in.
|
Superseded by #180, which consolidates this patch into the reviewed debugger capabilities landing unit. The replacement carries forward the feedback and fixes discussed here. Closing this draft to reduce the active stack; this PR and its discussion remain the historical review record. |
Description
Adds a compatibility-preserving, bounded, read-only snapshot over the existing web PersistentStore implementation.
Type of Change
Testing
bazel test //...)Testing Details
npm testpassed 436/436; the CLI production build passed.//src/valdi_modules/src/valdi/web_renderer:testpassed.bazel query //...passed.Checklist
Related Issues
Relates to #154
Additional Context
Stack 6/22. Stacked on #158 (
bjd/debugger-devtools-core). Review this PR as the single incremental commit06c5b4e0against that base; do not merge it before its parent.