fix(sandbox): INS-2464 additional checks

packages/insomnia/src/common/settings.ts

@@ -163,16 +163,15 @@ export interface Settings {
   saveVaultKeyToOSSecretManager: boolean;
   vaultSecretCacheDuration: number;
   dataFolders: string[];
-  // AST and shadowing check.
-  scriptSandboxEnabled: boolean;
-  // Wraps the user script in 'use strict', preventing accidental globals and making `this` undefined.
-  scriptStrictModeEnabled: boolean;
-  // Names of security rules that have been individually disabled.
-  disabledSecurityRules: string[];
-  // AST blocked-property names that have been individually disabled.
-  disabledBlockedProperties: string[];
-  // AST blocked-root names that have been individually disabled.
-  disabledBlockedRoots: string[];
+
+  // pre/post scripting sandbox related settings
+  scriptSandboxEnabled: boolean;  
+  scriptStrictModeEnabled: boolean; 
+  scriptBlockUnresolvableProperties: boolean; 
+  disabledSecurityRules: string[]; 
+  disabledBlockedProperties: string[]; 
+  disabledBlockedRoots: string[]; 
+
   /** Custom npm registry URL for plugin installation (e.g., corporate mirror). Empty string uses the default https://registry.npmjs.org/. */
   npmRegistryUrl: string;
 }

packages/insomnia/src/insomnia-data/src/models/settings.ts

