fix(script): setting invalid value to environment value should be rejected - INS-2957 (#10249)

* fix: setting undefined to environment value should be rejected

* fix: reject setting environment value to undefined or null

* fix: comments

* fix: comments

* refactor: allow saving null to environments via script

* fix: add patch

* fix: comments

packages/insomnia-scripting-environment/src/objects/__tests__/environments.test.ts

@@ -1,6 +1,7 @@
 import { validate } from 'uuid';
-import { describe, expect, it } from 'vitest';
+import { describe, expect, it, vi } from 'vitest';
 
+import { getExistingConsole } from '../console';
 import { Environment, Variables } from '../environments';
 import { Folder, ParentFolders } from '../folders';
 import { Url } from '../urls';
@@ -118,6 +119,20 @@ describe('test Variables object', () => {
     folders.get('folder2').environment.set('value', 'folder1ValueOverride');
     expect(variables.get('value')).toEqual('folder1ValueOverride');
   });
+
+  it.each([0, '', false])('get returns a falsy local value (%s) instead of falling through to lower-precedence scopes', value => {
+    const variables = new Variables({
+      baseGlobalVars: new Environment('baseGlobals', { count: 'baseGlobals-value' }),
+      globalVars: new Environment('globals', {}),
+      environmentVars: new Environment('environments', {}),
+      collectionVars: new Environment('baseEnvironment', {}),
+      iterationDataVars: new Environment('iterationData', {}),
+      folderLevelVars: [],
+      localVars: new Environment('local', { count: value }),
+    });
+
+    expect(variables.get('count')).toBe(value);
+  });
 });
 
 describe('Environment serialization', () => {
@@ -126,3 +141,85 @@ describe('Environment serialization', () => {
     expect(JSON.stringify(environment)).toEqual(JSON.stringify({ user_id: 'abc', count: 1 }));
   });
 });
+
+describe('set accepts null/undefined but rejects NaN', () => {
+  it('Environment.set accepts null', () => {
+    const warnSpy = vi.spyOn(getExistingConsole(), 'warn');
+    const env = new Environment('test', {});
+    env.set('key', null);
+    expect(warnSpy).not.toHaveBeenCalled();
+    expect(env.has('key')).toBe(true);
+    expect(env.get('key')).toBeNull();
+    warnSpy.mockRestore();
+  });
+
+  it('Environment.set accepts undefined', () => {
+    const warnSpy = vi.spyOn(getExistingConsole(), 'warn');
+    const env = new Environment('test', {});
+    env.set('key', undefined);
+    expect(warnSpy).not.toHaveBeenCalled();
+    expect(env.has('key')).toBe(true);
+    expect(env.get('key')).toBeUndefined();
+    warnSpy.mockRestore();
+  });
+
+  it('Environment.set warns and skips on NaN', () => {
+    const warnSpy = vi.spyOn(getExistingConsole(), 'warn');
+    const env = new Environment('test', {});
+    env.set('key', Number.NaN);
+    expect(warnSpy).toHaveBeenCalledWith('Variable "key" has a NaN value');
+    expect(env.has('key')).toBe(false);
+    warnSpy.mockRestore();
+  });
+
+  it('Variables.set accepts null', () => {
+    const warnSpy = vi.spyOn(getExistingConsole(), 'warn');
+    const variables = new Variables({
+      baseGlobalVars: new Environment('baseGlobals', {}),
+      globalVars: new Environment('globals', {}),
+      environmentVars: new Environment('environments', {}),
+      collectionVars: new Environment('baseEnvironment', {}),
+      iterationDataVars: new Environment('iterationData', {}),
+      folderLevelVars: [],
+      localVars: new Environment('local', {}),
+    });
+    variables.set('key', null);
+    expect(warnSpy).not.toHaveBeenCalled();
+    expect(variables.get('key')).toBeNull();
+    warnSpy.mockRestore();
+  });
+
+  it('Variables.set accepts undefined', () => {
+    const warnSpy = vi.spyOn(getExistingConsole(), 'warn');
+    const variables = new Variables({
+      baseGlobalVars: new Environment('baseGlobals', {}),
+      globalVars: new Environment('globals', {}),
+      environmentVars: new Environment('environments', {}),
+      collectionVars: new Environment('baseEnvironment', {}),
+      iterationDataVars: new Environment('iterationData', {}),
+      folderLevelVars: [],
+      localVars: new Environment('local', {}),
+    });
+    variables.set('key', undefined);
+    expect(warnSpy).not.toHaveBeenCalled();
+    expect(variables.get('key')).toBeUndefined();
+    warnSpy.mockRestore();
+  });
+
+  it('Variables.set warns and skips on NaN', () => {
+    const warnSpy = vi.spyOn(getExistingConsole(), 'warn');
+    const variables = new Variables({
+      baseGlobalVars: new Environment('baseGlobals', {}),
+      globalVars: new Environment('globals', {}),
+      environmentVars: new Environment('environments', {}),
+      collectionVars: new Environment('baseEnvironment', {}),
+      iterationDataVars: new Environment('iterationData', {}),
+      folderLevelVars: [],
+      localVars: new Environment('local', {}),
+    });
+    variables.set('key', Number.NaN);
+    expect(warnSpy).toHaveBeenCalledWith('Variable "key" has a NaN value');
+    expect(variables.get('key')).toBeUndefined();
+    warnSpy.mockRestore();
+  });
+});

