packages/bruno-js/src/sandbox/node-vm/index.js (3)

203-215: Redundant normalization on line 204.

additionalContextRootsAbsolute entries are already normalized at lines 123-126. The path.normalize(allowedRoot) call on line 204 is redundant.

🔎 Proposed simplification

  const isWithinAllowedRoot = additionalContextRootsAbsolute.some((allowedRoot) => {
-   const normalizedAllowedRoot = path.normalize(allowedRoot);
-   const relativePath = path.relative(normalizedAllowedRoot, normalizedFilePath);
+   const relativePath = path.relative(allowedRoot, normalizedFilePath);
    return !relativePath.startsWith('..') && !path.isAbsolute(relativePath);
  });

---

187-194: JSDoc incomplete: missing @throws for out-of-root access.

The JSDoc at line 185 mentions throwing when "module is outside collection path," but the actual error message now references "allowed context roots." Consider updating the JSDoc to reflect this.

🔎 Proposed JSDoc update

- * @throws {Error} When module is outside collection path or cannot be loaded
+ * @throws {Error} When module is outside allowed context roots or cannot be loaded

---

243-250: Nested createCustomRequire recomputes additionalContextRootsAbsolute on each local module load.

This is inefficient for deeply nested local imports. Consider passing the already-computed additionalContextRootsAbsolute array to avoid redundant path resolution and normalization on every nested require.

🔎 Proposed fix

First, update loadLocalModule to receive the computed roots:

  function loadLocalModule({
    moduleName,
    collectionPath,
    scriptContext,
    localModuleCache,
    currentModuleDir,
-   additionalContextRootsAbsolute = []
+   additionalContextRootsAbsolute = [],
+   scriptingConfig
  }) {

Then update the nested createCustomRequire:

    require: createCustomRequire({
-     scriptingConfig: scriptContext.scriptingConfig || {},
+     scriptingConfig: scriptingConfig,
      collectionPath,
      scriptContext,
      currentModuleDir: moduleDir,
      localModuleCache,
-     allowScriptFilesystemAccess: get(scriptContext.scriptingConfig, 'filesystemAccess.allow', false)
+     allowScriptFilesystemAccess: get(scriptingConfig, 'filesystemAccess.allow', false)
    })

And pass scriptingConfig from loadLocalModule call site at line 132.

packages/bruno-js/src/sandbox/node-vm/index.spec.js (1)

186-209: Caching test doesn't truly verify caching behavior.

The test writes a static module with count: 1 and checks mod1.count === mod2.count. This will always be true since the file content is fixed. The test doesn't verify that the module code executed only once.

A more robust test would use a side-effect (e.g., incrementing a counter in context) to prove the module wasn't re-executed.

🔎 Suggested improvement

it('should cache loaded modules', async () => {
  fs.writeFileSync(
    path.join(collectionPath, 'cached.js'),
    `
    bru.incrementCount();
    module.exports = { loaded: true };
    `
  );

  let loadCount = 0;
  const context = {
    bru: {
      setVar: jest.fn(),
      incrementCount: () => { loadCount++; }
    },
    console: console
  };

  const script = `
    const mod1 = require('./cached');
    const mod2 = require('./cached');
    bru.setVar('same', mod1 === mod2);
  `;

  await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig: {} });

  expect(loadCount).toBe(1); // Module executed only once
  expect(context.bru.setVar).toHaveBeenCalledWith('same', true);
});

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created

Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:23.647Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.

Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.

packages/bruno-js/src/sandbox/node-vm/index.js (1)

121-126: Security gap: nested createCustomRequire recomputes allowed roots instead of inheriting them.

The nested createCustomRequire call at line 244 derives additionalContextRootsAbsolute from scriptContext.scriptingConfig || {} rather than accepting the already-computed and validated array. If scriptContext.scriptingConfig differs or is manipulated, nested modules could load from unexpected paths.

Pass additionalContextRootsAbsolute as an explicit parameter to createCustomRequire and propagate it to nested calls.

🔎 Proposed fix

