fix: remove unsafe exec() in jwt.js#7920
Open
orbisai0security wants to merge 1 commit into
Open
Conversation
Automated security fix generated by Orbis Security AI
orbisai0security
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughThe JWT shim in the QuickJS sandbox is updated to deep-clone options across sign, verify, and decode paths using JSON serialization, and the decode path is rewritten to perform manual JWT token parsing instead of delegating to the jwt library's decode function. ChangesJWT Option Handling & Decoding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-js/src/sandbox/quickjs/shims/lib/jwt.js (1)
156-171:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDecode rewrite breaks parity with
jwt.decode— multiple behavior regressions.Swapping
jwt.decode(nativeToken, nativeOptions)for a hand-rolled parser changes the contract that callers depend on:
- Throws instead of returning
nullon malformed tokens.jwt.decodereturnsnullfor any non-parseable token; line 19 throws'jwt malformed', then re-throws into the sandbox. Existing scripts relying onif (jwt.decode(t)) { ... }will now crash.- Non-JSON payloads will crash.
jsonwebtokensupports string payloads (and offersoptions.jsonto force JSON parsing). Line 20's unconditionalJSON.parse()will throw on tokens with non-JSON payloads, e.g. plain-string tokens.base64vsbase64url. JWT segments are base64url-encoded. Lines 20 and 24 use'base64'; the correct encoding is'base64url'(Node ≥ 15.7.0).options.jsonis silently ignored (destructured intonativeOptionson line 14 but never checked).The deep-clone pattern on line 14 using
JSON.parse(JSON.stringify())is reasonable for sandbox isolation, but considerstructuredClone()(Node 17+) for more robust handling of edge case values.🛠️ Suggested fix preserving parity with
jsonwebtoken'sdecode- const parts = String(nativeToken).split('.'); - if (parts.length !== 3) throw new Error('jwt malformed'); - const payload = JSON.parse(Buffer.from(parts[1], 'base64').toString('utf8')); - const complete = nativeOptions && nativeOptions.complete; - let decoded; - if (complete) { - const header = JSON.parse(Buffer.from(parts[0], 'base64').toString('utf8')); - decoded = { header, payload, signature: parts[2] }; - } else { - decoded = payload; - } - return marshallToVm(decoded, vm); + const parts = String(nativeToken).split('.'); + if (parts.length !== 3) { + return marshallToVm(null, vm); + } + const decodeSegment = (seg) => Buffer.from(seg, 'base64url').toString('utf8'); + const forceJson = nativeOptions && nativeOptions.json; + const rawPayload = decodeSegment(parts[1]); + let payload; + try { + payload = JSON.parse(rawPayload); + } catch (e) { + if (forceJson) throw e; + payload = rawPayload; + } + const complete = nativeOptions && nativeOptions.complete; + const decoded = complete + ? { header: JSON.parse(decodeSegment(parts[0])), payload, signature: parts[2] } + : payload; + return marshallToVm(decoded, vm);🤖 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/sandbox/quickjs/shims/lib/jwt.js` around lines 156 - 171, The new hand-rolled parser in packages/bruno-js/src/sandbox/quickjs/shims/lib/jwt.js breaks parity with jsonwebtoken.decode; update the decode logic (the try/catch block that reads nativeToken, parts, payload, complete and nativeOptions) to: 1) treat malformed tokens by returning null instead of throwing (so if parts.length !== 3 return marshallToVm(null, vm)); 2) decode segments using base64url (use Buffer.from(segment.replace(/-/g,'+').replace(/_/g,'/'), 'base64') or Buffer.from(..., 'base64url') when supported) not plain 'base64'; 3) only JSON.parse the payload when nativeOptions && nativeOptions.json is truthy (otherwise leave the payload as the raw string and return that); 4) still honor nativeOptions.complete to return { header, payload, signature } (parsing header with base64url and only JSON.parse(header) when json option applies as jsonwebtoken does), and finally ensure errors only wrap unexpected failures with vm.newError while preserving the decode contract (return null for parseable-but-invalid inputs instead of throwing).
🧹 Nitpick comments (1)
packages/bruno-js/src/sandbox/quickjs/shims/lib/jwt.js (1)
17-17: ⚡ Quick winReplace
JSON.parse(JSON.stringify(vm.dump(...)))withstructuredClone()for faithful option cloning.The current approach silently mutates real-world JWT option values:
Buffer→{ type: 'Buffer', data: [...] },Date→ ISO string (losesinstanceof Date), andundefinedproperties are dropped.structuredClone()preserves these types while remaining pollution-safe after crossing the QuickJS boundary. Bruno's Node v22.12.0 baseline well exceeds the v17+ requirement.♻️ Suggested change (apply at all three sites)
- nativeOptions = JSON.parse(JSON.stringify(vm.dump(options))); + nativeOptions = structuredClone(vm.dump(options));Applies to: lines 17, 88, 153
🤖 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/sandbox/quickjs/shims/lib/jwt.js` at line 17, Replace the fragile deep-clone using JSON.parse(JSON.stringify(vm.dump(options))) with structuredClone(vm.dump(options)) at each occurrence (the nativeOptions assignments around lines 17, 88, and 153) so Buffer, Date, undefined, and other types are preserved when crossing the QuickJS boundary; locate the nativeOptions variable assignments in the jwt shim file and substitute structuredClone(vm.dump(options)) for the current JSON-based cloning in all three places.
🤖 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.
Outside diff comments:
In `@packages/bruno-js/src/sandbox/quickjs/shims/lib/jwt.js`:
- Around line 156-171: The new hand-rolled parser in
packages/bruno-js/src/sandbox/quickjs/shims/lib/jwt.js breaks parity with
jsonwebtoken.decode; update the decode logic (the try/catch block that reads
nativeToken, parts, payload, complete and nativeOptions) to: 1) treat malformed
tokens by returning null instead of throwing (so if parts.length !== 3 return
marshallToVm(null, vm)); 2) decode segments using base64url (use
Buffer.from(segment.replace(/-/g,'+').replace(/_/g,'/'), 'base64') or
Buffer.from(..., 'base64url') when supported) not plain 'base64'; 3) only
JSON.parse the payload when nativeOptions && nativeOptions.json is truthy
(otherwise leave the payload as the raw string and return that); 4) still honor
nativeOptions.complete to return { header, payload, signature } (parsing header
with base64url and only JSON.parse(header) when json option applies as
jsonwebtoken does), and finally ensure errors only wrap unexpected failures with
vm.newError while preserving the decode contract (return null for
parseable-but-invalid inputs instead of throwing).
---
Nitpick comments:
In `@packages/bruno-js/src/sandbox/quickjs/shims/lib/jwt.js`:
- Line 17: Replace the fragile deep-clone using
JSON.parse(JSON.stringify(vm.dump(options))) with
structuredClone(vm.dump(options)) at each occurrence (the nativeOptions
assignments around lines 17, 88, and 153) so Buffer, Date, undefined, and other
types are preserved when crossing the QuickJS boundary; locate the nativeOptions
variable assignments in the jwt shim file and substitute
structuredClone(vm.dump(options)) for the current JSON-based cloning in all
three places.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1f0eb05-6910-4da9-9f71-cb76274ed105
📒 Files selected for processing (1)
packages/bruno-js/src/sandbox/quickjs/shims/lib/jwt.js
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.
Summary
Fix critical severity security issue in
packages/bruno-js/src/sandbox/quickjs/shims/lib/jwt.js.Vulnerability
V-003packages/bruno-js/src/sandbox/quickjs/shims/lib/jwt.js:1Description: Bruno executes pre-request and post-response JavaScript scripts embedded in .bru collection files inside a QuickJS sandbox. The sandbox exposes shims for axios, JWT, and other Node.js libraries. Evidence in bruno-testbench.json:1958 confirms that scripts can make arbitrary external HTTP calls, modify request bodies, and change request methods. If the QuickJS sandbox isolation is incomplete — through shim escape techniques, prototype pollution, or unpatched QuickJS CVEs — a malicious collection file obtained from an untrusted source can execute arbitrary code in the Electron main process context with full system access.
Changes
packages/bruno-js/src/sandbox/quickjs/shims/lib/jwt.jsVerification
Automated security fix by OrbisAI Security
Summary by CodeRabbit