Skip to content

Conversation

@subtleGradient
Copy link
Contributor

@subtleGradient subtleGradient commented Dec 3, 2025

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:

  • Added TypeScript type declaration for ShadowRealm (Stage 3 proposal)
  • Made safeEvaluateMath synchronous per TC39 spec (evaluate() is sync, not async)
  • Added input validation regex /^[\d\s+\-*/.()]+$/ to only allow safe math characters
  • Removed unnecessary parentheses wrapping around expression
  • Added doc comment noting that ShadowRealm doesn't prevent DoS vectors like infinite loops

Review & Testing Checklist for Human

  • Test the example runs - Requires Node.js with --experimental-shadow-realm flag. Verify the example actually executes without errors.
  • Verify regex validation is correct - The regex /^[\d\s+\-*/.()]+$/ should only allow digits, whitespace, operators (+, -, *, /), decimal point, and parentheses. Confirm no edge cases allow malicious input.
  • Confirm type declaration matches spec - The manually added declare class ShadowRealm should match https://tc39.es/proposal-shadowrealm/

Recommended test plan:

cd examples
node --experimental-shadow-realm -r ts-node/register tools-example.ts

Notes

  • ShadowRealm is a TC39 Stage 3 proposal with limited runtime support (Chrome 124+, Node.js 19+ with flag)
  • The code includes documentation comments with browser/Node.js support info and a link to the spec for developer education
  • While ShadowRealm isolates from host APIs, it doesn't prevent DoS vectors like infinite loops (documented in code comments)

Requested by: Tom Aylott (@subtleGradient)
Link to Devin run: https://app.devin.ai/sessions/51990981c3dc4d3b99fb9f0e2e7a3702

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>
@devin-ai-integration
Copy link
Contributor

Original prompt from Tom
@Devin <https://github.com/OpenRouterTeam/typescript-sdk/blob/main/examples/tools-example.ts#L209> uses `eval` in an example; this is absolutely unacceptable; refactor this example to demonstrate the functionality of our typescript sdk without indirectly communicating unfathomably egregious security violations that would cause prospective users to throw out the baby with this toxic bathwater. Thanks! you got this !! WE BELIEVE IN YOU
Thread URL: https://openrouter.slack.com/archives/C08N1M9AXU2/p1764771329629299

<https://github.com/OpenRouterTeam/typescript-sdk/blob/main/examples/tools-example.ts|tools-example.ts>

@devin-ai-integration
Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copilot AI review requested due to automatic review settings December 3, 2025 14:22
Copy link

Copilot AI left a 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 with safeEvaluateMath() 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();
Copy link

Copilot AI Dec 3, 2025

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>;
}

Copilot uses AI. Check for mistakes.

// 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})`);
Copy link

Copilot AI Dec 3, 2025

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})`);
Suggested change
const result = await realm.evaluate(`(${expression})`);
const result = realm.evaluate(`(${expression})`);

Copilot uses AI. Check for mistakes.

// 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})`);
Copy link

Copilot AI Dec 3, 2025

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);

Copilot uses AI. Check for mistakes.
* Node.js support: Available behind --experimental-shadow-realm flag (v19+)
* See: https://tc39.es/proposal-shadowrealm/
*/
async function safeEvaluateMath(expression: string): Promise<number> {
Copy link

Copilot AI Dec 3, 2025

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.

Copilot uses AI. Check for mistakes.
// Safely evaluate the math expression using ShadowRealm
try {
const _result = eval(expression);
const _result = await safeEvaluateMath(expression);
Copy link

Copilot AI Dec 3, 2025

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);
Suggested change
const _result = await safeEvaluateMath(expression);
const _result = safeEvaluateMath(expression);

Copilot uses AI. Check for mistakes.
- 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>
@mattapperson mattapperson merged commit 47a2ce2 into main Dec 3, 2025
3 checks passed
@mattapperson mattapperson deleted the devin/1764771465-remove-eval-from-tools-example branch December 3, 2025 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants