feat(collections): update collection variables event handling#8337
Open
naman-bruno wants to merge 1 commit into
Open
feat(collections): update collection variables event handling#8337naman-bruno wants to merge 1 commit into
naman-bruno wants to merge 1 commit into
Conversation
naman-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-bruno
as code owners
Contributor
WalkthroughThis PR re-enables previously stubbed collection variable and global environment variable mutation APIs ( ChangesCollection & Global Variable API End-to-End
Sequence Diagram(s)sequenceDiagram
participant Bru as Bru class
participant ScriptRuntime as ScriptRuntime/TestRuntime
participant ElectronNet as network/index.js
participant IpcHook as useIpcEvents
participant Redux as Redux Store
Bru->>Bru: setCollectionVar/deleteCollectionVar/etc.
ScriptRuntime->>ElectronNet: collectionVariables in script/test result
ElectronNet->>ElectronNet: syncCollectionVarsToCollection(collectionVariables)
ElectronNet->>IpcHook: emit main:collection-variables-update
IpcHook->>Redux: dispatch collectionVariablesUpdateEvent
Redux->>Redux: _applyCollectionVariablesUpdate merge/drop vars
Redux->>Redux: saveCollectionSettings silent persist to disk
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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: 3
🧹 Nitpick comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.spec.js (1)
187-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one test for the “new key insertion” reducer branch.
Current cases cover update/removal and no-op, but not the
incomingKeys.forEachpath that creates new vars with default metadata. A focused test here will lock inuid,enabled, and inferreddataTypebehavior.Suggested test addition
+ it('adds brand-new vars with defaults when script introduces a new key', () => { + const state = stateWith([{ uid: 'v1', name: 'keep', value: 'old', enabled: true }]); + + const next = reducer( + state, + _applyCollectionVariablesUpdate({ + collectionUid: 'col1', + collectionVariables: { keep: 'old', new_flag: false } + }) + ); + + const byName = Object.fromEntries( + next.collections[0].draft.root.request.vars.req.map((v) => [v.name, v]) + ); + + expect(byName.new_flag).toMatchObject({ name: 'new_flag', value: false, enabled: true, dataType: 'boolean' }); + expect(byName.new_flag.uid).toBeTruthy(); + });As per coding guidelines: “Add tests for any new functionality or meaningful changes” and “Cover both the happy path and the realistically problematic paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/index.spec.js` around lines 187 - 250, Add a new test case within the _applyCollectionVariablesUpdate describe block that tests the "new key insertion" scenario where collection variables are added that do not already exist in the initial state. The test should use stateWith to create initial vars, then call the reducer with _applyCollectionVariablesUpdate passing collectionVariables containing new keys that weren't in the original state, and verify that the reducer creates these new variables with proper uid generation, enabled status set to true by default, and correct inferred dataType based on the value type (number for numeric values, boolean for boolean values, undefined for strings).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js`:
- Around line 2555-2563: The collectionVariablesUpdateEvent function
unconditionally persists collection settings whenever collectionVariables is
present, causing unnecessary disk writes even when variables haven't changed.
Before dispatching saveCollectionSettings at the end of the function, first
retrieve the existing collection variables from the collection object and
compare them with the incoming collectionVariables parameter. Only dispatch the
saveCollectionSettings call if the variables have actually changed, thereby
avoiding no-op persistence operations that can accumulate on hot request
execution paths.
In `@packages/bruno-electron/src/ipc/network/index.js`:
- Around line 510-532: The syncCollectionVarsToCollection function performs a
destructive merge that replaces root.request.vars.req with only variables
present in the collectionVariables parameter, which contains only enabled
variables (filtered upstream by mergeVars). This silently deletes any disabled
variables that existed before. Verify whether this behavior is intentional by
either preserving disabled variables from the existing root.request.vars.req
array during the merge operation (keeping disabled vars that aren't in the
incoming collectionVariables), or by modifying the upstream logic to pass the
complete snapshot of all variables instead of just the enabled-only projection.
In `@packages/bruno-js/src/bru.js`:
- Around line 263-269: Fix the crash vulnerability in deleteAllGlobalEnvVars()
and deleteAllCollectionVars() methods by using
Object.prototype.hasOwnProperty.call() instead of calling hasOwnProperty()
directly on the object, since a property named hasOwnProperty would shadow the
method and cause a TypeError. Additionally, add key validation to both
setGlobalEnvVar() and setCollectionVar() methods to reject dangerous keys like
__proto__, constructor, prototype, and hasOwnProperty before they are set,
preventing prototype poisoning attacks.
---
Nitpick comments:
In
`@packages/bruno-app/src/providers/ReduxStore/slices/collections/index.spec.js`:
- Around line 187-250: Add a new test case within the
_applyCollectionVariablesUpdate describe block that tests the "new key
insertion" scenario where collection variables are added that do not already
exist in the initial state. The test should use stateWith to create initial
vars, then call the reducer with _applyCollectionVariablesUpdate passing
collectionVariables containing new keys that weren't in the original state, and
verify that the reducer creates these new variables with proper uid generation,
enabled status set to true by default, and correct inferred dataType based on
the value type (number for numeric values, boolean for boolean values, undefined
for strings).
🪄 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: 440ea1b5-68f7-4c00-9744-7bbba23c9038
📒 Files selected for processing (22)
packages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/global-environments.jspackages/bruno-app/src/utils/codemirror/autocomplete.jspackages/bruno-converters/src/postman/postman-translations.jspackages/bruno-converters/src/utils/bruno-to-postman-translator.jspackages/bruno-converters/src/utils/postman-to-bruno-translator.jspackages/bruno-converters/tests/bruno/bruno-to-postman-translations/variables.test.jspackages/bruno-converters/tests/postman/postman-translations/postman-comments.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/multiline-syntax.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/variables.test.jspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-js/src/bru.jspackages/bruno-js/src/runtime/script-runtime.jspackages/bruno-js/src/runtime/test-runtime.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.js
Comment on lines
+2555
to
+2563
| export const collectionVariablesUpdateEvent = ({ collectionUid, collectionVariables }) => (dispatch, getState) => { | ||
| if (!collectionVariables) return Promise.resolve(); | ||
| const state = getState(); | ||
| const collection = findCollectionByUid(state.collections.collections, collectionUid); | ||
| if (!collection) return Promise.resolve(); | ||
|
|
||
| dispatch(_applyCollectionVariablesUpdate({ collectionUid, collectionVariables })); | ||
| return dispatch(saveCollectionSettings(collectionUid, null, true)); | ||
| }; |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid unconditional saves for unchanged collection vars.
At Line [2562], this always persists collection settings whenever collectionVariables is present. Because this event is emitted on script execution flow, unchanged snapshots can still trigger disk writes repeatedly on request hot paths.
Proposed guard (skip no-op persistence)
export const collectionVariablesUpdateEvent = ({ collectionUid, collectionVariables }) => (dispatch, getState) => {
if (!collectionVariables) return Promise.resolve();
const state = getState();
const collection = findCollectionByUid(state.collections.collections, collectionUid);
if (!collection) return Promise.resolve();
+
+ const currentVars = collection?.draft?.root?.request?.vars?.req || collection?.root?.request?.vars?.req || [];
+ const unchanged =
+ currentVars.length === Object.keys(collectionVariables).length
+ && currentVars.every((v) => Object.prototype.hasOwnProperty.call(collectionVariables, v.name) && collectionVariables[v.name] === v.value);
+ if (unchanged) return Promise.resolve();
dispatch(_applyCollectionVariablesUpdate({ collectionUid, collectionVariables }));
return dispatch(saveCollectionSettings(collectionUid, null, true));
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js`
around lines 2555 - 2563, The collectionVariablesUpdateEvent function
unconditionally persists collection settings whenever collectionVariables is
present, causing unnecessary disk writes even when variables haven't changed.
Before dispatching saveCollectionSettings at the end of the function, first
retrieve the existing collection variables from the collection object and
compare them with the incoming collectionVariables parameter. Only dispatch the
saveCollectionSettings call if the variables have actually changed, thereby
avoiding no-op persistence operations that can accumulate on hot request
execution paths.
Comment on lines
+510
to
+532
| const syncCollectionVarsToCollection = ({ collection, collectionVariables }) => { | ||
| if (!collection || !collectionVariables) return; | ||
| const applyTo = (root) => { | ||
| if (!root) return; | ||
| root.request = root.request || {}; | ||
| root.request.vars = root.request.vars || {}; | ||
| const existing = root.request.vars.req || []; | ||
| const incoming = new Map(Object.entries(collectionVariables)); | ||
| const next = []; | ||
| for (const v of existing) { | ||
| if (!incoming.has(v.name)) continue; | ||
| next.push({ ...v, value: incoming.get(v.name) }); | ||
| incoming.delete(v.name); | ||
| } | ||
| for (const [name, value] of incoming) { | ||
| next.push({ uid: uuid(), name, value, enabled: true, type: 'text' }); | ||
| } | ||
| root.request.vars.req = next; | ||
| }; | ||
| if (collection.draft?.root) applyTo(collection.draft.root); | ||
| if (collection.root) applyTo(collection.root); | ||
| }; | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate prepareRequest and request variable construction:"
rg -n --type=js -C5 "prepareRequest\\s*\\(|const prepareRequest|function prepareRequest" packages/bruno-electron/src
echo
echo "Trace how request.collectionVariables is built:"
rg -n --type=js -C6 "collectionVariables" packages/bruno-electron/src
echo
echo "Check whether vars.req/collection vars are filtered by enabled state upstream:"
rg -n --type=js -C6 "vars\\.req|enabled" packages/bruno-electron/srcRepository: usebruno/bruno
Length of output: 50371
🏁 Script executed:
# Search for scriptResult.collectionVariables assignment and construction
rg -n --type=js "scriptResult.*collectionVariables|collectionVariables.*scriptResult" packages/bruno-electron/src/
echo "---"
# Look for where collectionVariables is set in test/script execution
rg -n --type=js -B5 -A5 "collectionVariables:" packages/bruno-electron/src/ipc/ | head -100
echo "---"
# Check script execution context - look for test/pre-post script runners
fd -t f "*.js" packages/bruno-electron/src | xargs grep -l "preRequestScript\|postRequestScript\|scriptResult" | head -5
echo "---"
# Look for the actual script execution result building
rg -n --type=js -B10 "collectionVariables.*=" packages/bruno-electron/src/ipc/network/index.js | grep -A10 "scriptResult"Repository: usebruno/bruno
Length of output: 10606
🏁 Script executed:
# Find where scriptResult is built - search for script execution
rg -n --type=js -B20 "scriptResult\s*=" packages/bruno-electron/src/ipc/network/index.js | head -150
echo "---"
# Look for the script runner or execution context
rg -n --type=js "executeScript|runScript|evalScript" packages/bruno-electron/src | head -20Repository: usebruno/bruno
Length of output: 1635
🏁 Script executed:
# Find ScriptRuntime class definition
fd -type f "*.js" packages/bruno-electron/src | xargs grep -l "class ScriptRuntime"
echo "---"
# Search for ScriptRuntime and its methods
fd -type f -path "*/script-runtime*" packages/bruno-electron/src
echo "---"
# Look for the actual script runtime implementation
find packages/bruno-electron/src -name "*script*runtime*" -o -name "*runtime*script*" | head -10Repository: usebruno/bruno
Length of output: 502
🏁 Script executed:
# Find ScriptRuntime class definition
rg -n "class ScriptRuntime" packages/bruno-electron/src
echo "---"
# Search for script-runtime files
find packages/bruno-electron/src -name "*script*" -type f | grep -i runtime
echo "---"
# Look for where scriptResult.collectionVariables is set from the script execution
rg -n "runRequestScript|runResponseScript" packages/bruno-electron/src | head -20Repository: usebruno/bruno
Length of output: 385
🏁 Script executed:
# Search for ScriptRuntime import
rg -n "ScriptRuntime" packages/bruno-electron/src | head -30
echo "---"
# Look for the actual import statement to understand where it comes from
rg -n "import.*ScriptRuntime|require.*ScriptRuntime" packages/bruno-electron/src
echo "---"
# Search in the common or shared packages
find packages -name "*script*runtime*" -o -name "*runtime*" | grep -E "\.(js|ts)$" | head -20Repository: usebruno/bruno
Length of output: 900
🏁 Script executed:
# Read the script-runtime.js file to understand what collectionVariables it returns
cat packages/bruno-js/src/runtime/script-runtime.js
echo "---"
# Also check the test to understand the expected behavior
rg -n "collectionVariables" packages/bruno-js/src/runtime/script-runtime.js -B5 -A5Repository: usebruno/bruno
Length of output: 12382
🏁 Script executed:
# Now check what the Bru class does with collectionVariables during script execution
rg -n "class Bru" packages/bruno-js/src -A 50 | head -80
echo "---"
# Look for setVar or any mutation of collectionVariables in the Bru context
rg -n "collectionVariables" packages/bruno-js/src -B2 -A2 | grep -E "this\.collectionVariables|bru\.collectionVariables" | head -20Repository: usebruno/bruno
Length of output: 7534
🏁 Script executed:
# Verify: does buildRequestScriptResult return bru.collectionVariables (modified by script) or the input collectionVariables?
rg -n "buildRequestScriptResult\|buildResponseScriptResult" packages/bruno-js/src/runtime/script-runtime.js -A 15
echo "---"
# Check if bru.collectionVariables is modified by scripts via setVar
rg -n "setVar\|getVar" packages/bruno-js/src/bru.js -B2 -A5
echo "---"
# Verify the initial request.collectionVariables comes from enabled-only filter
rg -n "collectionVariables\[" packages/bruno-electron/src/utils/collection.js -B3 -A3Repository: usebruno/bruno
Length of output: 373
Verify full-snapshot semantics before destructive merge in syncCollectionVarsToCollection.
This helper replaces root.request.vars.req with only names present in collectionVariables. Because collectionVariables is built upstream as an enabled-only projection (see mergeVars in packages/bruno-electron/src/utils/collection.js:86 filtering if (_var.enabled)), disabled collection variables will be silently deleted and persist that deletion via collectionVariablesUpdateEvent.
Confirm whether dropped disabled vars is intentional. If not, preserve disabled vars during merge or pass full snapshot instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-electron/src/ipc/network/index.js` around lines 510 - 532, The
syncCollectionVarsToCollection function performs a destructive merge that
replaces root.request.vars.req with only variables present in the
collectionVariables parameter, which contains only enabled variables (filtered
upstream by mergeVars). This silently deletes any disabled variables that
existed before. Verify whether this behavior is intentional by either preserving
disabled variables from the existing root.request.vars.req array during the
merge operation (keeping disabled vars that aren't in the incoming
collectionVariables), or by modifying the upstream logic to pass the complete
snapshot of all variables instead of just the enabled-only projection.
Comment on lines
+263
to
+269
| deleteAllGlobalEnvVars() { | ||
| for (let key in this.globalEnvironmentVariables) { | ||
| if (this.globalEnvironmentVariables.hasOwnProperty(key)) { | ||
| delete this.globalEnvironmentVariables[key]; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect write/delete paths on mutable variable maps:"
rg -n --type=js -C3 "setCollectionVar\\(|deleteAllCollectionVars\\(|setGlobalEnvVar\\(|deleteAllGlobalEnvVars\\(" packages/bruno-js/src/bru.js
echo
echo "Inspect direct hasOwnProperty calls on those maps:"
rg -n --type=js "globalEnvironmentVariables\\.hasOwnProperty|collectionVariables\\.hasOwnProperty" packages/bruno-js/src/bru.jsRepository: usebruno/bruno
Length of output: 1434
🏁 Script executed:
# Get full implementation of setGlobalEnvVar and setCollectionVar
sed -n '247,262p' packages/bruno-js/src/bru.js
echo "---"
sed -n '343,365p' packages/bruno-js/src/bru.js
echo "---"
# Check for variableNameRegex definition
rg -n "variableNameRegex" packages/bruno-js/src/bru.js -B2 -A2Repository: usebruno/bruno
Length of output: 1965
🏁 Script executed:
# Check how globalEnvironmentVariables and collectionVariables are initialized
sed -n '1,100p' packages/bruno-js/src/bru.js | grep -A30 "constructor"
echo "---"
# Check if these are ever populated from untrusted sources (JSON, user input)
rg -n "globalEnvironmentVariables\s*=" packages/bruno-js/src/bru.js -B2 -A2
echo "---"
# Check if collectionVariables is populated from untrusted sources
rg -n "collectionVariables\s*=" packages/bruno-js/src/bru.js -B2 -A2Repository: usebruno/bruno
Length of output: 2727
🏁 Script executed:
# Test the regex pattern to see if it allows dangerous keys
node -e "
const variableNameRegex = /^[\\w-.]*$/;
const dangerous = ['__proto__', 'constructor', 'prototype', 'hasOwnProperty'];
dangerous.forEach(key => {
console.log(\`'\${key}' matches regex: \${variableNameRegex.test(key)}\`);
});
"Repository: usebruno/bruno
Length of output: 287
🏁 Script executed:
# Check where globalEnvironmentVariables and collectionVariables are populated from
# Look for deserialization or JSON parsing
rg -n "JSON.parse|yaml.load|yaml.parse" packages/bruno-js/src/bru.js
echo "---"
# Check if these objects are ever populated from external sources (files, network, etc)
# Search the parent codebase for where Bru is instantiated
rg -rn "new Bru\(" --type=js packages/bruno-js | head -20
echo "---"
# Check if there's any object.create or Object.create with null prototype
rg -n "Object.create" packages/bruno-js/src/bru.jsRepository: usebruno/bruno
Length of output: 792
🏁 Script executed:
# Check if setGlobalEnvVar is used in ways that might pass untrusted keys
rg -n "setGlobalEnvVar" packages/bruno-js/src/bru.js -B2 -A2
echo "---"
# Verify that calling hasOwnProperty after setting hasOwnProperty as a key would fail
node -e "
const obj = {};
obj['hasOwnProperty'] = 'malicious';
try {
obj.hasOwnProperty('test');
console.log('No error');
} catch (e) {
console.log('Error:', e.message);
}
"
echo "---"
# Check for prototype pollution with __proto__
node -e "
const obj1 = {};
const obj2 = {};
obj1['__proto__'] = { polluted: true };
console.log('obj2.polluted after __proto__ assignment:', obj2.polluted);
"Repository: usebruno/bruno
Length of output: 421
🏁 Script executed:
# Verify the full risk: test if deleteAllGlobalEnvVars crashes with hasOwnProperty key
node -e "
class TestObj {
constructor() {
this.globalEnvironmentVariables = {};
}
deleteAllGlobalEnvVars() {
for (let key in this.globalEnvironmentVariables) {
if (this.globalEnvironmentVariables.hasOwnProperty(key)) {
delete this.globalEnvironmentVariables[key];
}
}
}
}
const obj = new TestObj();
obj.globalEnvironmentVariables['hasOwnProperty'] = 'poison';
try {
obj.deleteAllGlobalEnvVars();
console.log('No error - delete succeeded');
} catch (e) {
console.log('CRASH:', e.message);
}
"
echo "---"
# Confirm setGlobalEnvVar has NO regex validation unlike setCollectionVar
sed -n '247,251p' packages/bruno-js/src/bru.js
echo "(setGlobalEnvVar - no regex validation)"
echo "---"
sed -n '343,351p' packages/bruno-js/src/bru.js
echo "(setCollectionVar - has regex validation)"Repository: usebruno/bruno
Length of output: 817
Fix critical crash in variable deletion and add key validation to prevent poisoning.
deleteAllGlobalEnvVars() and deleteAllCollectionVars() will crash with TypeError if a script sets a key named hasOwnProperty, because the code calls .hasOwnProperty() on the object directly. Additionally, setGlobalEnvVar() has no validation and setCollectionVar()'s regex allows dangerous keys (__proto__, constructor, prototype, hasOwnProperty).
🔧 Suggested patch
setGlobalEnvVar(key, value) {
if (!key) {
throw new Error('Creating a env variable without specifying a name is not allowed.');
}
+ if (key === '__proto__' || key === 'constructor' || key === 'prototype' || key === 'hasOwnProperty') {
+ throw new Error(`Environment variable name: "${key}" is not allowed.`);
+ }
+
this.globalEnvironmentVariables[key] = value;
}
setCollectionVar(key, value) {
if (!key) {
throw new Error('Creating a variable without specifying a name is not allowed.');
}
+ if (key === '__proto__' || key === 'constructor' || key === 'prototype' || key === 'hasOwnProperty') {
+ throw new Error(`Variable name: "${key}" is not allowed.`);
+ }
+
if (variableNameRegex.test(key) === false) {
throw new Error(
`Variable name: "${key}" contains invalid characters!`
+ ' Names must only contain alpha-numeric characters, "-", "_", "."'
);
}
this.collectionVariables[key] = value;
}
deleteAllGlobalEnvVars() {
- for (let key in this.globalEnvironmentVariables) {
- if (this.globalEnvironmentVariables.hasOwnProperty(key)) {
- delete this.globalEnvironmentVariables[key];
- }
- }
+ for (const key of Object.keys(this.globalEnvironmentVariables)) {
+ delete this.globalEnvironmentVariables[key];
+ }
}
deleteAllCollectionVars() {
- for (let key in this.collectionVariables) {
- if (this.collectionVariables.hasOwnProperty(key)) {
- delete this.collectionVariables[key];
- }
- }
+ for (const key of Object.keys(this.collectionVariables)) {
+ delete this.collectionVariables[key];
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-js/src/bru.js` around lines 263 - 269, Fix the crash
vulnerability in deleteAllGlobalEnvVars() and deleteAllCollectionVars() methods
by using Object.prototype.hasOwnProperty.call() instead of calling
hasOwnProperty() directly on the object, since a property named hasOwnProperty
would shadow the method and cause a TypeError. Additionally, add key validation
to both setGlobalEnvVar() and setCollectionVar() methods to reject dangerous
keys like __proto__, constructor, prototype, and hasOwnProperty before they are
set, preventing prototype poisoning attacks.
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.
Description
JIRA
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Release Notes
New Features
setCollectionVar,deleteCollectionVar,deleteAllCollectionVars,getAllCollectionVars).deleteGlobalEnvVar,deleteAllGlobalEnvVars).Bug Fixes
Tests