Skip to content

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • updated rce to use 'isolate' pkg for RCE for true isolation

Type of Change

  • Other: Security

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Dec 15, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Review Updated (UTC)
docs Skipped Skipped Dec 15, 2025 8:54pm

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 15, 2025

Greptile Overview

Greptile Summary

Migrated JavaScript code execution from Node.js vm module to isolated-vm package for stronger sandboxing and security isolation.

Key Changes

  • Replaced vm.createContext() with isolated-vm V8 isolate creation (128MB memory limit)
  • Moved secure fetch implementation into isolated-vm.ts with SSRF validation
  • Added comprehensive security tests for constructor chain escapes, process/require blocking
  • Fixed TypeScript build errors in docs app (unrelated type fixes for fumadocs)

Security Improvements

  • True V8 isolation prevents prototype chain escapes to access process.env or require
  • Dangerous globals (process, require, module, etc.) explicitly undefined in bootstrap
  • Secure fetch wrapper validates URLs before allowing requests
  • Tests verify blocking of SSRF attacks and file system access attempts

Issues Found

  • Multiple JSON.parse/JSON.stringify calls without error handling could crash isolate (lines 159, 161, 177)
  • Global variable blocking uses simple assignment which can be bypassed; stronger protection recommended
  • Security test validation could be more assertive about expected error responses

Confidence Score: 3/5

  • This PR significantly improves security isolation but has several unhandled JSON parsing errors that could crash the isolate or cause unexpected behavior
  • The migration to isolated-vm is architecturally sound and addresses critical security concerns with the previous vm implementation. However, the implementation has multiple logic errors where JSON.parse/stringify operations lack error handling (lines 159, 161, 177 in isolated-vm.ts), which could cause runtime failures. These are definite bugs that need fixing before merge. The test coverage is comprehensive but validation could be stronger. Score of 3 reflects: good security architecture (+), comprehensive tests (+), but critical error handling gaps (-2).
  • Pay close attention to apps/sim/lib/execution/isolated-vm.ts - the JSON parsing errors need to be fixed to prevent isolate crashes

Important Files Changed

File Analysis

Filename Score Overview
apps/sim/lib/execution/isolated-vm.ts 4/5 New isolated-vm implementation for secure code execution. Properly implements sandboxing with global restrictions, secure fetch wrapper, and error handling. Minor concern: setting globals to undefined in bootstrap may be bypassable.
apps/sim/app/api/function/execute/route.ts 5/5 Migrated from Node.js vm module to isolated-vm for better security. Removed createSecureFetch function and delegated to isolated-vm.ts. Error handling and line number tracking preserved correctly.
apps/sim/app/api/function/execute/route.test.ts 5/5 Comprehensive test overhaul replacing vm mocks with isolated-vm mocks. Added robust security tests for constructor chain escapes, process/require blocking, and SSRF prevention. Tests correctly validate sandboxing.
apps/sim/package.json 5/5 Added isolated-vm@6.0.2 and binary-extensions@3.1.0 dependencies. Clean addition with no version conflicts.

Sequence Diagram

sequenceDiagram
    participant Client
    participant API as Function Execute API
    participant IVM as Isolated VM
    participant Security as Security Validation
    participant Host as Host Environment

    Client->>API: POST /api/function/execute
    Note over API: Extract code, params, envVars
    
    alt E2B Enabled & Python
        API->>Host: executeInE2B()
        Host-->>API: E2B Result
    else JavaScript Execution
        API->>IVM: executeInIsolatedVM(request)
        
        Note over IVM: Create new V8 Isolate<br/>(memoryLimit: 128MB)
        IVM->>IVM: Initialize context & jail
        IVM->>IVM: Set up console callbacks
        IVM->>IVM: Set up secure fetch wrapper
        
        Note over IVM: Bootstrap sandbox:<br/>- Define console object<br/>- Define fetch function<br/>- Undefine dangerous globals
        
        IVM->>IVM: Inject params & envVars
        IVM->>IVM: Compile & run user code
        
        alt User code calls fetch
            IVM->>Security: validateProxyUrl(url)
            Security-->>IVM: Validation result
            alt URL is valid
                IVM->>Host: fetch(url, options)
                Host-->>IVM: Response
            else SSRF detected
                IVM-->>IVM: Throw Security Error
            end
        end
        
        alt Execution success
            IVM-->>API: {result, stdout}
        else Execution error
            IVM-->>API: {error, stdout}
        end
        
        IVM->>IVM: Dispose isolate
    end
    
    API-->>Client: JSON response
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

@waleedlatif1
Copy link
Collaborator Author

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

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.

2 participants