@@ -79,6 +79,7 @@ export function init(): BaseSettings {
     dataFolders: [],
     scriptSandboxEnabled: true,
     scriptStrictModeEnabled: true,
+    scriptBlockUnresolvableProperties: true,
     disabledSecurityRules: [],
     disabledBlockedProperties: [],
     disabledBlockedRoots: [],

packages/insomnia/src/scripting/__tests__/sandbox.test.ts

@@ -18,15 +18,14 @@ const withoutProperty = (name: string) =>
 const withoutRoot = (name: string) =>
   new Set([...ALL_BLOCKED_ROOTS].filter(r => r !== name));
 
-// ---------------------------------------------------------------------------
-// Blocked properties — one canonical script per rule covering both dot and
-// bracket notation where applicable. The unblocking section below mirrors
-// each rule to confirm the disable path works too.
-// ---------------------------------------------------------------------------
+// Passes blockDynamic=false — mirrors disabling scriptBlockUnresolvableProperties in settings.
+const checkNoDynamic = (script: string) =>
+  () => checkSandboxViolations(script, ALL_BLOCKED_PROPERTIES, ALL_BLOCKED_ROOTS, false);
 
 describe('checkSandboxViolations', () => {
 
-  describe('blocked properties — dot notation', () => {
+  // Blocked property names accessed via dot notation (obj.constructor, etc.)
+  describe('properties: dot', () => {
     it('blocks prototype',               () => blocked('Promise.prototype.then'));
     it('blocks mainModule',              () => blocked('proc.mainModule'));
     it('blocks constructor',             () => blocked('obj.constructor'));
@@ -47,7 +46,8 @@ describe('checkSandboxViolations', () => {
     it('blocks getOwnPropertyDescriptors', () => blocked('Object.getOwnPropertyDescriptors(obj)'));
   });
 
-  describe('blocked properties — bracket notation', () => {
+  // Same property names but via static-string bracket access (obj["constructor"])
+  describe('properties: bracket', () => {
     it('blocks constructor',       () => blocked('obj["constructor"]'));
     it('blocks __proto__',         () => blocked('obj["__proto__"]'));
     it('blocks prototype',         () => blocked('Promise["prototype"]'));
@@ -56,11 +56,8 @@ describe('checkSandboxViolations', () => {
     it('blocks defineProperty',    () => blocked('Object["defineProperty"](obj, "key", desc)'));
   });
 
-  // ---------------------------------------------------------------------------
-  // Blocked roots
-  // ---------------------------------------------------------------------------
-
-  describe('blocked roots — direct member access', () => {
+  // Blocked global roots accessed directly via member expressions
+  describe('roots: member', () => {
     it('blocks this',       () => blocked('this.x'));
     it('blocks globalThis', () => blocked('globalThis.require'));
     it('blocks global',     () => blocked('global.require'));
@@ -74,23 +71,28 @@ describe('checkSandboxViolations', () => {
     it('blocks arguments',  () => blocked('arguments[0]'));
   });
 
-  describe('blocked roots — direct call', () => {
+  // Direct invocation of a blocked root identifier — caught by the CallExpression visitor
+  describe('roots: call', () => {
     it('blocks constructor called directly', () =>
       blocked('constructor("return process")()'));
+
+    it('blocks globalThis called directly', () =>
+      blocked('globalThis()'));
+
+    it('blocks aliased constructor call', () =>
+      blocked('const c = constructor; c("return process")()'));
   });
 
-  describe('blocked roots — bracket notation', () => {
+  // Blocked roots reached via bracket access — root traversal still matches
+  describe('roots: bracket', () => {
     it('blocks globalThis["require"]', () => blocked('globalThis["require"]()'));
     it('blocks window["process"]',     () => blocked('window["process"]'));
     it('blocks self["require"]',       () => blocked('self["require"]'));
     it('blocks process["env"]',        () => blocked('process["env"]'));
   });
 
-  // ---------------------------------------------------------------------------
-  // Alias chains and destructuring
-  // ---------------------------------------------------------------------------
-
-  describe('this — alias chains and destructuring', () => {
+  // ThisExpression handling: dynamic keys, const/let aliasing, and destructuring forms
+  describe('this: aliases & destructuring', () => {
     it('blocks this.process.mainModule.require via member', () =>
       blocked(`this.process.mainModule.require('child_process')`));
 
@@ -113,7 +115,8 @@ describe('checkSandboxViolations', () => {
       blocked(`({ process } = this)`));
   });
 
-  describe('globalThis — alias chains and destructuring', () => {
+  // Alias-tracking for globalThis: covers Logical/Conditional/Sequence init forms and transitive aliases
+  describe('globalThis: aliases & destructuring', () => {
     it('blocks const alias: const g = globalThis; g.require', () =>
       blocked(`const g = globalThis; g.require('child_process')`));
 
@@ -122,13 +125,34 @@ describe('checkSandboxViolations', () => {
 
     it('blocks destructuring assignment from globalThis', () =>
       blocked(`({ require } = globalThis)`));
-  });
 
-  // ---------------------------------------------------------------------------
-  // Prototype chain mutation
-  // ---------------------------------------------------------------------------
+    it('blocks renamed destructuring from globalThis', () =>
+      blocked(`const { require: r } = globalThis`));
+
+    it('blocks alias init via LogicalExpression', () =>
+      blocked(`const g = null || globalThis; g.require('child_process')`));
+
+    it('blocks alias init via ConditionalExpression', () =>
+      blocked(`const g = cond ? globalThis : null; g.require('child_process')`));
+
+    it('blocks alias init via SequenceExpression', () =>
+      blocked(`const g = (0, globalThis); g.require('child_process')`));
+
+    it('blocks transitive alias (alias of alias)', () =>
+      blocked(`const a = globalThis; const b = a; b.require('child_process')`));
+
+    it('blocks assignment alias via LogicalExpression', () =>
+      blocked(`let g; g = false || globalThis; g.require('child_process')`));
+
+    it('blocks computed property on aliased root', () =>
+      blocked(`const g = globalThis; g["require"]`));
+
+    it('blocks blocked root used deep inside an assignment RHS', () =>
+      blocked(`a.b.c = globalThis.process`));
+  });
 
-  describe('prototype chain mutation', () => {
+  // Reads and writes against built-in .prototype objects must be rejected
+  describe('prototype mutation', () => {
     it('blocks Promise.prototype.then mutation', () =>
       blocked(`Promise.prototype.then = function(fn) { fn.call(globalThis); }`));
 
@@ -148,10 +172,7 @@ describe('checkSandboxViolations', () => {
       blocked(`Promise['prototype']`));
   });
 
-  // ---------------------------------------------------------------------------
-  // Dynamic import
-  // ---------------------------------------------------------------------------
-
+  // Static `import` declarations and dynamic `import()` are both rejected at the AST level
   describe('import', () => {
     it('blocks dynamic import()', () =>
       blocked(`import('child_process')`));
@@ -166,20 +187,17 @@ describe('checkSandboxViolations', () => {
       blocked(`import { readFile } from 'fs'`));
   });
 
-  // ---------------------------------------------------------------------------
-  // Symbol.species
-  // ---------------------------------------------------------------------------
-
+  // Symbol.species would let user code subvert built-in subclassing — always blocked
   describe('Symbol.species', () => {
     it('blocks Symbol.species', () =>
       blocked(`Symbol.species`));
-  });
 
-  // ---------------------------------------------------------------------------
-  // Unblocking — disabling a rule must allow previously blocked scripts
-  // ---------------------------------------------------------------------------
+    it('blocks Symbol["species"]', () =>
+      blocked(`Symbol['species']`));
+  });
 
-  describe('unblocking — disabling a blocked property rule allows the script', () => {
+  // Verify each blocked property rule can be individually disabled by settings
+  describe('unblocking: properties', () => {
     const cases: [name: string, script: string][] = [
       ['prototype',                'Promise.prototype.then'],
       ['mainModule',               'proc.mainModule'],
@@ -207,7 +225,8 @@ describe('checkSandboxViolations', () => {
     }
   });
 
-  describe('unblocking — disabling a blocked root rule allows the script', () => {
+  // Verify each blocked root rule can be individually disabled and that alias tracking follows
+  describe('unblocking: roots', () => {
     const cases: [name: string, script: string][] = [
       ['this',        'this.x'],
       ['globalThis',  'globalThis.require'],
@@ -235,10 +254,34 @@ describe('checkSandboxViolations', () => {
       expect(check('const g = globalThis; g.require', ALL_BLOCKED_PROPERTIES, withoutRoot('globalThis'))).not.toThrow());
   });
 
-  // ---------------------------------------------------------------------------
-  // Allowed scripts
-  // ---------------------------------------------------------------------------
+  // Fail-closed policy: any computed key that cannot be resolved at parse time is rejected
+  describe('dynamic computed keys', () => {
+    it('blocks concatenated string key: obj["con"+"structor"]', () =>
+      blocked(`obj["con"+"structor"]`));
 
+    it('blocks variable key: const k = "constructor"; obj[k]', () =>
+      blocked(`const k = "constructor"; obj[k]`));
+
+    it('blocks unverifiable computed key: obj[someExpr]', () =>
+      blocked(`obj[someExpr]`));
+
+    it('blocks template literal with expressions: obj[`${x}`]', () =>
+      blocked('obj[`${x}`]'));
+  });
+
+  // Static-but-obfuscated keys (template literals, concatenation) must still resolve and block
+  describe('blocked properties via computed access', () => {
+    it('blocks constructor via template literal: obj[`constructor`]', () =>
+      blocked('obj[`constructor`]'));
+
+    it('blocks constructor via concatenation: obj["con"+"structor"]', () =>
+      blocked('obj["con"+"structor"]'));
+
+    it('blocks AsyncFunction constructor via template: (async()=>{})[`constructor`]', () =>
+      blocked('(async()=>{})[`constructor`]'));
+  });
+
+  // Generic JS patterns and common pm/insomnia usage that must continue to pass
   describe('allowed scripts', () => {
     it('allows normal variable declarations', () =>
       allowed(`const x = 1 + 2`));
@@ -261,7 +304,140 @@ describe('checkSandboxViolations', () => {
     it('allows console.log', () =>
       allowed(`console.log('hello')`));
 
-    it('allows class with prototype-like property name in string', () =>
-      allowed(`const key = 'prototype'; obj[key]`));
+    it('allows safe property access via string literal', () =>
+      allowed(`obj['foo']`));
+
+    it('allows safe property access via dot notation', () =>
+      allowed(`obj.foo`));
+
+    it('allows arrow functions and closures', () =>
+      allowed(`const add = (a, b) => a + b; add(1, 2)`));
+
+    it('allows class declarations and instantiation', () =>
+      allowed(`class Foo { greet() { return 'hi' } } new Foo().greet()`));
+
+    it('allows spread and rest', () =>
+      allowed(`const a = [1, 2]; const b = [...a, 3]; const fn = (...args) => args.length`));
+
+    it('allows optional chaining on user objects', () =>
+      allowed(`const x = obj?.foo?.bar`));
+
+    it('allows nullish coalescing', () =>
+      allowed(`const x = a ?? b`));
+
+    it('allows destructuring user objects and arrays', () =>
+      allowed(`const { a, b } = data; const [first] = list`));
+
+    it('allows template literals', () =>
+      allowed('const s = `hello ${name}`'));
+
+    it('allows try/catch/finally', () =>
+      allowed(`try { doThing() } catch (e) { console.log(e) } finally { cleanup() }`));
+
+    it('allows for-of loops', () =>
+      allowed(`for (const item of items) { console.log(item) }`));
+
+    it('allows JSON.parse / JSON.stringify', () =>
+      allowed(`JSON.parse(JSON.stringify({ a: 1 }))`));
+
+    it('allows Promise chains', () =>
+      allowed(`Promise.resolve(1).then(v => v + 1)`));
+
+    it('allows Math / Date / Number / String built-ins', () =>
+      allowed(`Math.max(1, Date.now()); Number('3'); String(1)`));
+
+    it('allows Array methods', () =>
+      allowed(`[1, 2, 3].map(x => x * 2).filter(x => x > 1)`));
+
+    it('allows regex literals', () =>
+      allowed(`const re = /foo/g; 'foobar'.match(re)`));
+
+    it('allows common pm patterns', () =>
+      allowed(`pm.environment.set('k', 'v'); pm.variables.get('k')`));
+
+    it('allows common insomnia patterns', () =>
+      allowed(`insomnia.request.headers.add({ key: 'X', value: '1' })`));
+  });
+
+  // With dynamic-key blocking disabled, only statically resolvable blocked names still throw
+  describe('blockDynamic=false toggle', () => {
+    it('allows concatenated string key when block-dynamic is off', () =>
+      expect(checkNoDynamic('obj["con"+"structor"]')).not.toThrow());
+
+    it('allows identifier variable key when block-dynamic is off', () =>
+      expect(checkNoDynamic('obj[someVar]')).not.toThrow());
+
+    it('allows template literal with expression when block-dynamic is off', () =>
+      expect(checkNoDynamic('obj[`${x}`]')).not.toThrow());
+
+    it('still blocks a static string literal with a blocked name even when block-dynamic is off', () =>
+      expect(checkNoDynamic('obj["constructor"]')).toThrow());
+
+    it('still blocks a no-expression template literal with a blocked name even when block-dynamic is off', () =>
+      expect(checkNoDynamic('obj[`constructor`]')).toThrow());
+  });
+
+  // These scripts intentionally pass AST checks; runtime masking in maskRules catches them.
+  describe('function-wrapper indirection (AST-pass, runtime-masked)', () => {
+    it('module indirection via function wrapper', () => {
+      allowed(`
+        const getModule = function() { return module; };
+        const m = getModule();
+        m.require('child_process');
+      `);
+    });
+
+    it('self indirection via function wrapper', () => {
+      allowed(`
+        const getSelf = function() { return self; };
+        const w = getSelf();
+        w.require('child_process');
+      `);
+    });
+
+    it('exports indirection via function wrapper', () => {
+      allowed(`
+        const getExports = function() { return exports; };
+        const e = getExports();
+        e.foo = 'bar';
+      `);
+    });
+
+    it('Buffer indirection via function wrapper', () => {
+      allowed(`
+        const getBuf = function() { return Buffer; };
+        const b = getBuf();
+        b.allocUnsafe(256);
+      `);
+    });
+
+    it('frames indirection via function wrapper', () => {
+      allowed(`
+        const getFrames = function() { return frames; };
+        const f = getFrames();
+        f[0];
+      `);
+    });
+  });
+
+  // Demonstrates the trade-off when block-dynamic is disabled: AsyncFunction reachable via concat
+  describe('AsyncFunction constructor via computed access', () => {
+    it('blocks AsyncFunction constructor with blockDynamic=true (default)', () => {
+      expect(() => checkSandboxViolations(
+        `(async () => {})['con' + 'structor']`,
+        ALL_BLOCKED_PROPERTIES,
+        ALL_BLOCKED_ROOTS,
+        true
+      )).toThrow();
+    });
+
+    it('allows AsyncFunction constructor access when blockDynamic=false', () => {
+      expect(() => checkSandboxViolations(
+        `(async () => {})['con' + 'structor']`,
+        ALL_BLOCKED_PROPERTIES,
+        ALL_BLOCKED_ROOTS,
+        false
+      )).not.toThrow();
+    });
   });
 });