fix(devtools-vite): avoid piping raw DOM console arguments#424
Open
Dzinlife wants to merge 1 commit intoTanStack:mainfrom
Open
fix(devtools-vite): avoid piping raw DOM console arguments#424Dzinlife wants to merge 1 commit intoTanStack:mainfrom
Dzinlife wants to merge 1 commit intoTanStack:mainfrom
Conversation
📝 WalkthroughWalkthroughThe pull request improves console argument serialization in the devtools-vite package by introducing a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant Console as Console Interceptor
participant Serializer as serializeConsoleArg
participant Fetch as Fetch/SSE Endpoint
Client->>Console: console.warn(domElement, ...)
Console->>Serializer: serialize each argument
Serializer->>Serializer: detect type (DOM, Error, Date, etc.)
Serializer->>Serializer: convert to safe representation
Serializer-->>Console: serialized arguments
Console->>Fetch: POST request body with serialized args
Fetch-->>Console: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/devtools-vite/src/virtual-console.ts (2)
177-184: Optional: preferObject.keysoverfor...in+hasOwnProperty.
for (var key in arg)walks the prototype chain and is then filtered byhasOwnProperty.call.Object.keys(arg)returns the same set of own enumerable string keys directly and is the more idiomatic choice here (and avoids surprises with prototype-polluted globals).♻️ Proposed change
var objectResult = {}; - for (var key in arg) { - if (Object.prototype.hasOwnProperty.call(arg, key)) { - objectResult[key] = serializeConsoleArg(arg[key], seen); - } - } + var ownKeys = Object.keys(arg); + for (var ki = 0; ki < ownKeys.length; ki++) { + var key = ownKeys[ki]; + objectResult[key] = serializeConsoleArg(arg[key], seen); + } seen.pop(); return objectResult;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools-vite/src/virtual-console.ts` around lines 177 - 184, The loop building objectResult in serializeConsoleArg currently uses for...in + hasOwnProperty which walks the prototype chain; change it to iterate Object.keys(arg).forEach or a for..of over Object.keys(arg) so only own enumerable keys are processed, e.g., replace the for (var key in arg) { if (Object.prototype.hasOwnProperty.call(arg, key)) { ... } } with direct iteration of Object.keys(arg) and keep the same serializeConsoleArg(arg[key], seen) assignment and seen.pop() logic unchanged.
162-185: Consider depth/breadth caps to fully achieve the stall-prevention goal.The current serializer recurses without any depth or length limits. This still leaves a few easy ways to produce huge payloads or stall the page — e.g., a
Uint8Array/Bufferof length N falls through to the object branch (sinceArray.isArrayreturnsfalse) andfor...inmaterializes N numeric-indexed properties. Deeply nested config-like objects can also blow the stack or balloon the body sent to fetch.Since the PR's goal is to avoid stalling on serialization, capping
MAX_DEPTH, array length, object key count, and string length would close those remaining holes.♻️ Sketch of caps + TypedArray short-circuit
- function serializeConsoleArg(arg, seen) { + var MAX_DEPTH = 6; + var MAX_ARRAY_LEN = 100; + var MAX_OBJECT_KEYS = 100; + + function serializeConsoleArg(arg, seen, depth) { + if (depth === undefined) depth = 0; if (arg === undefined) return 'undefined'; if (arg === null) return null; @@ if (argType !== 'object') return arg; @@ + if (depth >= MAX_DEPTH) return '[MaxDepth]'; + + if (typeof ArrayBuffer !== 'undefined' && ArrayBuffer.isView && ArrayBuffer.isView(arg)) { + return '[' + (arg.constructor && arg.constructor.name || 'TypedArray') + '(' + arg.length + ')]'; + } + if (seen.indexOf(arg) !== -1) { return '[Circular]'; } seen.push(arg); if (Array.isArray(arg)) { var arrayResult = []; - for (var arrayIndex = 0; arrayIndex < arg.length; arrayIndex++) { - arrayResult.push(serializeConsoleArg(arg[arrayIndex], seen)); + var arrayLen = Math.min(arg.length, MAX_ARRAY_LEN); + for (var arrayIndex = 0; arrayIndex < arrayLen; arrayIndex++) { + arrayResult.push(serializeConsoleArg(arg[arrayIndex], seen, depth + 1)); } + if (arg.length > MAX_ARRAY_LEN) arrayResult.push('… (' + (arg.length - MAX_ARRAY_LEN) + ' more)'); seen.pop(); return arrayResult; } var objectResult = {}; - for (var key in arg) { - if (Object.prototype.hasOwnProperty.call(arg, key)) { - objectResult[key] = serializeConsoleArg(arg[key], seen); - } - } + var keys = Object.keys(arg); + var keyLimit = Math.min(keys.length, MAX_OBJECT_KEYS); + for (var ki = 0; ki < keyLimit; ki++) { + objectResult[keys[ki]] = serializeConsoleArg(arg[keys[ki]], seen, depth + 1); + } + if (keys.length > MAX_OBJECT_KEYS) objectResult['…'] = '(' + (keys.length - MAX_OBJECT_KEYS) + ' more keys)'; seen.pop(); return objectResult; }Recursive call sites at lines 136-137 and 171/180 would also need to thread
depth + 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools-vite/src/virtual-console.ts` around lines 162 - 185, The serializer serializeConsoleArg currently recurses without limits; add caps to prevent huge/stalling payloads by introducing MAX_DEPTH, MAX_ITEMS (for arrays/objects), and MAX_STRING_LENGTH constants, add a depth parameter (default 0) that is incremented at each recursive call, and return a truncated marker like "[MaxDepth]" or truncated strings when depth exceeds MAX_DEPTH or items exceed MAX_ITEMS; also short-circuit binary/typed arrays (detect ArrayBuffer, SharedArrayBuffer, and TypedArray/Buffer) to return a summarized placeholder instead of iterating their numeric properties, and ensure every recursive call site (including where serializeConsoleArg(arg[arrayIndex], seen) and object property calls) passes depth + 1 and enforces item and string caps.packages/devtools-vite/src/virtual-console.test.ts (1)
80-133: Solid regression test for DOM serialization.Capturing
console.warnbefore evaluating the generated IIFE so the mock is bound asoriginalConsole.warn, then asserting it was invoked with the rawbuttonelement is a clean way to confirm both that the original logger receives unmodified args and that the piped payload uses the serialized form.Optional: consider a follow-up test for circular references and
Errorto lock in those branches ofserializeConsoleArg, since they're new behaviors not covered elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools-vite/src/virtual-console.test.ts` around lines 80 - 133, Add tests that cover serializeConsoleArg's circular-reference and Error branches: create a new test (e.g., "serializes circular refs and Error objects before sending logs") that follows the same pattern as the existing serializes DOM elements test—use vi.useFakeTimers(), stub global fetch and EventSource, capture original console.warn, call new Function(generateConsolePipeCode(['warn'], TEST_VITE_URL))(), then invoke console.warn with (1) an object containing a circular reference and (2) an Error instance; advance timers, assert originalWarnMock got the raw arguments, assert fetch was called and parse init.body to verify body.entries contain serialized representations for the circular object (no throws, handles "[Circular]" or chosen format) and a serialized Error (name, message, stack or message), referencing serializeConsoleArg, generateConsolePipeCode, fetchMock and originalWarnMock to locate code paths to validate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/devtools-vite/src/virtual-console.test.ts`:
- Around line 80-133: Add tests that cover serializeConsoleArg's
circular-reference and Error branches: create a new test (e.g., "serializes
circular refs and Error objects before sending logs") that follows the same
pattern as the existing serializes DOM elements test—use vi.useFakeTimers(),
stub global fetch and EventSource, capture original console.warn, call new
Function(generateConsolePipeCode(['warn'], TEST_VITE_URL))(), then invoke
console.warn with (1) an object containing a circular reference and (2) an Error
instance; advance timers, assert originalWarnMock got the raw arguments, assert
fetch was called and parse init.body to verify body.entries contain serialized
representations for the circular object (no throws, handles "[Circular]" or
chosen format) and a serialized Error (name, message, stack or message),
referencing serializeConsoleArg, generateConsolePipeCode, fetchMock and
originalWarnMock to locate code paths to validate.
In `@packages/devtools-vite/src/virtual-console.ts`:
- Around line 177-184: The loop building objectResult in serializeConsoleArg
currently uses for...in + hasOwnProperty which walks the prototype chain; change
it to iterate Object.keys(arg).forEach or a for..of over Object.keys(arg) so
only own enumerable keys are processed, e.g., replace the for (var key in arg) {
if (Object.prototype.hasOwnProperty.call(arg, key)) { ... } } with direct
iteration of Object.keys(arg) and keep the same serializeConsoleArg(arg[key],
seen) assignment and seen.pop() logic unchanged.
- Around line 162-185: The serializer serializeConsoleArg currently recurses
without limits; add caps to prevent huge/stalling payloads by introducing
MAX_DEPTH, MAX_ITEMS (for arrays/objects), and MAX_STRING_LENGTH constants, add
a depth parameter (default 0) that is incremented at each recursive call, and
return a truncated marker like "[MaxDepth]" or truncated strings when depth
exceeds MAX_DEPTH or items exceed MAX_ITEMS; also short-circuit binary/typed
arrays (detect ArrayBuffer, SharedArrayBuffer, and TypedArray/Buffer) to return
a summarized placeholder instead of iterating their numeric properties, and
ensure every recursive call site (including where
serializeConsoleArg(arg[arrayIndex], seen) and object property calls) passes
depth + 1 and enforces item and string caps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7301ded-7d58-4613-8271-a3ee674de38a
📒 Files selected for processing (3)
.changeset/safe-console-dom-pipe.mdpackages/devtools-vite/src/virtual-console.test.tspackages/devtools-vite/src/virtual-console.ts
Dzinlife
force-pushed
the
codex/safe-console-piping-dom-serialization
branch
from
8d202e6 to
cd077e3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Changes
Console piping currently only probes console arguments with
JSON.stringify, then sends the original argument when the probe succeeds. DOM nodes such as buttons can pass that probe, so a console warning that includes an element can put the raw DOM object into the payload and make the browser stall while the devtools runtime serializes it.This PR serializes console arguments into JSON-safe values before sending them through the Vite console pipe:
<button id="save" class="primary" type="button">It also adds regression tests for logging DOM elements, circular references, errors, and large payloads through
console.warn.✅ Checklist
pnpm test:pr.Ran targeted validation locally:
pnpm --filter @tanstack/devtools-vite test:lib -- virtual-console.test.tspnpm --filter @tanstack/devtools-vite test:typespnpm --filter @tanstack/devtools-vite test:eslintpnpm --filter @tanstack/devtools-vite buildpnpm exec prettier --check .changeset/safe-console-dom-pipe.md packages/devtools-vite/src/virtual-console.ts packages/devtools-vite/src/virtual-console.test.ts🚀 Release Impact