Add integrated debugger DevTools core - #158
Conversation
📊 PR Size: size/XLTotal changes: 6888 lines (26 files) Top files changed:
...and 16 more files Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+) |
|
| Test Suite | Result |
|---|---|
| Valdi Smoke Tests | ❌ failure |
| valdi_web Integration Test | ✅ success |
| Linux: Build & Export | ✅ success |
| Linux: C++ Tests | ✅ success |
| Test Coverage Delta | ✅ success |
| macOS: C++ & Platform Tests | ❌ failure |
| API Surface Check | ✅ success |
| Linux: Registry Validation | ✅ success |
| Snapshot Tests | ✅ success |
| Linux: Module Tests | ✅ success |
| Linux: Hotreload Smoke | ✅ success |
| Linux: Build Compiler | ✅ 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.
DevTools core review. One medium item inline; one low item below.
🟢 Low — serveStatic (server.ts ~2243-2246) stops sending X-Frame-Options: DENY for devtools-panel.html and relaxes CSP frame-ancestors to chrome-extension://*. That permits any installed Chromium extension (not only the ephemeral Valdi one) to embed the loopback panel. Framing risk is low here (an embedder still lacks the target nonce and cross-origin reads are blocked), but it would be cleaner to pin to the specific extension origin if the unpacked-extension ID allows it, or add a comment explaining why the wildcard is required.
Note: this stack is being squashed to ~3 PRs — please carry this feedback into whichever squashed PR these changes land in.
| if (expression.length > 10_000) { | ||
| throw new ApiRequestError(400, 'Web preview console expressions cannot exceed 10,000 characters.'); | ||
| } | ||
| const wrapped = |
There was a problem hiding this comment.
🟠 Med — this endpoint forwards a caller-supplied expression into the inspected application for evaluation. Its only real authorization is the page-injected __VALDI_DEVTOOLS_TARGET_NONCE__ guard; the loopback binding and the application/json content-type requirement are useful defense-in-depth but are not authorization.
Concern: if that nonce is ever logged, shortened, made predictable, or the page-side guard is dropped during a refactor, this path silently loses its only protection.
Suggested fix: add an explicit comment here (and at the guard in owlCdpClient) stating that the per-session nonce is the sole authorization boundary and that its entropy and secrecy must be preserved, so a future change does not weaken it unintentionally.
|
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 the top-level browser DevTools surface, bounded tree model, Owl and Chromium clients, loopback routes, and package integration.
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 5/22. Stacked on #157 (
bjd/debugger-web-bridge). Review this PR as the single incremental commit5f768c6fagainst that base; do not merge it before its parent.