feat: add initial browser demo setup with Vite and React components#58
Merged
frontegg-david merged 8 commits intomainfrom Feb 25, 2026
Merged
feat: add initial browser demo setup with Vite and React components#58frontegg-david merged 8 commits intomainfrom
frontegg-david merged 8 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new browser sandbox library ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as React App
participant BE as BrowserEnclave
participant IA as IframeAdapter
participant Outer as Outer Iframe
participant Inner as Inner Iframe
participant Tool as ToolHandler
User->>App: enter code & click Run
App->>BE: BrowserEnclave.run(code, toolHandler?)
BE->>BE: validate/serialize config
BE->>IA: execute(code, context)
IA->>Outer: create outer iframe (srcdoc)
Outer->>Inner: create inner iframe (srcdoc)
Inner->>Inner: execute user code
alt tool-call
Inner->>Outer: postMessage(tool-call)
Outer->>IA: forward tool-call to host
IA->>Tool: invoke toolHandler
Tool-->>IA: return tool result
IA->>Outer: postMessage(tool-response)
Outer->>Inner: postMessage(tool-response)
end
Inner->>Outer: postMessage(result)
Outer->>IA: forward result
IA->>BE: return ExecutionResult (success/error & stats)
BE->>App: resolve run promise with result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (1)
libs/browser/e2e/buffer-shim.mjs (1)
5-5:⚠️ Potential issue | 🟡 MinorRemove the
_prefix from_encoder— it is not an unused variable.Same issue as in
apps/browser-demo/scripts/buffer-shim.mjs:_encoderis actively used on line 10 and should not carry the_prefix per coding guidelines.As per coding guidelines: "unused parameters and variables prefixed with underscore
_".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/buffer-shim.mjs` at line 5, Rename the variable `_encoder` to `encoder` throughout the module (declaration using new TextEncoder() and all usages, e.g., where the encoder is used later in the file) so it no longer carries the leading underscore; update every reference to `_encoder` to `encoder` to comply with the coding guideline that only unused variables are prefixed with `_`.
🟡 Minor comments (10)
apps/browser-demo/src/App.css-368-372 (1)
368-372:⚠️ Potential issue | 🟡 MinorReplace deprecated
word-break: break-word.Stylelint flags this keyword as deprecated; prefer
overflow-wrap: anywhere.🧹 Suggested fix
.result-pre { font-family: 'SF Mono', 'Fira Code', Menlo, Consolas, monospace; font-size: 0.8rem; white-space: pre-wrap; - word-break: break-word; + overflow-wrap: anywhere; margin: 0; } @@ .console-msg { - word-break: break-word; + overflow-wrap: anywhere; }Also applies to: 403-404
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.css` around lines 368 - 372, The CSS class .result-pre uses the deprecated property value "word-break: break-word"; replace that declaration with the modern equivalent "overflow-wrap: anywhere" (and remove the deprecated rule) to satisfy stylelint; apply the same change to the other occurrences noted (the rules around lines 403-404) so all instances of "word-break: break-word" are replaced with "overflow-wrap: anywhere".apps/browser-demo/src/components/ExecutionControls.tsx-9-25 (1)
9-25:⚠️ Potential issue | 🟡 MinorAlign status class with status text precedence.
If
readyandenclaveErrorare both truthy, the text shows an error while the class shows “ready”.✅ Safer status derivation
export function ExecutionControls({ onRun, running, ready, enclaveLoading, enclaveError }: ExecutionControlsProps) { const canRun = ready && !running; + const statusText = enclaveLoading + ? 'Loading enclave...' + : enclaveError + ? `Error: ${enclaveError}` + : ready + ? 'Ready' + : 'Initializing...'; + const statusClass = enclaveLoading + ? 'status-loading' + : enclaveError + ? 'status-error' + : ready + ? 'status-ready' + : 'status-loading'; return ( <div className="execution-controls"> <button className="run-btn" onClick={onRun} disabled={!canRun}> {running ? 'Running...' : 'Run'} </button> - <span className={`status-indicator ${ready ? 'status-ready' : enclaveError ? 'status-error' : 'status-loading'}`}> - {enclaveLoading - ? 'Loading enclave...' - : enclaveError - ? `Error: ${enclaveError}` - : ready - ? 'Ready' - : 'Initializing...'} - </span> + <span className={`status-indicator ${statusClass}`}>{statusText}</span> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/components/ExecutionControls.tsx` around lines 9 - 25, The status class and text precedence in ExecutionControls are inconsistent (ready can override error visually); compute a single derived status value (e.g., in ExecutionControls create a status variable using the same precedence: enclaveLoading -> 'loading', enclaveError -> 'error', ready -> 'ready', else 'initializing') and then use that status variable both to build the span className (e.g., `status-indicator status-${status}`) and to select the displayed text (map statuses to "Loading enclave...", `Error: ${enclaveError}`, "Ready", "Initializing..."); this ensures enclaveError wins over ready and keeps class and text in sync.apps/browser-demo/src/components/ToolHandlerConfig.tsx-11-18 (1)
11-18:⚠️ Potential issue | 🟡 MinorAvoid nested
<label>elements.A
<label>cannot contain another<label>; per the HTML Living Standard, label elements must not have any descendant label elements. This violates the spec and can confuse assistive technology.✅ Safer markup
- <label className="section-label"> - Tool Handler - <label className="toggle-label"> - <input type="checkbox" checked={enabled} onChange={(e) => onToggle(e.target.checked)} disabled={disabled} /> - <span>{enabled ? 'Enabled' : 'Disabled'}</span> - </label> - </label> + <div className="section-label"> + <span>Tool Handler</span> + <label className="toggle-label"> + <input type="checkbox" checked={enabled} onChange={(e) => onToggle(e.target.checked)} disabled={disabled} /> + <span>{enabled ? 'Enabled' : 'Disabled'}</span> + </label> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/components/ToolHandlerConfig.tsx` around lines 11 - 18, The JSX in the ToolHandlerConfig component currently nests a <label> inside another <label>; change the markup so labels are not nested: make the outer "section-label" a non-label container (e.g., a div or span) and keep a single <label> for the checkbox (or use a label with htmlFor). Add a unique id to the input and wire the label via htmlFor (or wrap only the input and its text in the one label) so props enabled, onToggle and disabled remain unchanged and accessibility is preserved.apps/browser-demo/src/hooks/use-console-capture.ts-12-36 (1)
12-36:⚠️ Potential issue | 🟡 MinorDouble
startCapturewithoutstopCaptureloses original console references.If
startCaptureis called while a capture is already active (e.g., rapid re-execution),originalsRef.currentis overwritten with the already-patched methods, making it impossible to restore the real console.Guard against this at the top of
startCapture:🛡️ Proposed fix
const startCapture = useCallback(() => { + // Restore previous patch first if still active + if (originalsRef.current) { + const levels: ConsoleLevel[] = ['log', 'info', 'warn', 'error']; + for (const level of levels) { + console[level] = originalsRef.current[level]; + } + originalsRef.current = null; + } + entriesRef.current = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/hooks/use-console-capture.ts` around lines 12 - 36, startCapture currently overwrites originalsRef.current when called multiple times, losing the real console methods; modify startCapture to early-return if originalsRef.current is already populated (or track a capturing flag) so you only bind and replace console methods once, ensuring originalsRef.current retains the original console methods used by stopCapture to restore them; reference startCapture, originalsRef, entriesRef, ENCLAVE_PREFIX, nextId and stopCapture when making the change.apps/browser-demo/src/App.tsx-39-71 (1)
39-71:⚠️ Potential issue | 🟡 MinorPreserve console output and stats on failed runs.
In the error path,
stopCapture()is called but captured entries aren’t stored and stats remain null, so failures lose console output and the stats panel stays empty. Move capture tofinallyand set stats for errors.🛠️ Suggested fix
startCapture(); try { const execResult = await run(code); - const captured = stopCapture(); - setConsoleEntries(captured); setResult(execResult); if (execResult.stats) { setStats({ duration: execResult.stats.duration, toolCallCount: execResult.stats.toolCallCount, iterationCount: execResult.stats.iterationCount, }); } } catch (err) { - stopCapture(); setResult({ success: false, error: { name: 'AppError', message: err instanceof Error ? err.message : String(err), }, stats: { duration: 0, toolCallCount: 0, iterationCount: 0 }, }); + setStats({ duration: 0, toolCallCount: 0, iterationCount: 0 }); } finally { + const captured = stopCapture(); + setConsoleEntries(captured); setRunning(false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.tsx` around lines 39 - 71, The current handleRun calls stopCapture() only in try/catch and never persists captured console entries or error stats on failure; refactor handleRun so stopCapture() is invoked exactly once in the finally block (e.g., declare a let captured variable outside try/catch, call captured = stopCapture() in finally) and then call setConsoleEntries(captured) in that finally; keep success-path stats assignment inside the try (using execResult.stats) and set a fallback stats object in the catch (e.g., duration:0, toolCallCount:0, iterationCount:0) via setStats inside catch so errors still show stats, and ensure setResult is left as-is for success and error flows (functions to locate: handleRun, startCapture, stopCapture, setConsoleEntries, setResult, setStats).libs/browser/e2e/fixtures/debug-outer.html-219-223 (1)
219-223:⚠️ Potential issue | 🟡 MinorAdd requestId and event.source validation to message listener.
The incoming message listener accepts any
__enclave_msg__payload without validatingdata.requestIdorevent.source. While outgoing messages includerequestId, the listener should verify it to prevent cross-frame spoofing in concurrent test runs.Suggested fix
window.addEventListener('message', function (event) { var data = event.data; if (!data || data.__enclave_msg__ !== true) return; + if (data.requestId !== requestId) return; + var fromParent = event.source === window.parent; + var fromInner = innerFrame && event.source === innerFrame.contentWindow; + if (!fromParent && !fromInner) return; if (completed) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/fixtures/debug-outer.html` around lines 219 - 223, The message handler currently only checks data.__enclave_msg__ and completed; update the listener (the anonymous callback passed to window.addEventListener('message')) to also validate that data.requestId exists and matches the expected requestId value used for this exchange and that event.source is the expected source (e.g. the iframe's contentWindow or the saved source reference used when posting the message) before proceeding; if either check fails, return early to prevent cross-frame spoofing. Ensure you reference/compare data.requestId and event.source inside the same callback where var data = event.data is defined.apps/browser-demo/scripts/buffer-shim.mjs-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorRemove the
_prefix from_encoder— it is not an unused variable.Per coding guidelines,
_prefix is reserved for unused parameters and variables._encoderis actively used on line 10.🐛 Proposed fix
-const _encoder = new TextEncoder(); +const encoder = new TextEncoder(); if (typeof globalThis.Buffer === 'undefined') { globalThis.Buffer = { byteLength(str) { - return _encoder.encode(String(str)).byteLength; + return encoder.encode(String(str)).byteLength; }, }; }As per coding guidelines: "unused parameters and variables prefixed with underscore
_".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/scripts/buffer-shim.mjs` at line 5, Rename the variable _encoder to encoder everywhere it's declared and used (change the const named _encoder in the top-level of buffer-shim.mjs and any subsequent uses, e.g., the call on line 10) so it no longer uses the underscore prefix reserved for unused identifiers; update any references to _encoder to use encoder to keep naming consistent with coding guidelines.libs/browser/src/index.ts-15-67 (1)
15-67:⚠️ Potential issue | 🟡 MinorAdd browser library documentation.
The@enclave-vm/browserlibrary exports a public API (BrowserEnclave, IframeAdapter, IframeExecutionContext, DoubleIframeConfig, and protocol types) but has no corresponding documentation under docs/enclave/. Create an API reference file (e.g., docs/enclave/api-reference/browser.mdx or a core-libraries section) documenting the browser API, its configuration options, and usage examples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/index.ts` around lines 15 - 67, The browser package lacks API docs—add a new MDX API reference (e.g., docs/enclave/api-reference/browser.mdx) that documents the exported public API: BrowserEnclave (constructor options and BrowserEnclaveOptions fields, SecurityLevel and SecurityLevelConfig usage), IframeAdapter and IframeExecutionContext (when/how to use the iframe adapter), DoubleIframeConfig and DEFAULT_DOUBLE_IFRAME_CONFIG, the protocol types (HostToOuterMessage, ToolCallMessage, ResultMessage, etc.), SECURITY_LEVEL_CONFIGS, and utilities like utf8ByteLength; include concise usage examples showing how to instantiate BrowserEnclave with common options, configure security levels/DoubleIframeConfig, and how to wire up IframeAdapter/iframe protocol messages, plus links to relevant types for deeper reference.libs/browser/src/adapters/iframe-adapter.ts-207-212 (1)
207-212:⚠️ Potential issue | 🟡 MinorWhitelist console levels before dynamic dispatch.
data.levelis untrusted; theisConsoleMessagecheck only validates the message type, not the level value. AlthoughconsoleMessageSchemaenforces the allowed levels, it is never applied to validate incoming messages. The TypeScript type assertion provides no runtime protection. Explicitly validate the level before callingconsole[method]().🛡️ Suggested fix
- const method = data.level as 'log' | 'warn' | 'error' | 'info'; - if (typeof console[method] === 'function') { - console[method]('[Enclave]', ...data.args); - } + const level = data.level; + if (level === 'log' || level === 'warn' || level === 'error' || level === 'info') { + console[level]('[Enclave]', ...data.args); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/iframe-adapter.ts` around lines 207 - 212, The code currently casts data.level and calls console[method] dynamically which is unsafe; update the message handling in the block that checks isConsoleMessage(data) to explicitly whitelist and validate the level before dispatching: either run consoleMessageSchema.parse/validate on the incoming message (use the same schema used elsewhere) or check data.level against an explicit allowedLevels set (e.g., ['log','warn','error','info']) and only then call the corresponding console method (or use a switch on data.level). Ensure you reference and use isConsoleMessage and the consoleMessageSchema validation path so untrusted levels are rejected before any console[method] access.libs/browser/src/adapters/outer-iframe-bootstrap.ts-139-152 (1)
139-152:⚠️ Potential issue | 🟡 MinorReset RegExp state before
.test()to avoid global/sticky flag drift.If a caller provides
goryflags in the allowed/blocked operation patterns,.test()becomes stateful and may yield false negatives on subsequent validations. ResetlastIndexbefore each test call.Suggested fix
// Whitelist check if (validationConfig.validateOperationNames && typeof allowedOperationPattern !== 'undefined') { + allowedOperationPattern.lastIndex = 0; if (!allowedOperationPattern.test(operationName)) { throw createSafeError('Operation "' + operationName + '" does not match allowed pattern'); } } // Blacklist check if (typeof blockedOperationPatterns !== 'undefined') { for (var i = 0; i < blockedOperationPatterns.length; i++) { + blockedOperationPatterns[i].lastIndex = 0; if (blockedOperationPatterns[i].test(operationName)) { throw createSafeError('Operation "' + operationName + '" matches blocked pattern'); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts` around lines 139 - 152, The regexp tests are stateful if patterns have g/y flags; before calling allowedOperationPattern.test(operationName) and before each blockedOperationPatterns[i].test(operationName) reset the RegExp state by setting allowedOperationPattern.lastIndex = 0 and blockedOperationPatterns[i].lastIndex = 0 respectively (only after the existing undefined checks), then perform the test and throw the same createSafeError messages on match/fail; update the checks around validationConfig, allowedOperationPattern, blockedOperationPatterns, operationName and createSafeError accordingly.
🧹 Nitpick comments (18)
apps/browser-demo/src/enclave-loader.ts (1)
7-13: Concurrent calls can trigger duplicate imports.If
loadEnclaveModule()is called multiple times before the firstimport()resolves, each call seescached === nulland fires a separate dynamic import. Caching the promise instead of the resolved value eliminates this.♻️ Proposed fix — cache the promise
-// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -let cached: any = null; +// eslint-disable-next-line `@typescript-eslint/no-explicit-any` +let cached: Promise<any> | null = null; export async function loadEnclaveModule() { - if (cached) return cached; - cached = await import('../vendor/enclave-browser-bundle.mjs'); + if (!cached) { + cached = import('../vendor/enclave-browser-bundle.mjs'); + } return cached; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/enclave-loader.ts` around lines 7 - 13, The current loadEnclaveModule function uses a plain value cache so concurrent calls can trigger duplicate dynamic imports; change the cached variable to hold the import Promise (e.g., Promise<any> | null) and in loadEnclaveModule set cached = import('../vendor/enclave-browser-bundle.mjs') when cached is null, then await and return cached; also consider clearing cached on a rejection to allow retries. Ensure you update the cached declaration and the loadEnclaveModule function to treat cached as the promise returned by import.libs/browser/project.json (1)
1-51: New publishable library (libs/browser) has no accompanying documentation.As a
scope:publishablelibrary, public API additions should have matching documentation underdocs/enclave/**. Consider adding initial API docs — even a stub — as part of this PR or a follow-up. As per coding guidelines, "When public APIs change, ensure there is a matching documentation update under docs/enclave/**."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/project.json` around lines 1 - 51, The new publishable library "libs/browser" is tagged scope:publishable but lacks docs; add a matching documentation file under docs/enclave (e.g., an initial API overview and usage stub) that documents the public surface exported by libs/browser (refer to its public entry "libs/browser/src/index.ts" and package/dist output used by the build/ nx-release-publish target) so future consumers and reviewers can find API details; include at least a basic README or MD under docs/enclave/browser that covers exported functions/classes, installation, and a minimal example.apps/browser-demo/src/components/CodeEditor.tsx (1)
10-18:<label>is not programmatically associated with the<textarea>.Without an
htmlFor/idpair, screen readers won't link the label to the textarea. Minor for a demo, but easy to fix.♿ Proposed fix
- <label className="section-label">Code</label> + <label className="section-label" htmlFor="code-editor">Code</label> <textarea + id="code-editor" className="code-textarea"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/components/CodeEditor.tsx` around lines 10 - 18, The label in the CodeEditor component isn't programmatically linked to the textarea; add an id for the textarea and set the label's htmlFor to that id so screen readers associate them. Update the CodeEditor component to either accept an id prop or generate one (e.g., React's useId) and apply it to the <textarea> and label (label htmlFor={id}, textarea id={id}); keep existing props (value, onChange, disabled) unchanged.apps/browser-demo/src/types.ts (1)
25-29: Align ExecutionStats with shared browser types to avoid drift.The shared type includes
startTime/endTime(libs/browser/src/types.ts). Consider adding them (optionally) for consistency.🔧 Optional alignment
export interface ExecutionStats { duration: number; toolCallCount: number; iterationCount: number; + startTime?: number; + endTime?: number; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/types.ts` around lines 25 - 29, The ExecutionStats interface currently only has duration, toolCallCount, and iterationCount; to align with the shared browser types (libs/browser/src/types.ts) add optional startTime and endTime fields to ExecutionStats so consumers can use timestamps without breaking existing code—update the interface named ExecutionStats in apps/browser-demo/src/types.ts to include startTime?: number and endTime?: number (or the appropriate timestamp type used in the shared types) and run type checks to ensure compatibility.apps/browser-demo/src/components/ResultDisplay.tsx (1)
1-4: Prefer a typed result union instead ofany.This removes the eslint override and makes the component safer to evolve.
♻️ Suggested refactor
+type ResultError = { name?: string; message?: string }; +type ResultPayload = + | { success: true; value: unknown } + | { success: false; error?: ResultError }; + interface ResultDisplayProps { - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - result: any | null; + result: ResultPayload | null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/components/ResultDisplay.tsx` around lines 1 - 4, Replace the brittle any in ResultDisplayProps with a proper discriminated union type (e.g., define ResultData = SuccessResult | ErrorResult | null or similar) and remove the eslint-disable-next-line; update the ResultDisplayProps to use result: ResultData and adjust the ResultDisplay component to narrow on the discriminant fields (or instanceof checks) so all branches are typed safely (reference symbols: ResultDisplayProps, result, ResultDisplay).libs/browser/src/adapters/inner-iframe-bootstrap.ts (2)
657-662:generateUserCodeExecutionis a pass-through — consider documenting the security invariant.This function returns
userCodeunchanged, relying entirely on upstream AST transformation/validation to ensure safety. The comment mentions this, but there's no runtime guard. At minimum, consider asserting the code doesn't contain</script>here as a defense-in-depth measure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts` around lines 657 - 662, The function generateUserCodeExecution currently returns userCode unchanged relying only on upstream AST validation; add a defense-in-depth runtime check inside generateUserCodeExecution to assert or sanitize that userCode does not contain closing script tags (e.g., reject or escape any occurrences of "</script>") and optionally log/throw a clear error if the invariant is violated so callers cannot accidentally inject script delimiters despite upstream checks.
519-527:_definePropertyis saved after functions that reference it are declared.
_definePropertyis assigned at line 520, butcreateSafeError(line 58) references it. This works today only becausecreateSafeErroris never invoked before line 520 executes. However,var _definePropertyis hoisted asundefined, so any future change that callscreateSafeErrorduring the setup phase (before line 520) would silently fail inside thetry/catchon line 62.Consider moving the
_definePropertysave to the top of the IIFE, right after the initial variable declarations (around line 50), for robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts` around lines 519 - 527, The helper variable _defineProperty is captured after functions like createSafeError are declared (createSafeError references _defineProperty), which risks createSafeError seeing undefined if invoked earlier; fix by moving the assignment of _defineProperty = Object.defineProperty to the top of the IIFE (immediately after initial var declarations, before createSafeError and other functions are defined) so createSafeError and any early callers always see the real defineProperty reference even if Object is later neutralized.apps/browser-demo/src/hooks/use-console-capture.ts (1)
6-6: Module-levelnextIdis shared across all hook instances and never resets.This is fine for unique ID generation in a demo app, but worth noting it will monotonically increase across the app lifetime. If you ever need deterministic IDs (e.g., for tests), consider scoping it inside the hook via a ref.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/hooks/use-console-capture.ts` at line 6, nextId is declared at module scope so it increments globally across all instances of the hook; to scope IDs per-hook (deterministic for tests) move the counter inside the useConsoleCapture hook using a React ref (e.g., const idRef = useRef(0)) or initialize with useRef(() => nextValue) so each hook instance has its own counter that persists across renders but resets per-instance; update all places that reference nextId to use idRef.current and increment idRef.current as needed, and remove or deprecate the module-level nextId variable.apps/browser-demo/src/hooks/use-enclave.ts (1)
19-53: Race condition between concurrentinitcalls.When
securityLevelchanges rapidly, multipleinitcalls can interleave. Thedisposedflag prevents stale state updates, but the old enclave disposal on line 28–35 runs inside the new effect invocation, not the cleanup of the previous one. If the asyncloadEnclaveModulefrom the previous run completes after the new one starts, the previous effect'sdisposedflag is set, but its enclave instance (assigned on line 41) might complete construction and get assigned toenclaveRef.currentbefore the new init disposes the old one.The cleanup function (line 57–67) mitigates this since React calls it before re-running the effect, but the in-effect disposal on lines 28–35 is redundant and could race with the cleanup. Consider removing the in-effect disposal and relying solely on the cleanup function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/hooks/use-enclave.ts` around lines 19 - 53, The in-effect disposal inside init is causing a race with concurrent init calls; remove the block that disposes enclaveRef.current (the try/catch that calls enclaveRef.current.dispose() and sets enclaveRef.current = null) from inside the async init function and rely solely on the effect's cleanup to dispose previous instances; keep the disposed flag, the loadEnclaveModule call, assignment to enclaveRef.current = new mod.BrowserEnclave(...), and the cleanup code that disposes and nulls enclaveRef.current so all disposal happens deterministically in the effect cleanup rather than during init.libs/browser/e2e/fixtures/debug-outer.html (1)
1-12: Please include problem/solution, test evidence, and security-impact notes in the PR description.This PR touches sandbox/VM behavior, so the required metadata should be recorded.
Based on learnings: PRs must include problem/solution description, test evidence (
yarn testornx test <project>), and security-impact notes for sandbox/VM changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/fixtures/debug-outer.html` around lines 1 - 12, Update the PR description to include three required sections for sandbox/VM changes: a brief "Problem" describing what behavior or bug this patch addresses (reference the affected file debug-outer.html / outer sandbox behavior), a "Solution" describing what you changed (what the patch does to the sandbox/VM), and "Security impact" notes describing any risks or mitigations for sandbox/VM behavior; also add test evidence lines showing the actual command(s) run and their results (e.g., `yarn test` or `nx test <project>` with pass/fail summary) so reviewers can reproduce the verification.libs/browser/src/types.ts (1)
226-298:doubleIframetyping prevents partial overrides ofparentValidation.
BrowserEnclaveOptions.doubleIframeusesPartial<DoubleIframeConfig>, but becauseparentValidationis a required field inDoubleIframeConfig, callers cannot partially override it. The runtime code supports merging partialparentValidationvalues via defaults, but the types don't reflect this. Allow nested partial overrides to improve API ergonomics.♻️ Suggested type adjustment
export interface BrowserEnclaveOptions { /** * Double iframe configuration */ - doubleIframe?: Partial<DoubleIframeConfig>; + doubleIframe?: Omit<Partial<DoubleIframeConfig>, 'parentValidation'> & { + parentValidation?: Partial<ParentValidationConfig>; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/types.ts` around lines 226 - 298, BrowserEnclaveOptions.doubleIframe is currently typed as Partial<DoubleIframeConfig> which still forces the required parentValidation fields from DoubleIframeConfig; update the typing so callers can supply nested partials for parentValidation (e.g., use a deep-partial utility or explicitly make parentValidation?: Partial<DoubleIframeConfig['parentValidation']>) so the type matches runtime merging behavior; target the BrowserEnclaveOptions interface and the doubleIframe property and ensure DoubleIframeConfig.parentValidation is allowed to be partially provided by callers.libs/browser/e2e/build-test-bundle.mjs (1)
14-14:rootpath derivation is fragile if this script moves.
path.resolve(__dirname, '../../..')hardcodes the assumption that this script lives exactly 3 levels below the monorepo root. If the script is relocated, the alias on line 27 will silently resolve to a non-existent path and produce an opaque esbuild error.Consider resolving the root via a manifest anchor (e.g.,
package.jsonwalk-up) or at minimum add a guard:const root = path.resolve(__dirname, '../../..'); +// Sanity-check: ensure the resolved root contains a workspace manifest. +import { existsSync } from 'node:fs'; +if (!existsSync(path.resolve(root, 'package.json'))) { + throw new Error(`Monorepo root not found at: ${root}`); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/build-test-bundle.mjs` at line 14, The root path derivation using the root constant (const root = path.resolve(__dirname, '../../..')) is brittle if the script is moved; replace it with a robust lookup (walk up parent directories until a manifest like package.json is found) or at minimum validate the computed root before using it for the esbuild alias: check fs.existsSync(path.join(root, 'package.json')) (or the aliased target) and throw or log a clear error if missing. Update any use of root for the alias resolution to rely on the discovered manifest path and surface a helpful message explaining the expected repository root when validation fails.libs/browser/src/utils/utf8-byte-length.ts (1)
10-19: ConsiderencodeIntoto avoid per-call heap allocation.
encoder.encode(value)allocates a newUint8Arrayon every invocation purely to read.byteLength. If this utility sits on a hot path (e.g., called per-message in the iframe protocol), it generates unnecessary GC pressure. A pre-allocated scratch buffer withencodeIntoavoids the allocation entirely.♻️ Optional: zero-allocation approach with encodeInto
const encoder = new TextEncoder(); +// Pre-allocate a scratch buffer large enough for typical payloads. +// encodeInto writes up to buffer.length bytes and returns { written }. +const _scratch = new Uint8Array(4096); export function utf8ByteLength(value: string): number { - return encoder.encode(value).byteLength; + if (value.length * 3 <= _scratch.length) { + // Fast path: string fits in scratch (worst case 3 bytes/char for BMP). + return encoder.encodeInto(value, _scratch).written; + } + // Slow path: fall back to full allocation for large strings. + return encoder.encode(value).byteLength; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/utils/utf8-byte-length.ts` around lines 10 - 19, The current utf8ByteLength implementation calls encoder.encode(value) which allocates a new Uint8Array per call; replace this with a reusable scratch buffer and TextEncoder.encodeInto to avoid per-call heap allocations: create a module-scoped scratch Uint8Array (e.g., sized conservatively like value.length * 4) and call encoder.encodeInto(value, scratch) inside utf8ByteLength to read the written byteCount; if encodeInto reports buffer overflow, grow the scratch buffer (allocate a larger Uint8Array) and retry or fall back to encoder.encode once as a safety net. Ensure you keep the existing encoder constant and only mutate/replace the scratch buffer as needed so subsequent calls reuse it.apps/browser-demo/scripts/buffer-shim.mjs (1)
1-13: Duplicate oflibs/browser/e2e/buffer-shim.mjs— extract to a shared location.The content of
apps/browser-demo/scripts/buffer-shim.mjsandlibs/browser/e2e/buffer-shim.mjsis identical. Any future fix (e.g., toString(str)coercion or encoding logic) will need to be applied in two places.Consider placing a single canonical shim (e.g., under
libs/browser/src/utils/or a sharedscripts/directory) and symlinking or importing it from both locations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/scripts/buffer-shim.mjs` around lines 1 - 13, Duplicate buffer shim logic (globalThis.Buffer with byteLength using _encoder/TextEncoder) exists in both buffer-shim.mjs copies; extract this into a single canonical module (e.g., a shared buffer-shim.mjs) and update the two original modules to import or re-export that canonical shim instead of duplicating code; ensure the exported behavior preserves globalThis.Buffer, byteLength, _encoder usage and String(str) coercion so future fixes apply once.libs/browser/e2e/helpers.ts (1)
29-43:dispose()is skipped ifenclave.run()rejects.The
.then()chain only callsdispose()on success. Ifrun()throws, the enclave (and its iframes) leak for the remainder of the test. The same pattern exists inrunWithToolHandler(line 66-68). For e2e tests the page teardown usually cleans up, but adding a.finally()would be more defensive.♻️ Suggested fix
export async function runInEnclave(page: Page, code: string, opts: RunOptions = {}) { return page.evaluate( ({ code, opts }) => { // eslint-disable-next-line `@typescript-eslint/no-explicit-any` const EB = (window as any).EnclaveBrowser; const enclave = new EB.BrowserEnclave(opts); // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - return enclave.run(code).then((result: any) => { - enclave.dispose(); - return result; - }); + return enclave.run(code).then( + (result: any) => { enclave.dispose(); return result; }, + (err: any) => { enclave.dispose(); throw err; }, + ); }, { code, opts }, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/helpers.ts` around lines 29 - 43, The enclave.dispose() call is only invoked on success; update runInEnclave (and the similar runWithToolHandler) so the enclave is always disposed even if enclave.run() rejects: inside the page.evaluate closure create the EB.BrowserEnclave as before, then ensure enclave.dispose() is invoked in a finally block (or via a .finally() on the promise) so the result is returned on success and the rejection is propagated after disposal; reference the enclave variable inside the evaluate callback to perform the guaranteed cleanup.libs/browser/playwright.config.ts (1)
4-7:__dirnameworks here because Playwright's config loader uses CJS-compatible transforms, but note the inconsistency.The companion
.mjsbuild scripts (e.g.,build-test-bundle.mjs,build-enclave-bundle.mjs) use the ESM-safepath.dirname(fileURLToPath(import.meta.url))pattern. This.tsfile relies on__dirnamebeing injected by Playwright's runner. This is fine today, but if the project's TS config changes to ESM module resolution, it would break here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/playwright.config.ts` around lines 4 - 7, The config uses __dirname (in testDir and globalSetup) which is CJS-only; replace it with an ESM-safe dirname obtained via fileURLToPath(import.meta.url) (import fileURLToPath from 'url' and compute const __dirname = path.dirname(fileURLToPath(import.meta.url')) or a similarly named constant) and update calls in defineConfig (testDir, globalSetup, any other path.resolve(__dirname, ...)) to use that value so the config continues to work if TS/Module resolution changes to ESM; keep the symbol references: defineConfig, testDir, globalSetup, and the path.resolve(... ) usages.libs/browser/e2e/console-capture.spec.ts (2)
15-21: Repeated inline enclave creation — consider extracting a helper or reusingrunInEnclave.The pattern of
new EB.BrowserEnclave(…) → enclave.run(…) → enclave.dispose()is duplicated three times in this file and mirrors therunInEnclavehelper. I understand the console listener must be attached before execution, but a thin wrapper that returns the enclave result while letting the caller set up listeners first would reduce boilerplate.Also applies to: 35-41, 54-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/console-capture.spec.ts` around lines 15 - 21, Extract a small helper in this test file (e.g., openBrowserEnclave or getBrowserEnclave) that encapsulates new EB.BrowserEnclave({ timeout: 5000 }) and returns the enclave instance plus a convenience runAndDispose(resultFn) or simply the enclave so callers can attach console listeners before calling enclave.run, then call enclave.dispose() in a finally block; replace the three inline patterns of new EB.BrowserEnclave(...) → enclave.run(...) → enclave.dispose() with calls to this helper and use runInEnclave when appropriate to reduce duplication while preserving the ability to set up listeners before execution (refer to BrowserEnclave, enclave.run, enclave.dispose, and runInEnclave).
9-27:page.waitForTimeout(200)is a flaky wait pattern — consider polling instead.Fixed delays are fragile across different CI environments and hardware. This pattern repeats across all four tests (lines 24, 43, 62, 90). A polling approach such as
page.waitForFunctionthat checks for the expected console message would be more robust.♻️ Example replacement for the fixed delay
- // Wait briefly for postMessage relay to reach host console - await page.waitForTimeout(200); - expect(result.success).toBe(true); - expect(logs.some((l) => l.includes('hello from enclave'))).toBe(true); + expect(result.success).toBe(true); + // Poll until the relayed message arrives (or timeout) + await expect + .poll(() => logs.some((l) => l.includes('hello from enclave')), { timeout: 2000 }) + .toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/console-capture.spec.ts` around lines 9 - 27, The test uses a flaky fixed delay (page.waitForTimeout(200)) to wait for the enclave console message; replace that with an event-based wait that polls for the specific console message. In the 'console.log relayed to host' test, remove the page.waitForTimeout call and use await page.waitForEvent('console', { predicate: (msg) => msg.type() === 'log' && msg.text().includes('hello from enclave') }); and apply the same change to the other three tests (they currently use page.waitForTimeout) so they each wait for the expected console event rather than sleeping.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (61)
.gitignoreapps/browser-demo/.gitignoreapps/browser-demo/eslint.config.mjsapps/browser-demo/index.htmlapps/browser-demo/package.jsonapps/browser-demo/project.jsonapps/browser-demo/scripts/buffer-shim.mjsapps/browser-demo/scripts/build-enclave-bundle.mjsapps/browser-demo/src/App.cssapps/browser-demo/src/App.tsxapps/browser-demo/src/components/CodeEditor.tsxapps/browser-demo/src/components/ConsoleOutput.tsxapps/browser-demo/src/components/ExampleSnippets.tsxapps/browser-demo/src/components/ExecutionControls.tsxapps/browser-demo/src/components/ResultDisplay.tsxapps/browser-demo/src/components/SecurityLevelPicker.tsxapps/browser-demo/src/components/StatsPanel.tsxapps/browser-demo/src/components/ToolHandlerConfig.tsxapps/browser-demo/src/data/examples.tsapps/browser-demo/src/data/mock-tools.tsapps/browser-demo/src/enclave-loader.tsapps/browser-demo/src/hooks/use-console-capture.tsapps/browser-demo/src/hooks/use-enclave.tsapps/browser-demo/src/main.tsxapps/browser-demo/src/types.tsapps/browser-demo/tsconfig.app.jsonapps/browser-demo/tsconfig.jsonapps/browser-demo/vite.config.tseslint.config.mjslibs/browser/e2e/buffer-shim.mjslibs/browser/e2e/build-test-bundle.mjslibs/browser/e2e/console-capture.spec.tslibs/browser/e2e/execution.spec.tslibs/browser/e2e/fixtures/debug-inner.htmllibs/browser/e2e/fixtures/debug-inner2.htmllibs/browser/e2e/fixtures/debug-outer.htmllibs/browser/e2e/fixtures/serve.jsonlibs/browser/e2e/fixtures/test-harness.htmllibs/browser/e2e/global-setup.tslibs/browser/e2e/helpers.tslibs/browser/e2e/protocol.spec.tslibs/browser/e2e/security-isolation.spec.tslibs/browser/e2e/timeout.spec.tslibs/browser/e2e/tool-calls.spec.tslibs/browser/e2e/validation.spec.tslibs/browser/package.jsonlibs/browser/playwright.config.tslibs/browser/project.jsonlibs/browser/src/adapters/iframe-adapter.tslibs/browser/src/adapters/iframe-html-builder.tslibs/browser/src/adapters/iframe-protocol.tslibs/browser/src/adapters/inner-iframe-bootstrap.tslibs/browser/src/adapters/outer-iframe-bootstrap.tslibs/browser/src/browser-enclave.tslibs/browser/src/index.tslibs/browser/src/types.tslibs/browser/src/utils/utf8-byte-length.tslibs/browser/tsconfig.jsonlibs/browser/tsconfig.lib.jsonpackage.jsontsconfig.base.json
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
libs/browser/src/adapters/iframe-adapter.ts (1)
112-116:⚠️ Potential issue | 🟠 MajorValidate
event.sourceto prevent spoofed enclave messages.The handler accepts
__enclave_msg__from any window. A same-origin attacker or unrelated iframe could inject messages matching therequestId. Checkevent.sourceagainst the iframe'scontentWindow.Suggested fix
messageHandler = (event: MessageEvent) => { + if (event.source !== this.outerIframe?.contentWindow) return; const data = event.data; if (!isEnclaveMessage(data)) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/iframe-adapter.ts` around lines 112 - 116, The message handler currently trusts any MessageEvent whose data passes isEnclaveMessage; update messageHandler to also validate event.source strictly equals the enclave iframe's contentWindow (e.g., compare event.source === iframe.contentWindow) and skip processing if it doesn't match, handling null/undefined safely; keep the existing isEnclaveMessage check and failure early-return behavior to prevent spoofed enclave messages.
🧹 Nitpick comments (7)
apps/browser-demo/src/App.tsx (2)
20-21: Consider replacinganywith a typed union forresult.The eslint-disable suppresses the warning, but a discriminated union (success path vs. error path) already exists in the codebase via
ExecutionResult/types.ts. Typingresultproperly eliminates theanyand makes downstream rendering inResultDisplaytype-safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.tsx` around lines 20 - 21, The state variable result is typed as any; replace it with the existing discriminated union ExecutionResult (from types.ts) to remove the eslint-disable and provide type safety for rendering in ResultDisplay. Update the useState declaration for result / setResult to use ExecutionResult | null (or the appropriate union variant) and ensure any places that setResult currently pass raw values conform to the ExecutionResult shape so TypeScript can validate the success vs error branches.
57-66: Duplicate zero-stats literal in thecatchblock.The same
{ duration: 0, toolCallCount: 0, iterationCount: 0 }object is written twice — once embedded inside the error result (Line 64) and again passed tosetStats(Line 66). Extract it to a constant to keep both in sync if the shape ever changes.♻️ Proposed refactor
} catch (err) { + const zeroStats = { duration: 0, toolCallCount: 0, iterationCount: 0 }; setResult({ success: false, error: { name: 'AppError', message: err instanceof Error ? err.message : String(err), }, - stats: { duration: 0, toolCallCount: 0, iterationCount: 0 }, + stats: zeroStats, }); - setStats({ duration: 0, toolCallCount: 0, iterationCount: 0 }); + setStats(zeroStats);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.tsx` around lines 57 - 66, Extract the repeated zero-stats object into a single constant and use it in both places inside the catch block so they stay in sync; e.g., define a const (e.g. zeroStats) with { duration: 0, toolCallCount: 0, iterationCount: 0 } and then pass zeroStats into setResult(...) (inside the error.stats field) and into setStats(zeroStats); update the catch block around the setResult and setStats calls to reference that constant and ensure the constant is in scope for both calls.libs/browser/src/adapters/iframe-adapter.ts (2)
283-291:postMessage(msg, '*')— consider scoping the target origin.Since the outer iframe is loaded via
srcdoc, its origin isnull, so'*'is technically required here. Add a comment explaining this to prevent future reviewers from flagging it, and consider re-evaluating if the loading strategy changes (e.g., to blob URLs with a known origin).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/iframe-adapter.ts` around lines 283 - 291, In sendToOuter, the postMessage call uses '*' because the outer iframe is loaded via srcdoc and therefore has a null origin; update the sendToOuter implementation to include an inline comment next to outerIframe.contentWindow.postMessage(msg, '*') that explains the srcdoc/null-origin rationale and warns reviewers to re-evaluate the targetOrigin if the loading strategy changes (e.g., switching to blob URLs or a known origin), so future maintainers don't replace '*' without considering origin implications.
155-156: Unsafeerror as Errorcast — non-Error throwables will produceundefinedfields.If the
toolHandlerthrows a non-Error value (e.g., a string),err.nameanderr.messagewill beundefined, relying on the||fallbacks. This works but consider a more defensive approach:- } catch (error: unknown) { - const err = error as Error; + } catch (error: unknown) { + const err = error instanceof Error ? error : new Error(String(error));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/iframe-adapter.ts` around lines 155 - 156, The catch block in iframe-adapter.ts currently does a blind cast `const err = error as Error`, which yields undefined name/message for non-Error throwables; update the catch handling around the `toolHandler` call to defensively normalize the throwable: if error is an Error use it, if it's a string or primitive convert to a new Error(String(error)), and if it's an object without name/message build a descriptive Error (including JSON.stringify when safe); then use that normalized error's name/message/stack in the existing logging/response paths so non-Error throws no longer produce undefined fields.libs/browser/src/adapters/outer-iframe-bootstrap.ts (3)
290-319: Timeout fires even aftercompleted = true— benign but wasteful.The
setTimeouton line 294 is never cleared when execution completes normally (completed = trueon line 238 guards the body). This is safe, but if you want clean resource management, store the timer ID and clear it whencompletedis set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts` around lines 290 - 319, The timeout created with setTimeout (variable timeout) is never cleared after normal completion (when completed is set true), so store the timer id (e.g., timerId = setTimeout(...)) and call clearTimeout(timerId) wherever the code marks completion (the existing place that sets completed = true and tears down innerFrame / calls sendToHost); also clear the timer on any other early-exit completion paths to avoid the timeout handler running unnecessarily. Ensure references are to timeout, setTimeout, completed, innerFrame, and sendToHost so the timer is cleared in the same completion logic.
120-126:operationHistory.shift()in a loop is O(n²) in the worst case.Each
shift()is O(n) on the remaining array. Under sustained load, this can become quadratic. Given the rate limiter caps throughput, this is unlikely to be a practical issue, but a circular buffer or index-based cleanup would be more efficient if the rate limit is ever raised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts` around lines 120 - 126, The loop in validateOperation uses operationHistory.shift() repeatedly, causing O(n²) behavior; replace shifting with an index-based sliding window: add a head pointer (e.g., operationHistoryHead) and advance it in the while loop (while head < operationHistory.length && now - operationHistory[head].timestamp > 2000) head++; use operationHistory.slice(head) or treat the window as operationHistory[head..] when counting/iterating, and periodically compact the array (e.g., when head grows beyond a threshold, set operationHistory = operationHistory.slice(head) and reset head = 0) so operations in validateOperation and any code referencing operationHistory use the head-aware view.
91-106: Regex patterns from config could trigger ReDoS inside the sandbox.
allowedOperationPatternSourceandblockedOperationPatternSourcesare compiled intoRegExpobjects. If adversarial or poorly crafted, they can cause catastrophic backtracking. Since this runs in a sandboxed iframe with a timeout, the blast radius is limited to that iframe stalling until the timeout fires, but it could degrade user experience. Consider validating pattern complexity or documenting the risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts` around lines 91 - 106, The injected regex creation for validationConfig.allowedOperationPatternSource and validationConfig.blockedOperationPatternSources can cause ReDoS; before calling new RegExp (and when building blockedOperationPatterns), validate each source string (allowedOperationPatternSource and each entry in blockedOperationPatternSources) using a safety check (e.g., the safe-regex algorithm or a heuristic: max length, ban nested quantifiers like (.+)+, catastrophic constructs) and wrap RegExp construction in try/catch; if a pattern is unsafe or construction fails, log/emit a warning and skip or replace with a conservative safe fallback (e.g., a literal-safe pattern or undefined) so the sandbox won’t compile adversarial expressions. Ensure you reference validationConfig.allowedOperationPatternSource, validationConfig.blockedOperationPatternSources, validationConfig.blockedOperationPatternFlags, and the variables allowedOperationPattern / blockedOperationPatterns when applying these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/browser-demo/src/App.tsx`:
- Around line 46-47: The call to startCapture() is currently outside the try
block so if it throws we never hit the finally and stopCapture() won't run; move
the startCapture() invocation inside the try that already contains the rest of
the capture logic so that any exception still triggers the finally block and
calls stopCapture(); update the block around
useConsoleCapture/startCapture/stopCapture to ensure startCapture() is executed
after entering the try and that stopCapture() remains in the finally.
In `@apps/browser-demo/src/components/ExecutionControls.tsx`:
- Line 12: The status variable conflates real loading and pre-init states;
change the ternary that sets status (the line defining const status) to return
'loading' when enclaveLoading is true, 'error' when enclaveError is true,
'ready' when ready is true, and 'initializing' as the final fallback (instead of
'loading') so CSS can target 'initializing' vs 'loading'; update any usage of
status (e.g., className assignments or conditionals in ExecutionControls.tsx) to
handle the new 'initializing' token and adjust styles accordingly.
In `@libs/browser/src/adapters/iframe-adapter.ts`:
- Around line 207-213: The current console relay in iframe-adapter.ts uses a
dynamic console method with a type assertion that defeats TypeScript narrowing
(see isConsoleMessage, requestId, allowedLevels, data.level), which triggers
CodeQL; replace the dynamic dispatch with a safe explicit mapping or a switch:
first validate data.level against allowedLevels, then map each allowed string to
the corresponding console method (or use a switch on data.level) and call that
specific method with '[Enclave]' and ...data.args to avoid dynamic property
access and remove the unsafe type assertion.
- Around line 60-62: The current design uses a single outerIframe field which
gets overwritten if IframeAdapter.execute() is called concurrently, causing
cleanup() to remove the wrong iframe and leak the original; fix by preventing
concurrent executions or tracking multiple active requests: either (a) add a
concurrency guard in execute() (e.g., reject/throw when an execution is already
in flight using the existing disposed flag or a new inFlight boolean) and clear
that guard in cleanup(), or (b) change outerIframe to a Map<requestId,
HTMLIFrameElement> (generate a unique requestId per execute() call), store each
created iframe under that id, use the same requestId in cleanup() to remove the
correct iframe from the DOM and delete the Map entry; update execute(),
cleanup(), and any helper methods to accept/propagate requestId so each
execution manages its own iframe without overwriting shared state.
- Around line 296-308: The abort() method currently removes the iframe but does
not resolve/reject the pending execute() promise for the given requestId; update
abort(requestId) to settle the pending request before tearing down the iframe by
either (a) posting a synthetic result message via sendToOuter(...) that matches
the shape the message handler expects (type: 'result', requestId and an error
indicating abort) or (b) directly invoking the stored settle callback for
requestId (the same callback used by execute()/message handler) to
reject/resolve the promise, then proceed to remove outerIframe and null it;
reference abort(requestId), execute(), the requestId key, sendToOuter,
outerIframe, and the internal settle callback storage when making the change.
In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts`:
- Around line 193-274: The message handler currently trusts any event with
__enclave_msg__ === true; tighten it by discriminating event.source: for
messages originating from the inner iframe (types 'tool-call', 'result',
'console') require event.source === innerFrame &&/or innerFrame.contentWindow
(and guard innerFrame exists), and for messages from the host (types
'tool-response', 'abort') require event.source === window.parent; implement
these checks at the top of the window.addEventListener('message',
function(event) { ... }) block before processing each type (refer to symbols
event, innerFrame, sendToHost, sendToInner, and completed/aborted) and ignore
any messages that don't match the expected source.
- Around line 327-337: generatePatternDetectors currently injects p.detectBody
directly into generated JS without validation; update it to either validate
detectBody (port validateDetectBody and DANGEROUS_BODY_PATTERNS logic from
libs/core/src/double-vm/suspicious-patterns.ts into outer-iframe-bootstrap.ts
and call it for each SerializableSuspiciousPattern before using p.detectBody) or
remove/ignore custom suspiciousPatterns from the public API so callers cannot
supply unvalidated code (also update ParentValidationConfig and
browser-enclave.ts to reflect the chosen approach), ensuring you reference
generatePatternDetectors, validateDetectBody, DANGEROUS_BODY_PATTERNS,
DEFAULT_SERIALIZED_PATTERNS, ParentValidationConfig and the custom
suspiciousPatterns handling when making the change.
---
Duplicate comments:
In `@libs/browser/src/adapters/iframe-adapter.ts`:
- Around line 112-116: The message handler currently trusts any MessageEvent
whose data passes isEnclaveMessage; update messageHandler to also validate
event.source strictly equals the enclave iframe's contentWindow (e.g., compare
event.source === iframe.contentWindow) and skip processing if it doesn't match,
handling null/undefined safely; keep the existing isEnclaveMessage check and
failure early-return behavior to prevent spoofed enclave messages.
---
Nitpick comments:
In `@apps/browser-demo/src/App.tsx`:
- Around line 20-21: The state variable result is typed as any; replace it with
the existing discriminated union ExecutionResult (from types.ts) to remove the
eslint-disable and provide type safety for rendering in ResultDisplay. Update
the useState declaration for result / setResult to use ExecutionResult | null
(or the appropriate union variant) and ensure any places that setResult
currently pass raw values conform to the ExecutionResult shape so TypeScript can
validate the success vs error branches.
- Around line 57-66: Extract the repeated zero-stats object into a single
constant and use it in both places inside the catch block so they stay in sync;
e.g., define a const (e.g. zeroStats) with { duration: 0, toolCallCount: 0,
iterationCount: 0 } and then pass zeroStats into setResult(...) (inside the
error.stats field) and into setStats(zeroStats); update the catch block around
the setResult and setStats calls to reference that constant and ensure the
constant is in scope for both calls.
In `@libs/browser/src/adapters/iframe-adapter.ts`:
- Around line 283-291: In sendToOuter, the postMessage call uses '*' because the
outer iframe is loaded via srcdoc and therefore has a null origin; update the
sendToOuter implementation to include an inline comment next to
outerIframe.contentWindow.postMessage(msg, '*') that explains the
srcdoc/null-origin rationale and warns reviewers to re-evaluate the targetOrigin
if the loading strategy changes (e.g., switching to blob URLs or a known
origin), so future maintainers don't replace '*' without considering origin
implications.
- Around line 155-156: The catch block in iframe-adapter.ts currently does a
blind cast `const err = error as Error`, which yields undefined name/message for
non-Error throwables; update the catch handling around the `toolHandler` call to
defensively normalize the throwable: if error is an Error use it, if it's a
string or primitive convert to a new Error(String(error)), and if it's an object
without name/message build a descriptive Error (including JSON.stringify when
safe); then use that normalized error's name/message/stack in the existing
logging/response paths so non-Error throws no longer produce undefined fields.
In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts`:
- Around line 290-319: The timeout created with setTimeout (variable timeout) is
never cleared after normal completion (when completed is set true), so store the
timer id (e.g., timerId = setTimeout(...)) and call clearTimeout(timerId)
wherever the code marks completion (the existing place that sets completed =
true and tears down innerFrame / calls sendToHost); also clear the timer on any
other early-exit completion paths to avoid the timeout handler running
unnecessarily. Ensure references are to timeout, setTimeout, completed,
innerFrame, and sendToHost so the timer is cleared in the same completion logic.
- Around line 120-126: The loop in validateOperation uses
operationHistory.shift() repeatedly, causing O(n²) behavior; replace shifting
with an index-based sliding window: add a head pointer (e.g.,
operationHistoryHead) and advance it in the while loop (while head <
operationHistory.length && now - operationHistory[head].timestamp > 2000)
head++; use operationHistory.slice(head) or treat the window as
operationHistory[head..] when counting/iterating, and periodically compact the
array (e.g., when head grows beyond a threshold, set operationHistory =
operationHistory.slice(head) and reset head = 0) so operations in
validateOperation and any code referencing operationHistory use the head-aware
view.
- Around line 91-106: The injected regex creation for
validationConfig.allowedOperationPatternSource and
validationConfig.blockedOperationPatternSources can cause ReDoS; before calling
new RegExp (and when building blockedOperationPatterns), validate each source
string (allowedOperationPatternSource and each entry in
blockedOperationPatternSources) using a safety check (e.g., the safe-regex
algorithm or a heuristic: max length, ban nested quantifiers like (.+)+,
catastrophic constructs) and wrap RegExp construction in try/catch; if a pattern
is unsafe or construction fails, log/emit a warning and skip or replace with a
conservative safe fallback (e.g., a literal-safe pattern or undefined) so the
sandbox won’t compile adversarial expressions. Ensure you reference
validationConfig.allowedOperationPatternSource,
validationConfig.blockedOperationPatternSources,
validationConfig.blockedOperationPatternFlags, and the variables
allowedOperationPattern / blockedOperationPatterns when applying these checks.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/browser-demo/scripts/buffer-shim.mjsapps/browser-demo/src/App.cssapps/browser-demo/src/App.tsxapps/browser-demo/src/components/ExecutionControls.tsxapps/browser-demo/src/components/ToolHandlerConfig.tsxapps/browser-demo/src/enclave-loader.tsapps/browser-demo/src/hooks/use-console-capture.tslibs/browser/e2e/buffer-shim.mjslibs/browser/e2e/global-setup.tslibs/browser/e2e/helpers.tslibs/browser/src/adapters/iframe-adapter.tslibs/browser/src/adapters/outer-iframe-bootstrap.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- libs/browser/e2e/helpers.ts
- libs/browser/e2e/global-setup.ts
- apps/browser-demo/src/enclave-loader.ts
- apps/browser-demo/src/hooks/use-console-capture.ts
- apps/browser-demo/src/App.css
- libs/browser/e2e/buffer-shim.mjs
- apps/browser-demo/scripts/buffer-shim.mjs
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/browser-demo/src/App.tsx (1)
39-73: Past review concern addressed:startCapture()is now inside thetryblock.The execution flow is well-structured — guard, reset, try/catch/finally with proper cleanup. Console capture is correctly paired with
stopCapture()infinally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.tsx` around lines 39 - 73, handleRun correctly moved startCapture() into the try block and pairs stopCapture() in finally; no code changes required—leave the async handler as-is (function handleRun) ensuring startCapture runs before await run(code) and stopCapture() remains in the finally block and the dependency array still includes ready, running, code, run, startCapture, stopCapture.apps/browser-demo/src/components/ExecutionControls.tsx (1)
1-29: Clean presentational component; past review concern addressed.The
statusfallback is now correctly'initializing'(Line 12), matching the distinctstatusTextand CSS class. Logic is sound —canRunproperly guards concurrent runs and non-ready states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/components/ExecutionControls.tsx` around lines 1 - 29, The component ExecutionControls is correct (status and statusText align and canRun guards runs); remove the duplicate review marker/comment ([duplicate_comment]) from the PR and ensure no functional changes to the ExecutionControls component (specifically the status variable and statusText logic) are made—keep the existing status fallback 'initializing' and the canRun logic as implemented.
🧹 Nitpick comments (3)
apps/browser-demo/src/App.tsx (1)
20-21: Consider typingresultwith the enclave'sExecutionResultinstead ofany.The
eslint-disablefor@typescript-eslint/no-explicit-anycan be avoided by importingExecutionResultfrom the enclave library (or defining a local union type). Both the success path (line 49) and the catch-block fallback (lines 59-66) produce objects conforming to the same shape ({ success, error?, value?, stats? }). Typing this would catch shape mismatches at compile time, especially in the catch block where you construct the result manually.♻️ Example
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const [result, setResult] = useState<any>(null); + const [result, setResult] = useState<ExecutionResult | null>(null);If
ExecutionResultisn't exported from the browser-enclave library, a local type covering both branches would also work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.tsx` around lines 20 - 21, The state variable result is typed as any; replace that with the enclave's ExecutionResult (import ExecutionResult from the enclave/browser-enclave package) or define a local equivalent union type matching { success, error?, value?, stats? } and use useState<ExecutionResult | null>(null) for result and setResult; update the catch block where you construct the fallback object to produce the same ExecutionResult shape so TypeScript will validate it (refer to the result/setResult useState declaration and the error-handling block that builds the fallback result).apps/browser-demo/src/App.css (1)
130-131: Consider scopingtransitionto specific properties.
transition: all 0.15s(also on lines 240 and 301) will animate every property change, including layout properties likewidth/height, which can trigger unnecessary reflows. Narrowing to the intended properties (e.g.,background, color, border-color) is a small win. Fine for a demo, though.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.css` around lines 130 - 131, Replace the broad "transition: all 0.15s;" declarations with a scoped list of properties to avoid animating layout changes; locate each "transition: all 0.15s;" occurrence and change it to something like "transition: background-color 0.15s, color 0.15s, border-color 0.15s, transform 0.15s" (or the minimal set of properties your component actually animates) so only visual properties are animated.libs/browser/src/adapters/outer-iframe-bootstrap.ts (1)
91-106: No complexity or length limit on host-provided RegExp patterns — latent ReDoS surface.
allowedOperationPatternSourceand each entry inblockedOperationPatternSourcesare stringified and injected verbatim asnew RegExp(...)inside the outer iframe. An inadvertently catastrophic backtracking pattern (e.g.,(a+)+$) would stall the outer iframe's single-threaded JS, blocking all message relaying for the duration of execution. Consider adding a maximum pattern-source length check before injection.// Example guard (TypeScript, host side before generating HTML) const MAX_PATTERN_LEN = 500; if (validationConfig.allowedOperationPatternSource && validationConfig.allowedOperationPatternSource.length > MAX_PATTERN_LEN) { throw new Error('allowedOperationPatternSource exceeds maximum allowed length'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts` around lines 91 - 106, The injection of host-provided regex sources (validationConfig.allowedOperationPatternSource and entries in validationConfig.blockedOperationPatternSources) can allow very long/crafted patterns that cause ReDoS; before generating the RegExp strings in outer-iframe-bootstrap (where allowedOperationPattern and blockedOperationPatterns are composed), enforce a MAX_PATTERN_LEN check (e.g., 500) and validate each pattern source length: if any source exceeds the limit, either throw a clear error or reject it (fail fast) and log the offending key; apply this check for validationConfig.allowedOperationPatternSource and for each element of validationConfig.blockedOperationPatternSources (respecting blockedOperationPatternFlags), so no overly long pattern string is injected into new RegExp(...) in the generated outer iframe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/browser/src/adapters/iframe-adapter.ts`:
- Around line 318-342: The abort() handler is returning zeroed stats because it
has no access to the execution start time captured in execute(); update the code
to persist the start timestamp to a class field (e.g. this.pendingStartTime or
this.currentExecutionStart) when execute() begins, and then use that field
inside abort() to compute and populate stats.duration, stats.startTime, and
stats.endTime before calling pendingSettle; alternatively, modify pendingSettle
to accept an optional timestamp param and pass the execute() startTime through
to abort() so abort() can calculate realistic timing instead of all-zeros.
- Around line 281-295: The catch block currently casts the caught value to Error
(err) and reads err.message which can be undefined for non-Error throws; update
the iframe creation catch to normalize the unknown error before building the
settle payload (use the same pattern as at line 167): detect if error is an
instance of Error and use error.message, otherwise coerce to a string (e.g.,
String(error) or a small helper like getErrorMessage) and include that safe
message in the settle error.message; keep the rest of the settle payload (name
'IframeError', code 'IFRAME_CREATE_FAILED', defaultStats, startTime) unchanged.
- Around line 348-354: The dispose() method currently removes the iframe without
settling any in-flight execute() promise; update dispose() to mirror abort() by
calling pendingSettle (with an appropriate error/rejection reason, e.g., new
Error('disposed')) and resetting this.executing before tearing down the iframe
(outerIframe) so any awaiting execute() resolves/rejects immediately; ensure you
call pendingSettle/pendingSettled path and clear this.outerIframe after
pendingSettle so state is consistent with abort().
In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts`:
- Around line 205-219: The code throws a TypeError when computing
Object.keys(sanitizedArgs) if sanitizedArgs is null; change the operationHistory
push to compute argKeys defensively (e.g. const argKeys = sanitizedArgs &&
typeof sanitizedArgs === 'object' ? Object.keys(sanitizedArgs) : []; then push
argKeys) so null args are recorded as an empty key list and no exception is
thrown; update the block around sanitizedArgs, validateOperation and
operationHistory to use this safe argKeys variable.
---
Duplicate comments:
In `@apps/browser-demo/src/App.tsx`:
- Around line 39-73: handleRun correctly moved startCapture() into the try block
and pairs stopCapture() in finally; no code changes required—leave the async
handler as-is (function handleRun) ensuring startCapture runs before await
run(code) and stopCapture() remains in the finally block and the dependency
array still includes ready, running, code, run, startCapture, stopCapture.
In `@apps/browser-demo/src/components/ExecutionControls.tsx`:
- Around line 1-29: The component ExecutionControls is correct (status and
statusText align and canRun guards runs); remove the duplicate review
marker/comment ([duplicate_comment]) from the PR and ensure no functional
changes to the ExecutionControls component (specifically the status variable and
statusText logic) are made—keep the existing status fallback 'initializing' and
the canRun logic as implemented.
---
Nitpick comments:
In `@apps/browser-demo/src/App.css`:
- Around line 130-131: Replace the broad "transition: all 0.15s;" declarations
with a scoped list of properties to avoid animating layout changes; locate each
"transition: all 0.15s;" occurrence and change it to something like "transition:
background-color 0.15s, color 0.15s, border-color 0.15s, transform 0.15s" (or
the minimal set of properties your component actually animates) so only visual
properties are animated.
In `@apps/browser-demo/src/App.tsx`:
- Around line 20-21: The state variable result is typed as any; replace that
with the enclave's ExecutionResult (import ExecutionResult from the
enclave/browser-enclave package) or define a local equivalent union type
matching { success, error?, value?, stats? } and use useState<ExecutionResult |
null>(null) for result and setResult; update the catch block where you construct
the fallback object to produce the same ExecutionResult shape so TypeScript will
validate it (refer to the result/setResult useState declaration and the
error-handling block that builds the fallback result).
In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts`:
- Around line 91-106: The injection of host-provided regex sources
(validationConfig.allowedOperationPatternSource and entries in
validationConfig.blockedOperationPatternSources) can allow very long/crafted
patterns that cause ReDoS; before generating the RegExp strings in
outer-iframe-bootstrap (where allowedOperationPattern and
blockedOperationPatterns are composed), enforce a MAX_PATTERN_LEN check (e.g.,
500) and validate each pattern source length: if any source exceeds the limit,
either throw a clear error or reject it (fail fast) and log the offending key;
apply this check for validationConfig.allowedOperationPatternSource and for each
element of validationConfig.blockedOperationPatternSources (respecting
blockedOperationPatternFlags), so no overly long pattern string is injected into
new RegExp(...) in the generated outer iframe.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/browser-demo/src/App.cssapps/browser-demo/src/App.tsxapps/browser-demo/src/components/ExecutionControls.tsxlibs/browser/src/adapters/iframe-adapter.tslibs/browser/src/adapters/outer-iframe-bootstrap.ts
…ng in iframe adapter
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (7)
libs/browser/e2e/fixtures/debug-inner.html (2)
616-692:⚠️ Potential issue | 🟠 MajorPreserve
Object.definePropertybefore neutralization and use the saved reference.The script neutralizes dangerous
Objectstatics and later relies onObject.definePropertyfor safe-global injection. Save and use an alias to avoid self-breakage.🛠️ Suggested fix
+ var _defineProperty = Object.defineProperty; // Neutralize dangerous static methods on intrinsic Object (function () { ... })(); ... - Object.defineProperty(window, gKey, { + _defineProperty(window, gKey, { value: safeGlobals[gKey], writable: false, configurable: false, enumerable: false, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/fixtures/debug-inner.html` around lines 616 - 692, The neutralization loop deletes Object statics (the dangerous array around RealObject) but later calls Object.defineProperty when installing safeGlobals on window, which breaks; before the neutralization I should capture and reuse a safe alias to define properties (e.g., save the original Object.defineProperty into a local variable like savedDefineProperty), then in the injection loop use that saved alias instead of Object.defineProperty to set window properties (apply to the block referencing safeGlobals, customGlobals, and the for (var gKey in safeGlobals) loop).
143-160:⚠️ Potential issue | 🟠 MajorValidate message source and
requestIdbefore handling inner control messages.Current handling allows any frame to influence pending tool calls or abort state if it can post enclave-shaped messages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/fixtures/debug-inner.html` around lines 143 - 160, The message handler currently trusts any enclave-shaped postMessage; before touching pendingToolCalls or setting aborted, add strict validation: ensure event.source and event.origin match the expected sender (e.g., compare event.source to the known parent/child window reference and event.origin to an expectedOrigin variable) and verify data.requestId equals the current request/requestId used by this inner enclave; if any check fails, return early. Apply these checks inside the window.addEventListener('message', ...) handler before resolving/rejecting pendingToolCalls or assigning aborted, so only messages from the trusted frame with the matching requestId are processed.libs/browser/src/browser-enclave.ts (1)
306-315:⚠️ Potential issue | 🟠 MajorCustom
suspiciousPatternsconfig is still not honored at execution time.
run()always injectsDEFAULT_SERIALIZED_PATTERNS, andbuildDoubleIframeConfig()can clear defaults to[]when options are partially provided.🧩 Suggested fix
- const executionContext: IframeExecutionContext = { + const configuredPatterns = this.doubleIframeConfig.parentValidation.suspiciousPatterns; + const suspiciousPatterns = + configuredPatterns && configuredPatterns.length > 0 + ? configuredPatterns + : DEFAULT_SERIALIZED_PATTERNS; + + const executionContext: IframeExecutionContext = { ... - suspiciousPatterns: DEFAULT_SERIALIZED_PATTERNS, + suspiciousPatterns, validationConfig, };parentValidation: { ...DEFAULT_DOUBLE_IFRAME_CONFIG.parentValidation, ...options.parentValidation, - suspiciousPatterns: [...(options.parentValidation?.suspiciousPatterns ?? [])], + suspiciousPatterns: + options.parentValidation?.suspiciousPatterns !== undefined + ? [...options.parentValidation.suspiciousPatterns] + : [...DEFAULT_DOUBLE_IFRAME_CONFIG.parentValidation.suspiciousPatterns], },Also applies to: 419-426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/browser-enclave.ts` around lines 306 - 315, The executionContext currently hard-codes suspiciousPatterns to DEFAULT_SERIALIZED_PATTERNS, causing custom patterns to be ignored and buildDoubleIframeConfig can inadvertently clear defaults to []; update run() to set suspiciousPatterns to serializedConfig.suspiciousPatterns (or merged with DEFAULT_SERIALIZED_PATTERNS when needed) instead of always using DEFAULT_SERIALIZED_PATTERNS, and adjust buildDoubleIframeConfig to merge incoming options.suspiciousPatterns with DEFAULT_SERIALIZED_PATTERNS rather than replacing them so partial configs preserve defaults; reference run(), executionContext, buildDoubleIframeConfig(), DEFAULT_SERIALIZED_PATTERNS, and serializedConfig.suspiciousPatterns when making the change.libs/browser/e2e/fixtures/debug-inner2.html (1)
143-160:⚠️ Potential issue | 🟠 MajorValidate sender and
requestIdbefore processing inner control messages.
tool-responseandabortare accepted from any frame if__enclave_msg__is set. Bind handling to the expected parent frame and current request.🔒 Suggested fix
window.addEventListener('message', function (event) { var data = event.data; if (!data || data.__enclave_msg__ !== true) return; + if (event.source !== window.parent) return; + if (data.requestId !== requestId) return; if (data.type === 'tool-response') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/fixtures/debug-inner2.html` around lines 143 - 160, The message handler is trusting any frame that sets __enclave_msg__, so update the window.addEventListener('message', ...) callback to first validate the sender and request context: check event.source equals the expected parent frame (store/reference it as the expected parent/frame variable used when opening the iframe) and verify data.requestId (or data.callId) matches the currentRequestId for this inner enclave before processing; only then look at data.type ('tool-response' or 'abort') and operate on pendingToolCalls[data.callId] (delete/resolve/reject) or set aborted = true. This ensures pending/pendingToolCalls, the 'tool-response' handler, and the 'abort' path only act on messages from the legitimate parent and the active request.libs/browser/src/adapters/inner-iframe-bootstrap.ts (2)
389-458:⚠️ Potential issue | 🔴 CriticalMemory-limit patches are neutralized by current execution order.
Object.freeze(...)runs before redefiningrepeat/join/fill, so those limit-enforcing patches fail silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts` around lines 389 - 458, The freeze block currently runs before the memory-safe prototype patches, so Object.freeze on prototypes prevents redefining stringProto.repeat and arrayProto.join/fill; reorder or narrow freezing: move the entire "Memory-safe prototype patches" IIFE to run before the "Freeze all prototypes" IIFE (or alternatively adjust the freeze IIFE to skip String.prototype and Array.prototype so the patching IIFE can modify stringProto.repeat, arrayProto.join and arrayProto.fill), ensuring the patching IIFE executes and can define the wrapped methods before prototypes are frozen.
151-168:⚠️ Potential issue | 🔴 CriticalReject untrusted message events before handling
tool-response/abort.The listener should validate both sender and request binding before touching
pendingToolCallsoraborted.🔒 Suggested fix
window.addEventListener('message', function(event) { var data = event.data; if (!data || data.__enclave_msg__ !== true) return; + if (event.source !== window.parent) return; + if (data.requestId !== requestId) return; if (data.type === 'tool-response') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts` around lines 151 - 168, The message event handler on window (window.addEventListener('message', ...)) currently trusts all events and directly accesses pendingToolCalls and aborted; update it to validate event.origin and event.source (or a known token in event.data) and ensure the message is bound to the current request context before acting: first check a trustedOrigin/source and that data.requestId or other binding matches a record you control, then only read pendingToolCalls[data.callId], call pending.resolve/pending.reject (wrapping errors with createSafeError) or set aborted = true; do not mutate pendingToolCalls or aborted for untrusted messages.libs/browser/e2e/fixtures/debug-outer.html (1)
304-305:⚠️ Potential issue | 🟠 MajorThe embedded
innerHtmlruntime still carries theObject.definePropertyself-neutralization bug.This giant inline snapshot diverges from the maintained bootstrap codepath and reintroduces a known failure mode when installing safe globals.
🧹 Nitpick comments (5)
apps/browser-demo/src/App.css (2)
106-111: Consolidate repeated panel container styles.These six blocks duplicate the same declarations. Grouping them into one shared selector will reduce maintenance drift.
Suggested refactor
+.security-picker, +.tool-config, +.examples, +.result-display, +.console-output, +.stats-panel { + background: var(--bg-panel); + padding: 12px; + border-radius: var(--radius); + border: 1px solid var(--border); +} - -.security-picker { - background: var(--bg-panel); - padding: 12px; - border-radius: var(--radius); - border: 1px solid var(--border); -} - -.tool-config { - background: var(--bg-panel); - padding: 12px; - border-radius: var(--radius); - border: 1px solid var(--border); -} - -.examples { - background: var(--bg-panel); - padding: 12px; - border-radius: var(--radius); - border: 1px solid var(--border); -} - -.result-display { - background: var(--bg-panel); - padding: 12px; - border-radius: var(--radius); - border: 1px solid var(--border); -} - -.console-output { - background: var(--bg-panel); - padding: 12px; - border-radius: var(--radius); - border: 1px solid var(--border); -} - -.stats-panel { - background: var(--bg-panel); - padding: 12px; - border-radius: var(--radius); - border: 1px solid var(--border); -}Also applies to: 162-167, 212-217, 339-344, 385-390, 432-437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.css` around lines 106 - 111, Multiple CSS blocks (including .security-picker) repeat the same panel styling; consolidate by creating a single shared rule (e.g., .panel-container or .panel) with the declarations currently in .security-picker (background, padding, border-radius, border) and replace the duplicated blocks by applying that shared class—either by combining selectors into one rule (.security-picker, .other-class, .another-class { ... }) or by changing the elements to also include the new .panel-container class and removing the duplicate rules; update any JSX/HTML that relied on the old selectors to include the shared class so styling remains identical.
439-443: Make stats grid responsive at narrower widths.
grid-template-columns: 1fr 1fr 1frstays fixed even on compact screens, which can compress stat cards. Consider stepping down to 2 and then 1 column.Suggested refactor
`@media` (max-width: 960px) { .app-main { grid-template-columns: 1fr; overflow-y: auto; } .left-panel, .right-panel { overflow-y: visible; } .code-textarea { min-height: 150px; } + + .stats-grid { + grid-template-columns: repeat(2, minmax(0, 1fr)); + } } + +@media (max-width: 560px) { + .stats-grid { + grid-template-columns: 1fr; + } +}Also applies to: 469-483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.css` around lines 439 - 443, The .stats-grid CSS uses a fixed three-column layout which compresses cards on narrow screens; update the .stats-grid rule to be responsive by switching to a fluid grid (e.g., use repeat(auto-fit, minmax(..., 1fr)) or media queries) so it collapses from 3 columns to 2 and then 1 at narrower breakpoints; apply the same change to the other .stats-grid block referenced in the diff so both instances use the responsive grid settings.apps/browser-demo/src/types.ts (2)
25-29:ExecutionStatsis a subset of the library'sExecutionStats— consider importing or usingPick<>.
libs/browser/src/types.tsdefinesExecutionStatswith five fields (duration,toolCallCount,iterationCount,startTime,endTime) and exports it from the library index. The demo re-defines a trimmed version, which forces the manual projection inApp.tsx(lines 50–54). If the upstream type gains or renames fields, this silently diverges.♻️ Proposed fix
Option A — import the library type directly (no projection needed in
App.tsx):-export interface ExecutionStats { - duration: number; - toolCallCount: number; - iterationCount: number; -} +export type { ExecutionStats } from '@enclave-vm/browser';Option B — keep it local but make the structural intent explicit:
-export interface ExecutionStats { - duration: number; - toolCallCount: number; - iterationCount: number; -} +import type { ExecutionStats as LibStats } from '@enclave-vm/browser'; +export type ExecutionStats = Pick<LibStats, 'duration' | 'toolCallCount' | 'iterationCount'>;As per coding guidelines, prefer workspace import paths over deep cross-library relative imports or duplicate definitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/types.ts` around lines 25 - 29, The demo re-defines a partial ExecutionStats that duplicates the library type and forces manual projection in App.tsx; instead import the upstream ExecutionStats from the library index (or explicitly derive a subset with Pick<LibraryExecutionStats, 'duration' | 'toolCallCount' | 'iterationCount'>) and replace the local interface in apps/browser-demo/src/types.ts, then remove the manual projection in App.tsx (use the imported type directly for props/state). Reference: ExecutionStats and the projection usage in App.tsx so you update type imports and usages accordingly.
10-10:SecurityLevelis already exported fromlibs/browser— import it instead of re-defining.This is an exact duplicate of
libs/browser/src/types.tsline 11, which is already re-exported from the library's public index. Keeping a local copy means the demo can silently diverge if the upstream union is ever extended.♻️ Proposed fix
Remove the local definition and import from the workspace package:
-export type SecurityLevel = 'STRICT' | 'SECURE' | 'STANDARD' | 'PERMISSIVE';Add to the consumer files (or re-export from this file):
+export type { SecurityLevel } from '@enclave-vm/browser';As per coding guidelines, prefer workspace import paths over duplicate type definitions across library boundaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/types.ts` at line 10, Remove the locally duplicated SecurityLevel type and instead import and re-export the shared type from the workspace package (the libs/browser public index) so the demo uses the canonical definition; locate the local export SecurityLevel in this file, delete that line, add an import of SecurityLevel from the library's public entry, and (if needed) re-export it from this module or update consumer files to import from the workspace package.apps/browser-demo/src/App.tsx (1)
22-22:statsstate duplicatesresult?.stats— consider deriving it.
statsis always set in sync withresult.stats(lines 50–55 and 64–66).StatsPanelon line 109 could acceptresult?.stats ?? nulldirectly, eliminating the separate state slice and the manual projection.♻️ Proposed simplification
- const [stats, setStats] = useState<ExecutionStats | null>(null);Then in the catch/success paths, remove the
setStats(...)calls and update the JSX:- <StatsPanel stats={stats} /> + <StatsPanel stats={result?.stats ?? null} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.tsx` at line 22, Remove the redundant local state "stats" and derive it directly from "result?.stats": delete the useState declaration for stats and all setStats(...) calls in the code paths that handle result updates (the success and catch/error handlers that currently call setStats). Update the JSX to pass result?.stats ?? null into <StatsPanel /> instead of "stats" and remove any unused ExecutionStats type/import if it becomes unused; keep all references to "result" and the functions that produce/update it intact (e.g., the function handling the query/result assignment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/browser-demo/src/App.css`:
- Around line 265-283: The current .code-textarea removes the default outline
and only changes border color on .code-textarea:focus, which reduces keyboard
focus visibility; update the focus styles by adding a
.code-textarea:focus-visible rule that restores a clear accessible focus
indicator (e.g., a visible ring via box-shadow or outline with var(--accent) and
appropriate spread) while keeping the existing :focus border-color change;
target the .code-textarea selector so keyboard users get the strong focus ring
in all themes without affecting mouse focus behavior.
In `@apps/browser-demo/src/App.tsx`:
- Around line 58-65: The current setResult call overwrites the original error
type by hardcoding name: 'AppError'; change the payload construction in the
setResult call so the error.name preserves the original when err is an Error
(e.g. name: err instanceof Error ? err.name : 'AppError' or fallback to
String(err)), keep the existing message expression (message: err instanceof
Error ? err.message : String(err)) and stats: zeroStats; update the object
passed to setResult (the one containing success, error, stats) to use that
conditional for name so diagnostics retain the original Error type.
In `@libs/browser/e2e/fixtures/debug-inner.html`:
- Around line 414-509: The freeze IIFE runs before the "Memory-safe prototype
patches" IIFE, preventing Object.defineProperty from replacing
String.prototype.repeat, Array.prototype.join and Array.prototype.fill
(origRepeat/origJoin/origFill), so move the freeze block to after the
memory-safe patches IIFE or exclude stringProto/arrayProto from the protos
array; ensure the patch IIFE defines the new methods (using
Object.defineProperty on stringProto and arrayProto) before any
Object.freeze(protos[i]) calls so the memory caps (track and RangeError checks)
take effect.
In `@libs/browser/e2e/fixtures/debug-inner2.html`:
- Around line 414-509: The prototype-freezing IIFE that calls Object.freeze on
protos runs before the "Memory-safe prototype patches" IIFE, causing the
Object.defineProperty calls for stringProto.repeat, arrayProto.join and
arrayProto.fill (and variables origRepeat, origJoin, origFill) to fail; fix by
applying the memory-safe patches first and only then freezing prototypes (i.e.,
move the entire memory-safe prototype patches IIFE to run before the "Freeze all
prototypes" IIFE or move the Object.freeze loop to after the patch IIFE),
preserving the existing try/catch blocks and keeping the same property
definitions for repeat, join and fill.
In `@libs/browser/e2e/fixtures/debug-outer.html`:
- Around line 219-299: The postMessage handler bound in
window.addEventListener('message', ...) accepts any message with __enclave_msg__
and lacks sender/request identity checks, allowing cross-frame interference;
update the handler to first verify the message origin and identity by checking
that data.requestId equals the expected requestId for this session and that
data.sender (or an equivalent role field) matches the expected role (e.g.,
'inner' for messages coming from the inner iframe or 'host' for host messages)
before processing, and reject/ignore messages that do not match; apply these
checks at the top of the handler (the same function that references
data.__enclave_msg__, data.type, and uses sendToHost/sendToInner) so all
branches (tool-call, result, console, tool-response, abort) only act on verified
messages.
In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts`:
- Around line 463-485: The removal of dangerous globals deletes builtin
constructors like Proxy (and possibly WeakMap) before bootstrap uses them,
causing createSecureProxy() (which calls new Proxy(...) at the createSecureProxy
symbol) to fail during safeGlobals initialization; fix by caching references to
required builtins (e.g., Proxy, WeakMap) into local constants before the
deletion loops that iterate dangerousGlobals and browserDangerous (referenced
around getDangerousGlobals and the safeGlobals initialization block) and then
update createSecureProxy to use the cached references instead of global
window.Proxy; ensure any other bootstrap primitives used during initialization
are similarly cached.
In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts`:
- Around line 193-282: The outer iframe message relay lacks filtering by
requestId, so host-origin messages like 'tool-response' and 'abort' can affect
sibling outer iframes; update the message handler inside the
window.addEventListener('message', ...) listener to check that incoming messages
include data.requestId === requestId before acting on host-sourced types (at
least for branches handling data.type === 'tool-response' and data.type ===
'abort'), and likewise ensure inner-sourced messages (e.g., 'tool-call',
'result', 'console') include the same requestId if applicable; use the existing
variables requestId, fromInner, and fromHost to gate processing and drop
messages with mismatched or missing requestId.
---
Duplicate comments:
In `@libs/browser/e2e/fixtures/debug-inner.html`:
- Around line 616-692: The neutralization loop deletes Object statics (the
dangerous array around RealObject) but later calls Object.defineProperty when
installing safeGlobals on window, which breaks; before the neutralization I
should capture and reuse a safe alias to define properties (e.g., save the
original Object.defineProperty into a local variable like savedDefineProperty),
then in the injection loop use that saved alias instead of Object.defineProperty
to set window properties (apply to the block referencing safeGlobals,
customGlobals, and the for (var gKey in safeGlobals) loop).
- Around line 143-160: The message handler currently trusts any enclave-shaped
postMessage; before touching pendingToolCalls or setting aborted, add strict
validation: ensure event.source and event.origin match the expected sender
(e.g., compare event.source to the known parent/child window reference and
event.origin to an expectedOrigin variable) and verify data.requestId equals the
current request/requestId used by this inner enclave; if any check fails, return
early. Apply these checks inside the window.addEventListener('message', ...)
handler before resolving/rejecting pendingToolCalls or assigning aborted, so
only messages from the trusted frame with the matching requestId are processed.
In `@libs/browser/e2e/fixtures/debug-inner2.html`:
- Around line 143-160: The message handler is trusting any frame that sets
__enclave_msg__, so update the window.addEventListener('message', ...) callback
to first validate the sender and request context: check event.source equals the
expected parent frame (store/reference it as the expected parent/frame variable
used when opening the iframe) and verify data.requestId (or data.callId) matches
the currentRequestId for this inner enclave before processing; only then look at
data.type ('tool-response' or 'abort') and operate on
pendingToolCalls[data.callId] (delete/resolve/reject) or set aborted = true.
This ensures pending/pendingToolCalls, the 'tool-response' handler, and the
'abort' path only act on messages from the legitimate parent and the active
request.
In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts`:
- Around line 389-458: The freeze block currently runs before the memory-safe
prototype patches, so Object.freeze on prototypes prevents redefining
stringProto.repeat and arrayProto.join/fill; reorder or narrow freezing: move
the entire "Memory-safe prototype patches" IIFE to run before the "Freeze all
prototypes" IIFE (or alternatively adjust the freeze IIFE to skip
String.prototype and Array.prototype so the patching IIFE can modify
stringProto.repeat, arrayProto.join and arrayProto.fill), ensuring the patching
IIFE executes and can define the wrapped methods before prototypes are frozen.
- Around line 151-168: The message event handler on window
(window.addEventListener('message', ...)) currently trusts all events and
directly accesses pendingToolCalls and aborted; update it to validate
event.origin and event.source (or a known token in event.data) and ensure the
message is bound to the current request context before acting: first check a
trustedOrigin/source and that data.requestId or other binding matches a record
you control, then only read pendingToolCalls[data.callId], call
pending.resolve/pending.reject (wrapping errors with createSafeError) or set
aborted = true; do not mutate pendingToolCalls or aborted for untrusted
messages.
In `@libs/browser/src/browser-enclave.ts`:
- Around line 306-315: The executionContext currently hard-codes
suspiciousPatterns to DEFAULT_SERIALIZED_PATTERNS, causing custom patterns to be
ignored and buildDoubleIframeConfig can inadvertently clear defaults to [];
update run() to set suspiciousPatterns to serializedConfig.suspiciousPatterns
(or merged with DEFAULT_SERIALIZED_PATTERNS when needed) instead of always using
DEFAULT_SERIALIZED_PATTERNS, and adjust buildDoubleIframeConfig to merge
incoming options.suspiciousPatterns with DEFAULT_SERIALIZED_PATTERNS rather than
replacing them so partial configs preserve defaults; reference run(),
executionContext, buildDoubleIframeConfig(), DEFAULT_SERIALIZED_PATTERNS, and
serializedConfig.suspiciousPatterns when making the change.
---
Nitpick comments:
In `@apps/browser-demo/src/App.css`:
- Around line 106-111: Multiple CSS blocks (including .security-picker) repeat
the same panel styling; consolidate by creating a single shared rule (e.g.,
.panel-container or .panel) with the declarations currently in .security-picker
(background, padding, border-radius, border) and replace the duplicated blocks
by applying that shared class—either by combining selectors into one rule
(.security-picker, .other-class, .another-class { ... }) or by changing the
elements to also include the new .panel-container class and removing the
duplicate rules; update any JSX/HTML that relied on the old selectors to include
the shared class so styling remains identical.
- Around line 439-443: The .stats-grid CSS uses a fixed three-column layout
which compresses cards on narrow screens; update the .stats-grid rule to be
responsive by switching to a fluid grid (e.g., use repeat(auto-fit, minmax(...,
1fr)) or media queries) so it collapses from 3 columns to 2 and then 1 at
narrower breakpoints; apply the same change to the other .stats-grid block
referenced in the diff so both instances use the responsive grid settings.
In `@apps/browser-demo/src/App.tsx`:
- Line 22: Remove the redundant local state "stats" and derive it directly from
"result?.stats": delete the useState declaration for stats and all setStats(...)
calls in the code paths that handle result updates (the success and catch/error
handlers that currently call setStats). Update the JSX to pass result?.stats ??
null into <StatsPanel /> instead of "stats" and remove any unused ExecutionStats
type/import if it becomes unused; keep all references to "result" and the
functions that produce/update it intact (e.g., the function handling the
query/result assignment).
In `@apps/browser-demo/src/types.ts`:
- Around line 25-29: The demo re-defines a partial ExecutionStats that
duplicates the library type and forces manual projection in App.tsx; instead
import the upstream ExecutionStats from the library index (or explicitly derive
a subset with Pick<LibraryExecutionStats, 'duration' | 'toolCallCount' |
'iterationCount'>) and replace the local interface in
apps/browser-demo/src/types.ts, then remove the manual projection in App.tsx
(use the imported type directly for props/state). Reference: ExecutionStats and
the projection usage in App.tsx so you update type imports and usages
accordingly.
- Line 10: Remove the locally duplicated SecurityLevel type and instead import
and re-export the shared type from the workspace package (the libs/browser
public index) so the demo uses the canonical definition; locate the local export
SecurityLevel in this file, delete that line, add an import of SecurityLevel
from the library's public entry, and (if needed) re-export it from this module
or update consumer files to import from the workspace package.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/browser-demo/src/App.cssapps/browser-demo/src/App.tsxapps/browser-demo/src/components/ResultDisplay.tsxapps/browser-demo/src/types.tslibs/browser/e2e/fixtures/debug-inner.htmllibs/browser/e2e/fixtures/debug-inner2.htmllibs/browser/e2e/fixtures/debug-outer.htmllibs/browser/src/adapters/iframe-adapter.tslibs/browser/src/adapters/inner-iframe-bootstrap.tslibs/browser/src/adapters/outer-iframe-bootstrap.tslibs/browser/src/browser-enclave.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/browser-demo/src/components/ResultDisplay.tsx
… bootstrap processes
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
libs/browser/e2e/fixtures/debug-outer.html (1)
313-314:⚠️ Potential issue | 🟠 MajorEmbedded inner sandbox string still missing
_definePropertysave before neutralization.The inline inner HTML (line 313-314) neutralizes
Object.definePropertyviadelete RealObject[dangerous[i]]but then usesObject.defineProperty(window, gKey, ...)to install safe globals. If the delete succeeds, safe globals won't be defined. The standalone fixtures (debug-inner.html,debug-inner2.html) were fixed to save_definePropertybefore neutralization, but this embedded copy was not updated.This embedded copy is extremely difficult to maintain as a single-line string. Strongly recommend generating it programmatically (e.g., via
generateInnerIframeHtml) rather than hand-maintaining a ~400-line JS payload in a string literal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/fixtures/debug-outer.html` around lines 313 - 314, The embedded inner sandbox string omits saving the original Object.defineProperty before neutralizing it, causing Object.defineProperty to be deleted on RealObject and preventing installation of safe globals; fix by updating this embedded payload to mirror the other fixtures: capture and store the original defineProperty (e.g., _defineProperty) from RealObject before the "Neutralize dangerous static methods" loop that deletes properties, then use that saved _defineProperty (or restore defineProperty) so the subsequent loop that calls Object.defineProperty(window, gKey, ...) works; ideally replace the large inline string with a programmatically generated inner HTML function (e.g., generateInnerIframeHtml) or reuse the corrected debug-inner.html/debug-inner2.html content to avoid divergence.libs/browser/src/adapters/inner-iframe-bootstrap.ts (1)
659-663:⚠️ Potential issue | 🔴 CriticalUser code embedded without
</script>escaping — script breakout risk.
generateUserCodeExecutionreturnsuserCodeverbatim. This string is then embedded inside a<script>block bybuildIframeHtml. If the transformed code contains the literal</script>(e.g. in a string literal), the HTML parser will close the tag prematurely, breaking execution and potentially enabling injection.Escape
</→<\/(or split the closing tag) before embedding.🛠️ Proposed fix
function generateUserCodeExecution(userCode: string): string { - return userCode; + // Prevent </script> breakout when embedded inside <script> tags + return userCode.replace(/<\//g, '<\\/'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts` around lines 659 - 663, The generateUserCodeExecution function currently returns userCode verbatim which, when inlined into the <script> block by buildIframeHtml, allows a literal "</script>" in the transformed code to break out of the script tag; modify generateUserCodeExecution to escape closing script tags by replacing occurrences of "</script>" (or more generally "</") with "<\/script>" (or "<\/") before returning, ensuring the returned string is safe to embed in buildIframeHtml and referencing generateUserCodeExecution as the place to apply this transformation.
🧹 Nitpick comments (5)
apps/browser-demo/src/App.tsx (1)
41-43: Consolidate duplicated execution-reset state updates.The same reset block is repeated in
handleRunandhandleExampleSelect; extracting a helper reduces drift risk.♻️ Optional refactor
+ const resetExecutionState = useCallback(() => { + setResult(null); + setConsoleEntries([]); + setStats(null); + }, []); + const handleRun = useCallback(async () => { if (!ready || running) return; setRunning(true); - setResult(null); - setConsoleEntries([]); - setStats(null); + resetExecutionState(); @@ - }, [ready, running, code, run, startCapture, stopCapture]); + }, [ready, running, code, run, startCapture, stopCapture, resetExecutionState]); const handleExampleSelect = useCallback((exampleCode: string) => { setCode(exampleCode); - setResult(null); - setConsoleEntries([]); - setStats(null); - }, []); + resetExecutionState(); + }, [resetExecutionState]);Also applies to: 74-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/browser-demo/src/App.tsx` around lines 41 - 43, Duplicate state-reset logic (setResult(null); setConsoleEntries([]); setStats(null)) appears in handleRun and handleExampleSelect; extract these three calls into a single helper (e.g., resetExecutionState or resetRunState) and replace the duplicated blocks in handleRun and handleExampleSelect with a call to that helper so future changes stay consistent.libs/browser/e2e/fixtures/debug-inner2.html (1)
527-558: Fixture diverges from production:locationanddocumentinbrowserDangerous+ missingvar documentshadow.The production
inner-iframe-bootstrap.tsintentionally omitslocationanddocumentfrom thebrowserDangerouslist (with explanatory comments) and instead shadowsdocumentviavar document = undefinedin the user-code execution scope. This fixture includes both inbrowserDangerous(lines 542-543) and lacks thevar documentshadow. Whiledelete/assignment will silently fail for these non-configurable properties, this means the fixture doesn't accurately represent the production sandbox — which could lead to E2E tests passing here but behaving differently in production.Consider aligning the fixture with the production generator's approach for higher-fidelity E2E testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/fixtures/debug-inner2.html` around lines 527 - 558, The fixture's browserDangerous array currently includes 'location' and 'document' and lacks the local shadow used in production; update the fixture to match production by removing 'location' and 'document' from the browserDangerous list (the variable browserDangerous) and add a local shadow declaration (e.g., var document = undefined) in the user-code execution scope where the sandboxed globals are set up so the fixture mirrors inner-iframe-bootstrap.ts behavior.libs/browser/src/browser-enclave.ts (1)
185-229: Constructor looks solid; consider validating numeric config bounds.The constructor merges options cleanly with level-based defaults. One minor hardening: numeric options like
timeout,maxIterations,maxToolCalls, andmemoryLimitare accepted without bounds checking. For example,timeout: 0would cause immediate timeout, andmemoryLimit: -1would disable memory patches (via theif (ml <= 0) return;guard in the generated code). AddingMath.max(minValue, ...)or validation would prevent surprising behavior from misconfiguration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/browser-enclave.ts` around lines 185 - 229, The constructor for BrowserEnclave accepts numeric options (timeout, maxIterations, maxToolCalls, memoryLimit) without bounds checks; update the BrowserEnclave constructor to validate and clamp these values after merging with levelConfig (e.g., compute timeout, maxIterations, maxToolCalls, memoryLimit into local vars and apply Math.max/min or explicit validation), ensuring sensible minimums (e.g., timeout >= 1, iterations >= 1, toolCalls >= 0, memoryLimit >= 1) and either clamp or throw when out-of-range; reference the existing symbol names (timeout, maxIterations, maxToolCalls, memoryLimit, levelConfig, SECURITY_LEVEL_CONFIGS, BrowserEnclaveOptions) when adding these checks so the config assignment uses the validated/clamped values.libs/browser/e2e/fixtures/debug-inner.html (1)
1-747: Near-identical fixture todebug-inner2.html— consider generating from a shared template or parameterizing.
debug-inner.htmlanddebug-inner2.htmlare ~700 lines each and differ only in therequestId(line 14) and the__ag_mainbody (line 700return 2 + 3vsreturn 42). Maintaining two copies of the same sandbox runtime by hand will lead to drift (one gets a fix, the other doesn't). Consider either:
- Generating both fixtures from
generateInnerIframeHtmlwith different inputs, or- Extracting the shared runtime into a helper and parameterizing just the differing parts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/fixtures/debug-inner.html` around lines 1 - 747, The two fixtures (debug-inner.html and debug-inner2.html) duplicate the entire sandbox runtime and differ only by requestId and the __ag_main body; extract the shared runtime into a single generator function (e.g., generateInnerIframeHtml) or a helper template and parameterize the differing pieces (requestId and the __ag_main function body) so both fixtures are produced from one source instead of hand-copying; update usages to call generateInnerIframeHtml({ requestId, mainBody }) or similar and replace the static files with the generated outputs.libs/browser/e2e/fixtures/debug-outer.html (1)
219-226:requestIdcheck allows messages with norequestIdthrough.Line 226:
if (data.requestId && data.requestId !== requestId) return;— a message withdata.requestId === undefinedpasses this guard. For the productionouter-iframe-bootstrap.ts, the same pattern is used. While source checks (fromInner/fromHost) add a layer of defense, a strictdata.requestId !== requestIdwould prevent accidental processing of untagged messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/e2e/fixtures/debug-outer.html` around lines 219 - 226, The message handler currently lets messages with no requestId through because of the conditional `if (data.requestId && data.requestId !== requestId) return;`; update the guard in the window.addEventListener('message', function (event) { ... }) handler to reject any message whose data.requestId does not strictly equal the expected requestId (e.g. replace that line with a strict check like `if (data.requestId !== requestId) return;`) so untagged messages are ignored; keep the existing source checks (fromInner, fromHost) and completed handling intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/browser-demo/src/App.tsx`:
- Around line 67-70: The finally block can exit early if stopCapture() throws,
leaving running true; wrap the stopCapture() call in its own try/catch inside
the finally so errors are swallowed/logged and setConsoleEntries still receives
a fallback (e.g., []), and always call setRunning(false) after that; reference
the finally block around stopCapture(), setConsoleEntries(), and setRunning()
and ensure you log or ignore the stopCapture error (do not let it propagate),
and follow the ESLint guideline by prefixing any intentionally unused catch
parameters with an underscore (e.g., catch (_err)).
In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts`:
- Around line 193-200: The current message handler in window.addEventListener
allows messages with missing or undefined requestId through because it uses a
guarded check; update the guard to require exact match by replacing the loose
check that reads like "if (data.requestId && data.requestId !== requestId)
return" with a strict comparison "if (data.requestId !== requestId) return" so
any message without the expected requestId is rejected; apply the same fix in
the analogous handler in inner-iframe-bootstrap.ts (look for the requestId check
near the inner frame message handling, and keep the surrounding logic that uses
completed, innerFrame, fromInner, and fromHost unchanged).
In `@libs/browser/src/browser-enclave.ts`:
- Around line 390-411: The AST validator only receives custom globals for the
'agentscript' preset, so when BrowserEnclaveOptions.globals is supplied with a
non-'agentscript' preset the validator will incorrectly reject code; update the
preset selection logic (where presetName is used to construct JSAstValidator
with
createStrictPreset/createSecurePreset/createStandardPreset/createPermissivePreset)
to detect if this.config.globals (or BrowserEnclaveOptions.globals) is provided
and presetName !== 'agentscript' and throw a clear, descriptive Error (e.g.
"Custom globals are only supported with the 'agentscript' preset; remove globals
or switch preset") so callers get an immediate validation message, or
alternatively adjust the docs/comments on BrowserEnclaveOptions.globals to state
the preset-specific limitation if you prefer documentation over runtime
enforcement.
---
Duplicate comments:
In `@libs/browser/e2e/fixtures/debug-outer.html`:
- Around line 313-314: The embedded inner sandbox string omits saving the
original Object.defineProperty before neutralizing it, causing
Object.defineProperty to be deleted on RealObject and preventing installation of
safe globals; fix by updating this embedded payload to mirror the other
fixtures: capture and store the original defineProperty (e.g., _defineProperty)
from RealObject before the "Neutralize dangerous static methods" loop that
deletes properties, then use that saved _defineProperty (or restore
defineProperty) so the subsequent loop that calls Object.defineProperty(window,
gKey, ...) works; ideally replace the large inline string with a
programmatically generated inner HTML function (e.g., generateInnerIframeHtml)
or reuse the corrected debug-inner.html/debug-inner2.html content to avoid
divergence.
In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts`:
- Around line 659-663: The generateUserCodeExecution function currently returns
userCode verbatim which, when inlined into the <script> block by
buildIframeHtml, allows a literal "</script>" in the transformed code to break
out of the script tag; modify generateUserCodeExecution to escape closing script
tags by replacing occurrences of "</script>" (or more generally "</") with
"<\/script>" (or "<\/") before returning, ensuring the returned string is safe
to embed in buildIframeHtml and referencing generateUserCodeExecution as the
place to apply this transformation.
---
Nitpick comments:
In `@apps/browser-demo/src/App.tsx`:
- Around line 41-43: Duplicate state-reset logic (setResult(null);
setConsoleEntries([]); setStats(null)) appears in handleRun and
handleExampleSelect; extract these three calls into a single helper (e.g.,
resetExecutionState or resetRunState) and replace the duplicated blocks in
handleRun and handleExampleSelect with a call to that helper so future changes
stay consistent.
In `@libs/browser/e2e/fixtures/debug-inner.html`:
- Around line 1-747: The two fixtures (debug-inner.html and debug-inner2.html)
duplicate the entire sandbox runtime and differ only by requestId and the
__ag_main body; extract the shared runtime into a single generator function
(e.g., generateInnerIframeHtml) or a helper template and parameterize the
differing pieces (requestId and the __ag_main function body) so both fixtures
are produced from one source instead of hand-copying; update usages to call
generateInnerIframeHtml({ requestId, mainBody }) or similar and replace the
static files with the generated outputs.
In `@libs/browser/e2e/fixtures/debug-inner2.html`:
- Around line 527-558: The fixture's browserDangerous array currently includes
'location' and 'document' and lacks the local shadow used in production; update
the fixture to match production by removing 'location' and 'document' from the
browserDangerous list (the variable browserDangerous) and add a local shadow
declaration (e.g., var document = undefined) in the user-code execution scope
where the sandboxed globals are set up so the fixture mirrors
inner-iframe-bootstrap.ts behavior.
In `@libs/browser/e2e/fixtures/debug-outer.html`:
- Around line 219-226: The message handler currently lets messages with no
requestId through because of the conditional `if (data.requestId &&
data.requestId !== requestId) return;`; update the guard in the
window.addEventListener('message', function (event) { ... }) handler to reject
any message whose data.requestId does not strictly equal the expected requestId
(e.g. replace that line with a strict check like `if (data.requestId !==
requestId) return;`) so untagged messages are ignored; keep the existing source
checks (fromInner, fromHost) and completed handling intact.
In `@libs/browser/src/browser-enclave.ts`:
- Around line 185-229: The constructor for BrowserEnclave accepts numeric
options (timeout, maxIterations, maxToolCalls, memoryLimit) without bounds
checks; update the BrowserEnclave constructor to validate and clamp these values
after merging with levelConfig (e.g., compute timeout, maxIterations,
maxToolCalls, memoryLimit into local vars and apply Math.max/min or explicit
validation), ensuring sensible minimums (e.g., timeout >= 1, iterations >= 1,
toolCalls >= 0, memoryLimit >= 1) and either clamp or throw when out-of-range;
reference the existing symbol names (timeout, maxIterations, maxToolCalls,
memoryLimit, levelConfig, SECURITY_LEVEL_CONFIGS, BrowserEnclaveOptions) when
adding these checks so the config assignment uses the validated/clamped values.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/browser-demo/src/App.cssapps/browser-demo/src/App.tsxlibs/browser/e2e/fixtures/debug-inner.htmllibs/browser/e2e/fixtures/debug-inner2.htmllibs/browser/e2e/fixtures/debug-outer.htmllibs/browser/src/adapters/inner-iframe-bootstrap.tslibs/browser/src/adapters/outer-iframe-bootstrap.tslibs/browser/src/browser-enclave.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/browser-demo/src/App.css
… in iframe communication
…curity architecture, and React integration
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
libs/browser/src/adapters/inner-iframe-bootstrap.ts (1)
152-156:⚠️ Potential issue | 🟠 MajorValidate
event.sourcefor inbound outer-frame messages.Line 155 checks
requestId, but Line 152-156 still accepts matching messages from any source window. Gate this listener towindow.parentbefore processing tool responses/abort.🛠️ Proposed fix
window.addEventListener('message', function(event) { var data = event.data; if (!data || data.__enclave_msg__ !== true) return; + if (event.source !== window.parent) return; if (data.requestId !== requestId) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts` around lines 152 - 156, The message listener in window.addEventListener('message', function(event) { ... }) accepts matching requestId from any source; restrict it to only handle messages from the outer frame by validating event.source === window.parent (or abort if not) before checking data.__enclave_msg__ and requestId so only parent frame responses/aborts are processed; update the listener in inner-iframe-bootstrap.ts (the anonymous handler attached to window) to short-circuit when event.source !== window.parent.libs/browser/src/browser-enclave.ts (1)
314-326:⚠️ Potential issue | 🟠 MajorConfigured suspicious patterns are still ignored in execution context.
Line 325 always injects
DEFAULT_SERIALIZED_PATTERNS, so user-provideddoubleIframe.parentValidation.suspiciousPatternsnever take effect.As per coding guidelines `libs/**`: “Review for API correctness, breaking changes, and consistency with docs.”💡 Suggested fix
const executionContext: IframeExecutionContext = { config: serializedConfig, toolHandler: toolHandler || this.config.toolHandler, securityLevel: this.securityLevel, doubleIframeConfig: this.doubleIframeConfig, secureProxyConfig: this.config.secureProxyConfig, blockedProperties: serializedConfig.blockedProperties, - // TODO: Custom SuspiciousPattern[] from config are not currently serialized and - // injected. Proper support would require an API that accepts - // SerializableSuspiciousPattern[] (string bodies) or a serialization bridge. - suspiciousPatterns: DEFAULT_SERIALIZED_PATTERNS, + suspiciousPatterns: + pv.suspiciousPatterns && pv.suspiciousPatterns.length > 0 + ? pv.suspiciousPatterns + : DEFAULT_SERIALIZED_PATTERNS, validationConfig, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/browser-enclave.ts` around lines 314 - 326, The executionContext currently always sets suspiciousPatterns to DEFAULT_SERIALIZED_PATTERNS which ignores user-provided patterns; modify the code that builds executionContext (symbol: executionContext) to prefer the serialized user-configured suspicious patterns (e.g., serializedConfig.doubleIframe?.parentValidation?.suspiciousPatterns or validationConfig.suspiciousPatterns) and only fall back to DEFAULT_SERIALIZED_PATTERNS when none are present, preserving existing serialization/format expectations; ensure you reference the same serialized value used elsewhere so doubleIframe.parentValidation.suspiciousPatterns actually propagates into executionContext.suspiciousPatterns.
🧹 Nitpick comments (1)
libs/browser/src/browser-enclave.ts (1)
426-428: Prefer cloning nested default config objects in the no-options path.Line 427 shallow-copies only the top level.
parentValidationstays shared by reference, which can cause cross-instance config bleed if later mutated.💡 Suggested refactor
if (!options) { - return { ...DEFAULT_DOUBLE_IFRAME_CONFIG }; + return { + ...DEFAULT_DOUBLE_IFRAME_CONFIG, + parentValidation: { + ...DEFAULT_DOUBLE_IFRAME_CONFIG.parentValidation, + suspiciousPatterns: [...DEFAULT_DOUBLE_IFRAME_CONFIG.parentValidation.suspiciousPatterns], + }, + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/browser/src/browser-enclave.ts` around lines 426 - 428, The current early-return clones only the top-level DEFAULT_DOUBLE_IFRAME_CONFIG (return { ...DEFAULT_DOUBLE_IFRAME_CONFIG }) causing nested objects like parentValidation to be shared across instances; update the no-options path to deep-clone the default config (e.g., use structuredClone(DEFAULT_DOUBLE_IFRAME_CONFIG) or a shallow spread that also clones nested objects such as parentValidation) so each caller gets an independent DEFAULT_DOUBLE_IFRAME_CONFIG copy; keep references to DEFAULT_DOUBLE_IFRAME_CONFIG and parentValidation in your change to locate the return site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/enclave/core-libraries/enclave-browser/react-integration.mdx`:
- Around line 194-199: The example in handleRun can leave console capture active
if run() throws; update the snippet using startCapture() and stopCapture() so
stopCapture() is always called by wrapping the run() call in a try/finally (call
stopCapture() in finally) and propagate or handle errors as appropriate; ensure
both occurrences (handleRun and the other example at lines ~339-347) use the
same pattern to guarantee capture teardown.
- Around line 100-108: The handleRun function currently awaits run(code) without
catching rejections; wrap the await in a try/catch inside CodeRunner.handleRun
so any thrown/rejected errors are caught and handled: keep the existing early
return on ready, then in try call const res = await run(code) and setResult on
res.success or res.error as before, and in catch call setResult with a clear
error message (including the caught error.message) to avoid unhandled promise
rejections; reference the handleRun function, run, setResult, ready and code
symbols while applying this change.
In `@docs/enclave/core-libraries/enclave-browser/security-architecture.mdx`:
- Line 154: Update the table row for parallel(fns) so the description reflects
actual behavior: change “Parallel execution with max 100 concurrent operations”
to something like “Parallel execution of up to 100 operations total with a
maximum concurrency of 20” (edit the cell for `parallel(fns)` in the
security-architecture.mdx table to state both the total limit and the
concurrency cap).
In `@libs/browser/e2e/fixtures/debug-inner.html`:
- Around line 150-152: When handling data.type === 'abort' you currently only
set the aborted flag; instead, also actively reject any in-flight tool-call
promises by tracking and calling their stored reject functions: when creating
the tool call promise (the in-flight promise created where tool calls are sent),
save its reject callback into a pendingCalls/pendingPromises map keyed by the
call id, and in the abort branch call each stored reject with an AbortError
(then clear the map) before setting aborted = true so no promises remain
pending.
In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts`:
- Around line 33-40: The code currently embeds raw JSON.stringify outputs (e.g.,
blockedProperties) into an inline script which allows values like </script> from
config.globals to break out and inject markup; replace direct JSON.stringify
usage with a safe serializer that escapes closing script tags and HTML comment
openers (for example, create a utility safeSerialize(value) that does
JSON.stringify(value).replace(/<\/script/gi,'<\\/script').replace(/<!--/g,'<\\!--')
and use safeSerialize(...) wherever the code interpolates serialized payloads
such as blockedProperties, config.globals and any other inline JSON payloads
referenced in this file (also update the other occurrences noted in the review).
In `@libs/browser/src/adapters/outer-iframe-bootstrap.ts`:
- Around line 349-357: The validateDetectBody function currently checks
DANGEROUS_BODY_PATTERNS but misses script-closing tokens, allowing a detectBody
to break out of the inline <script>; update validateDetectBody to also reject
any detectBody containing case-insensitive script-closing sequences (e.g.,
"</script" and full "</script>") and common variants (with whitespace/newline
between “</” and “script” or different casing) before embedding; ensure you
perform the check alongside the existing loop (resetting pattern.lastIndex as
done now) and throw the same Error with the existing message when such a token
is found; apply the same check to the other analogous validation block
referenced in the file (the block around the second validation at lines
~365-376).
---
Duplicate comments:
In `@libs/browser/src/adapters/inner-iframe-bootstrap.ts`:
- Around line 152-156: The message listener in
window.addEventListener('message', function(event) { ... }) accepts matching
requestId from any source; restrict it to only handle messages from the outer
frame by validating event.source === window.parent (or abort if not) before
checking data.__enclave_msg__ and requestId so only parent frame
responses/aborts are processed; update the listener in inner-iframe-bootstrap.ts
(the anonymous handler attached to window) to short-circuit when event.source
!== window.parent.
In `@libs/browser/src/browser-enclave.ts`:
- Around line 314-326: The executionContext currently always sets
suspiciousPatterns to DEFAULT_SERIALIZED_PATTERNS which ignores user-provided
patterns; modify the code that builds executionContext (symbol:
executionContext) to prefer the serialized user-configured suspicious patterns
(e.g., serializedConfig.doubleIframe?.parentValidation?.suspiciousPatterns or
validationConfig.suspiciousPatterns) and only fall back to
DEFAULT_SERIALIZED_PATTERNS when none are present, preserving existing
serialization/format expectations; ensure you reference the same serialized
value used elsewhere so doubleIframe.parentValidation.suspiciousPatterns
actually propagates into executionContext.suspiciousPatterns.
---
Nitpick comments:
In `@libs/browser/src/browser-enclave.ts`:
- Around line 426-428: The current early-return clones only the top-level
DEFAULT_DOUBLE_IFRAME_CONFIG (return { ...DEFAULT_DOUBLE_IFRAME_CONFIG })
causing nested objects like parentValidation to be shared across instances;
update the no-options path to deep-clone the default config (e.g., use
structuredClone(DEFAULT_DOUBLE_IFRAME_CONFIG) or a shallow spread that also
clones nested objects such as parentValidation) so each caller gets an
independent DEFAULT_DOUBLE_IFRAME_CONFIG copy; keep references to
DEFAULT_DOUBLE_IFRAME_CONFIG and parentValidation in your change to locate the
return site.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/browser-demo/src/App.tsxdocs/docs.jsondocs/enclave/core-libraries/enclave-browser/configuration.mdxdocs/enclave/core-libraries/enclave-browser/overview.mdxdocs/enclave/core-libraries/enclave-browser/react-integration.mdxdocs/enclave/core-libraries/enclave-browser/security-architecture.mdxlibs/browser/e2e/fixtures/debug-inner.htmllibs/browser/e2e/fixtures/debug-inner2.htmllibs/browser/e2e/fixtures/debug-outer.htmllibs/browser/src/adapters/inner-iframe-bootstrap.tslibs/browser/src/adapters/outer-iframe-bootstrap.tslibs/browser/src/browser-enclave.ts
✅ Files skipped from review due to trivial changes (1)
- docs/enclave/core-libraries/enclave-browser/configuration.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/browser/e2e/fixtures/debug-outer.html
docs/enclave/core-libraries/enclave-browser/security-architecture.mdx
Outdated
Show resolved
Hide resolved
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.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores