feat: add PropertyList-based variable scoping APIs (bru.variables, bru.environment, bru.globals)#7887
Open
sanish-bruno wants to merge 4 commits intousebruno:mainfrom
Open
Conversation
…and bru.variables methods This commit modifies the translation logic to replace deprecated Postman API calls with updated bru methods for environment and variable management. The changes include updating method names in both the translation files and the corresponding test cases to ensure consistency and correctness in the translation process.
…slations and implementation This commit comments out the globals.unset and globals.clear methods in the Postman translations and the Bru class implementation, marking them as TODOs to be re-enabled once the UI sync issue is resolved. This change ensures that the code remains functional while addressing the current limitations in the UI.
This commit updates the order of sync read methods in the bru.js shims for variables, environment, and globals to ensure a consistent structure across the code. The changes enhance readability and maintainability without altering functionality.
sanish-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughReplaces flat Postman variable helpers with namespaced Bruno APIs ( Changes
Sequence Diagram(s)sequenceDiagram
participant PM as Postman Script
participant Translator as Postman→Bruno Translator
participant VM as QuickJS VM (bru shim)
participant Runtime as Bru Runtime (VariableList / backing store)
PM->>Translator: Parse & translate pm.* calls
Translator->>VM: Emit namespaced calls (bru.environment.get/set/has, bru.variables.*, bru.globals.*)
VM->>Runtime: Bridge call marshalled (sync write/read via property-list bridge)
Runtime->>Runtime: VariableList.get/set/unset/clear (interpolate/validate/persist)
Runtime-->>VM: Return values (or undefined for sync writes)
VM-->>Translator: (runtime results used in emitted code)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4 tasks
sanish-bruno
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/bruno-js/tests/variable-list.spec.js (1)
53-60: Add regression coverage for reserved keys (__proto__, etc.).Given
VariableList#setnow centralizes writes, this suite should include a blocking-case test for dangerous prototype keys to prevent future regressions.As per coding guidelines: `Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created.` and `Cover both the "happy path" and the realistically problematic paths.`Suggested test addition
test('set() allows valid key characters', () => { list.set('my-var_name.v2', 'ok'); expect(vars['my-var_name.v2']).toBe('ok'); }); + + test('set() rejects reserved prototype keys', () => { + expect(() => list.set('__proto__', 'x')).toThrow('not allowed'); + expect(() => list.set('prototype', 'x')).toThrow('not allowed'); + expect(() => list.set('constructor', 'x')).toThrow('not allowed'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/tests/variable-list.spec.js` around lines 53 - 60, The test suite is missing regression coverage for dangerous reserved prototype keys; add tests in packages/bruno-js/tests/variable-list.spec.js that exercise VariableList#set to ensure it rejects reserved keys such as "__proto__", "constructor" (and similar prototype-polluting names) by throwing the same validation error used for invalid characters; follow the existing test style (see tests 'set() validates key format' and 'set() allows valid key characters') and assert that calling list.set('__proto__', 'x') and list.set('constructor', 'x') throws the expected error to prevent prototype pollution regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-js/src/variable-list.js`:
- Line 3: The variableNameRegex and the set() logic currently allow
meta-reserved keys (e.g., "__proto__", "prototype", "constructor") which enables
prototype-pollution; update validation so the regex or the set() validator
explicitly rejects those reserved property names before assigning into the
internal object. Concretely, add a guard in the set method(s) referenced (around
the blocks at the locations matching variableNameRegex and the set
implementations) that returns or throws when key matches
/^__proto__$|^prototype$|^constructor$/ or when key is not matched by the
tightened variableNameRegex, and apply the same guard to the other
set/assignment sites noted (the blocks corresponding to lines 71-76 and 103-113)
to ensure all writes block prototype-related keys.
In `@packages/bruno-js/tests/variable-list.spec.js`:
- Around line 127-130: Rename the test titled 'has() still checks filtered keys
via dataSource' to reflect that filtered keys are not visible to has(); update
the test title string (the argument to the test() call) for the test that calls
list.has('__name__') and list.has('host') to something like "has() respects
filtered keys and returns false for filtered entries" so the description matches
the assertions in the test.
---
Nitpick comments:
In `@packages/bruno-js/tests/variable-list.spec.js`:
- Around line 53-60: The test suite is missing regression coverage for dangerous
reserved prototype keys; add tests in
packages/bruno-js/tests/variable-list.spec.js that exercise VariableList#set to
ensure it rejects reserved keys such as "__proto__", "constructor" (and similar
prototype-polluting names) by throwing the same validation error used for
invalid characters; follow the existing test style (see tests 'set() validates
key format' and 'set() allows valid key characters') and assert that calling
list.set('__proto__', 'x') and list.set('constructor', 'x') throws the expected
error to prevent prototype pollution regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2e37527-b140-46e2-a75e-82235836b8ae
📒 Files selected for processing (20)
packages/bruno-converters/src/postman/postman-translations.jspackages/bruno-converters/src/utils/postman-to-bruno-translator.jspackages/bruno-converters/tests/postman/postman-translations/postman-comments.spec.jspackages/bruno-converters/tests/postman/postman-translations/postman-edge-cases.spec.jspackages/bruno-converters/tests/postman/postman-translations/postman-variables.spec.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/combined.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/environment.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/legacy-tests-syntax.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/multiline-syntax.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/postman-references.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/request.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/response.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/testing-framework.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/variable-chaining.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/variables.test.jspackages/bruno-js/src/bru.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.jspackages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.jspackages/bruno-js/src/variable-list.jspackages/bruno-js/tests/variable-list.spec.js
| @@ -0,0 +1,117 @@ | |||
| const PropertyList = require('./property-list'); | |||
|
|
|||
| const variableNameRegex = /^[\w-.]*$/; | |||
Contributor
There was a problem hiding this comment.
Block reserved keys to prevent prototype-pollution behavior.
set() currently permits keys like __proto__ (and related meta-keys), which can mutate object prototype behavior when writing into plain objects. This is a security/correctness risk in scripting inputs.
Proposed fix
const PropertyList = require('./property-list');
const variableNameRegex = /^[\w-.]*$/;
+const reservedVariableKeys = new Set(['__proto__', 'prototype', 'constructor']);
@@
set(key, value, options) {
- if (!key) {
+ if (typeof key !== 'string' || key.length === 0) {
throw new Error('Creating a variable without specifying a name is not allowed.');
}
this.#validateKey(key);
this._variablesObj[key] = value;
@@
`#validateKey`(key) {
+ if (reservedVariableKeys.has(key)) {
+ throw new Error(`Variable name: "${key}" is not allowed.`);
+ }
if (this._validateKeyFn) {
this._validateKeyFn(key);
return;
}Also applies to: 71-76, 103-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-js/src/variable-list.js` at line 3, The variableNameRegex and
the set() logic currently allow meta-reserved keys (e.g., "__proto__",
"prototype", "constructor") which enables prototype-pollution; update validation
so the regex or the set() validator explicitly rejects those reserved property
names before assigning into the internal object. Concretely, add a guard in the
set method(s) referenced (around the blocks at the locations matching
variableNameRegex and the set implementations) that returns or throws when key
matches /^__proto__$|^prototype$|^constructor$/ or when key is not matched by
the tightened variableNameRegex, and apply the same guard to the other
set/assignment sites noted (the blocks corresponding to lines 71-76 and 103-113)
to ensure all writes block prototype-related keys.
Comment on lines
+127
to
+130
| test('has() still checks filtered keys via dataSource', () => { | ||
| // __name__ is filtered from reads, so has() won't find it | ||
| expect(list.has('__name__')).toBe(false); | ||
| expect(list.has('host')).toBe(true); |
Contributor
There was a problem hiding this comment.
Test name is misleading relative to the assertion.
The title says has() “still checks filtered keys”, but the assertion expects false for __name__. Please rename the test to reflect the actual expected behavior.
Suggested rename
- test('has() still checks filtered keys via dataSource', () => {
+ test('has() returns false for filtered keys', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-js/tests/variable-list.spec.js` around lines 127 - 130, Rename
the test titled 'has() still checks filtered keys via dataSource' to reflect
that filtered keys are not visible to has(); update the test title string (the
argument to the test() call) for the test that calls list.has('__name__') and
list.has('host') to something like "has() respects filtered keys and returns
false for filtered entries" so the description matches the assertions in the
test.
This commit introduces a new method `getGlobalEnvName` in the Bru class to retrieve the global environment name. Additionally, it updates the Bru shims to expose this method and define a property for the global environment name, enhancing the accessibility of environment information within the application.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA
Summary
Adds PropertyList-based variable scoping APIs to Bruno's scripting engine, providing a Postman-compatible interface for variable management.
New APIs
Three new namespaced objects on the
bruscripting context:bru.variables— runtime variables (equivalent topm.variables)bru.environment— active collection environment (equivalent topm.environment)bru.globals— active global environment (equivalent topm.globals)Each exposes a consistent PropertyList interface:
get(key)set(key, value){ persist: true })has(key)unset(key)clear()toObject()nameeach,map,filter,find,reduce)one,all,idx,count,indexOf,toJSON,toString)Postman mapping
pm.variables.get/set/has/unset/clear/toObjectbru.variables.get/set/has/unset/clear/toObjectpm.environment.get/set/has/unset/clear/toObject/namebru.environment.get/set/has/unset/clear/toObject/namepm.globals.get/set/has/toObjectbru.globals.get/set/has/toObjectpm.*.replaceIn()bru.interpolate()(shared, not scoped)Implementation
VariableListclass (packages/bruno-js/src/variable-list.js) — extendsPropertyListin dynamic mode, wraps the existing plain{ key: value }objects and mutates them in placebru.getVar(),bru.setEnvVar(), etc. methods remain unchanged and operate on the same underlying objectssyncWriteMethodssupport tocreatePropertyListBridge, wired all three VariableList instances with full bridge support.get()andtoObject()routed throughsyncReadObjectMethodsto safely handle object values across the VM boundaryglobals.name—getGlobalEnvironmentVariables()inbruno-appnow injects__name__(mirroring how collection environments work), exposed asbru.globals.namegetterpm.environment.has/pm.globals.hastransformations (now simple 1:1 translations). Filled in previously missing translations (unset,clear,hasfor globals/variables)Files changed
packages/bruno-js/src/variable-list.jspackages/bruno-js/src/bru.jsvariables,environment,globalsinstances +getGlobalEnvName()packages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.jssyncWriteMethodssupportpackages/bruno-js/src/sandbox/quickjs/shims/bru.jsnamegetterspackages/bruno-app/src/utils/collections/index.js__name__into global environment variablespackages/bruno-converters/src/utils/postman-to-bruno-translator.jspackages/bruno-converters/src/postman/postman-translations.jspackages/bruno-js/tests/variable-list.spec.jspackages/bruno-converters/tests/**Contribution Checklist:
Summary by CodeRabbit
New Features
get,set,has,unset,clear, and metadata operations.Tests