Update createCustomRequire signature to accept pre-computed roots:

 function createCustomRequire({
   scriptingConfig,
   collectionPath,
   scriptContext,
   currentModuleDir = collectionPath,
   localModuleCache = new Map(),
-  allowScriptFilesystemAccess = false
+  allowScriptFilesystemAccess = false,
+  additionalContextRootsAbsolute = null
 }) {
-  const additionalContextRoots = get(scriptingConfig, 'additionalContextRoots', []);
-  const additionalContextRootsAbsolute = lodash
-    .chain(additionalContextRoots)
-    .map((acr) => (path.isAbsolute(acr) ? acr : path.join(collectionPath, acr)))
-    .map((acr) => path.normalize(acr))
-    .value();
-  additionalContextRootsAbsolute.push(path.normalize(collectionPath));
+  if (additionalContextRootsAbsolute === null) {
+    const additionalContextRoots = get(scriptingConfig, 'additionalContextRoots', []);
+    additionalContextRootsAbsolute = lodash
+      .chain(additionalContextRoots)
+      .map((acr) => (path.isAbsolute(acr) ? acr : path.join(collectionPath, acr)))
+      .map((acr) => path.normalize(acr))
+      .value();
+    additionalContextRootsAbsolute.push(path.normalize(collectionPath));
+  }

Then pass it through in nested call at line 244:

     require: createCustomRequire({
       scriptingConfig: scriptContext.scriptingConfig || {},
       collectionPath,
       scriptContext,
       currentModuleDir: moduleDir,
       localModuleCache,
-      allowScriptFilesystemAccess: get(scriptContext.scriptingConfig, 'filesystemAccess.allow', false)
+      allowScriptFilesystemAccess: get(scriptContext.scriptingConfig, 'filesystemAccess.allow', false),
+      additionalContextRootsAbsolute
     })

Also applies to: 244-251

packages/bruno-js/src/sandbox/node-vm/index.js (1)

204-216: Solid security check with clear error messaging.

The containment check using path.relative correctly identifies when a file path escapes allowed roots. The !path.isAbsolute(relativePath) guard handles Windows cross-drive scenarios.

Minor note: Line 205 re-normalizes allowedRoot, but these are already normalized at line 124. You could remove the redundant normalization, but it's harmless.

🔎 Optional: Remove redundant normalization

   const isWithinAllowedRoot = additionalContextRootsAbsolute.some((allowedRoot) => {
-    const normalizedAllowedRoot = path.normalize(allowedRoot);
-    const relativePath = path.relative(normalizedAllowedRoot, normalizedFilePath);
+    const relativePath = path.relative(allowedRoot, normalizedFilePath);
     return !relativePath.startsWith('..') && !path.isAbsolute(relativePath);
   });

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.

packages/bruno-js/src/sandbox/node-vm/index.spec.js (1)

226-251: Test validates caching but setup has a subtle issue.

The callCount increment happens at test setup time (when writeFileSync is called), not at module execution time. The test still validates caching correctly (same object reference via mod1.count === mod2.count), but the intent seems to be testing that the module code is executed only once. The current test logic works, but the callCount variable is unused after the initial assignment.

🔎 Consider simplifying the test

  describe('createCustomRequire - module caching', () => {
    it('should cache loaded modules', async () => {
-     let callCount = 0;
      fs.writeFileSync(
        path.join(collectionPath, 'cached.js'),
-       `
-       module.exports = { count: ${++callCount} };
-       `
+       'module.exports = { count: 1 };'
      );

      const script = `
        const mod1 = require('./cached');
        const mod2 = require('./cached');
-       bru.setVar('same', mod1.count === mod2.count);
+       bru.setVar('same', mod1 === mod2);
      `;

packages/bruno-js/src/sandbox/node-vm/index.js (1)

168-176: Consider handling the case where additionalContextRootsAbsolute is empty.

The error message template handles the empty case with || ' - No "additionalContextRoots" defined', but since collectionPath is always pushed (line 48), the array should never be empty in practice. The fallback text is misleading because it suggests no roots are defined when collectionPath is always included.

🔎 Consider updating fallback message

-      throw new Error(`Could not resolve module "${moduleName}": ${requireError.message}\n\nThis most likely means you did not install the module under the collection or the "additionalContextRoots" using a package manager like npm.\n\nThese are your current "additionalContextRoots":\n${additionalContextRootsAbsolute.map((root) => `  - ${root}`).join('\n') || '  - No "additionalContextRoots" defined'}`);
+      throw new Error(`Could not resolve module "${moduleName}": ${requireError.message}\n\nThis most likely means you did not install the module under the collection or the "additionalContextRoots" using a package manager like npm.\n\nThese are your current allowed context roots:\n${additionalContextRootsAbsolute.map((root) => `  - ${root}`).join('\n')}`);

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created

Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:23.647Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.

Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.