-
Notifications
You must be signed in to change notification settings - Fork 14
refactor: replace eval with ShadowRealm for safe expression evaluation #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: replace eval with ShadowRealm for safe expression evaluation #103
Conversation
Remove dangerous eval() usage in the manual tool execution example and replace it with ShadowRealm API for secure, isolated JavaScript execution. ShadowRealm provides a separate realm with its own global object, preventing access to the host environment's APIs (no DOM, no Node.js APIs, no fetch, etc.), making it safe for evaluating untrusted expressions. Includes documentation link to TC39 proposal for contextual education. Co-Authored-By: Tom Aylott <thomas.42@gmail.com>
Original prompt from Tom |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the manual tool execution example to replace the dangerous eval() function with the ShadowRealm API for safer JavaScript expression evaluation. While the security improvement is valuable, the implementation has several critical issues with the ShadowRealm API usage that must be addressed.
Key Changes:
- Introduces a new
safeEvaluateMath()function using ShadowRealm for isolated code execution - Replaces
eval()call withsafeEvaluateMath()in the manual tool example - Adds comprehensive documentation about ShadowRealm browser/Node.js support
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ShadowRealm is a TC39 Stage 3 proposal for isolated JavaScript execution | ||
| // It creates a separate realm with its own global object, preventing access | ||
| // to the host environment's APIs (no DOM, no Node.js APIs, no fetch, etc.) | ||
| const realm = new ShadowRealm(); |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript may not have built-in type definitions for ShadowRealm since it's still a Stage 3 proposal. Consider adding a type declaration at the top of the file to avoid potential TypeScript errors:
// Type declaration for ShadowRealm (TC39 Stage 3 proposal)
declare class ShadowRealm {
constructor();
evaluate(sourceText: string): any;
importValue(specifier: string, bindingName: string): Promise<any>;
}
examples/tools-example.ts
Outdated
|
|
||
| // The expression is evaluated in complete isolation - even if malicious code | ||
| // is injected, it cannot access anything outside the shadow realm | ||
| const result = await realm.evaluate(`(${expression})`); |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ShadowRealm.prototype.evaluate() method returns synchronously, not a Promise. According to the TC39 spec (https://tc39.es/proposal-shadowrealm/#sec-shadowrealm.prototype.evaluate), this method should not be awaited. Remove the await keyword here.
const result = realm.evaluate(`(${expression})`);| const result = await realm.evaluate(`(${expression})`); | |
| const result = realm.evaluate(`(${expression})`); |
examples/tools-example.ts
Outdated
|
|
||
| // The expression is evaluated in complete isolation - even if malicious code | ||
| // is injected, it cannot access anything outside the shadow realm | ||
| const result = await realm.evaluate(`(${expression})`); |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapping the expression in parentheses allows arbitrary JavaScript expressions, not just math operations. For example, "1, console.log('hi'), 2" would be valid. Consider validating the expression contains only allowed characters (digits, operators, parentheses, whitespace) before evaluation:
// Validate expression contains only safe math characters
if (!/^[\d\s+\-*/.()]+$/.test(expression)) {
throw new Error('Expression contains invalid characters');
}
const result = realm.evaluate(expression);
examples/tools-example.ts
Outdated
| * Node.js support: Available behind --experimental-shadow-realm flag (v19+) | ||
| * See: https://tc39.es/proposal-shadowrealm/ | ||
| */ | ||
| async function safeEvaluateMath(expression: string): Promise<number> { |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function signature declares async and returns Promise<number>, but since ShadowRealm.prototype.evaluate() is synchronous (not a Promise), this function should not be async. Change to:
function safeEvaluateMath(expression: string): number {This also requires updating the call site at line 236 to remove await.
examples/tools-example.ts
Outdated
| // Safely evaluate the math expression using ShadowRealm | ||
| try { | ||
| const _result = eval(expression); | ||
| const _result = await safeEvaluateMath(expression); |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since safeEvaluateMath() should be synchronous (not async), remove the await keyword here:
const _result = safeEvaluateMath(expression);| const _result = await safeEvaluateMath(expression); | |
| const _result = safeEvaluateMath(expression); |
- Add TypeScript type declaration for ShadowRealm (Stage 3 proposal) - Make safeEvaluateMath synchronous (evaluate() is sync per TC39 spec) - Add input validation regex to only allow safe math characters - Remove unnecessary parentheses wrapping around expression - Add note about DoS vectors not being prevented by ShadowRealm Co-Authored-By: Tom Aylott <thomas.42@gmail.com>
Summary
Removes the dangerous
eval()usage from the manual tool execution example (examples/tools-example.ts) and replaces it with the ShadowRealm API for secure, isolated JavaScript execution.ShadowRealm provides a separate realm with its own global object, preventing evaluated code from accessing the host environment's APIs (no DOM, no Node.js APIs, no fetch, etc.). Combined with input validation, this provides defense-in-depth for evaluating math expressions.
Updates since last revision
Addressed PR feedback:
safeEvaluateMathsynchronous per TC39 spec (evaluate()is sync, not async)/^[\d\s+\-*/.()]+$/to only allow safe math charactersReview & Testing Checklist for Human
--experimental-shadow-realmflag. Verify the example actually executes without errors./^[\d\s+\-*/.()]+$/should only allow digits, whitespace, operators (+, -, *, /), decimal point, and parentheses. Confirm no edge cases allow malicious input.declare class ShadowRealmshould match https://tc39.es/proposal-shadowrealm/Recommended test plan:
cd examples node --experimental-shadow-realm -r ts-node/register tools-example.tsNotes
Requested by: Tom Aylott (@subtleGradient)
Link to Devin run: https://app.devin.ai/sessions/51990981c3dc4d3b99fb9f0e2e7a3702