packages/insomnia-scripting-environment/src/objects/environments.ts

@@ -35,7 +35,7 @@ export class Environment {
    * It is intended to be a private field and should not be accessed directly outside the class.
    */
   private _name: string;
-  private kvs = new Map<string, boolean | number | string | undefined>();
+  private kvs = new Map<string, boolean | number | string | null | undefined>();
 
   /**
    * Constructs an instance of the environment object.
@@ -81,15 +81,15 @@ export class Environment {
 
   /**
    * Sets a variable in the key-value store with the specified name and value.
-   * If the provided value is `null`, a warning is logged and the variable is not set.
+   * If the provided value is `NaN`, a warning is logged and the variable is not set.
    *
    * @param variableName - The name of the variable to set.
    * @param variableValue - The value to assign to the variable. Can be a boolean, number, string, undefined, or null.
-   *                        If `null`, the variable will not be set, and a warning will be logged.
+   *                        If `NaN`, the variable will not be set, and a warning will be logged.
    */
   set = (variableName: string, variableValue: boolean | number | string | undefined | null) => {
-    if (variableValue === null) {
-      getExistingConsole().warn(`Variable "${variableName}" has a null value`);
+    if (Number.isNaN(variableValue)) {
+      getExistingConsole().warn(`Variable "${variableName}" has a NaN value`);
       return;
     }
     this.kvs.set(variableName, variableValue);
@@ -285,8 +285,7 @@ export class Variables {
    * @returns The value of the variable if found, otherwise undefined
    */
   get = (variableName: string) => {
-    let finalVal: boolean | number | string | object | undefined;
-    [
+    const scope = [
       this.localVars,
       mergeFolderLevelVars(this.folderLevelVars),
       this.iterationDataVars,
@@ -294,27 +293,22 @@ export class Variables {
       this.collectionVars,
       this.globalVars,
       this.baseGlobalVars,
-    ].forEach(vars => {
-      const value = vars.get(variableName);
-      if (!finalVal && value) {
-        finalVal = value;
-      }
-    });
+    ].find(vars => vars.has(variableName));
 
-    return finalVal;
+    return scope?.get(variableName);
   };
 
   /**
    * Sets a local variable with the specified name and value.
-   * If the provided value is `null`, a warning is logged and the variable is not set.
+   * If the provided value is `NaN`, a warning is logged and the variable is not set.
    *
    * @param variableName - The name of the variable to set.
    * @param variableValue - The value to assign to the variable. Can be a boolean, number, string, undefined, or null.
-   *                        If `null`, the variable will not be set and a warning will be logged.
+   *                        If `NaN`, the variable will not be set and a warning will be logged.
    */
   set = (variableName: string, variableValue: boolean | number | string | undefined | null) => {
-    if (variableValue === null) {
-      getExistingConsole().warn(`Variable "${variableName}" has a null value`);
+    if (Number.isNaN(variableValue)) {
+      getExistingConsole().warn(`Variable "${variableName}" has a NaN value`);
       return;
     }
 

patches/json-order+1.1.3.patch

@@ -0,0 +1,13 @@
+diff --git a/node_modules/json-order/dist/parse.js b/node_modules/json-order/dist/parse.js
+index 7d4aad8..8eda055 100644
+--- a/node_modules/json-order/dist/parse.js
++++ b/node_modules/json-order/dist/parse.js
+@@ -12,7 +12,7 @@ var traverseObject = function (obj, map, parentKey, separator) {
+     }
+     childKeys.forEach(function (childKey) {
+         var value = obj[childKey];
+-        if (typeof value === 'object') {
++        if (value !== null && typeof value === 'object') {
+             traverseObject(value, map, "" + parentKey + separator + childKey, separator);
+         }
+     });