🛡️ Sentinel: [CRITICAL] Fix PowerShell command injection in audio playback#286
🛡️ Sentinel: [CRITICAL] Fix PowerShell command injection in audio playback#286Dexploarer wants to merge 1 commit intodevelopfrom
Conversation
…layback This fix updates the `playAudio` function in `src/api/sandbox-routes.ts` to use Base64 encoding/decoding when passing file paths to `powershell -Command` on Windows. The previous implementation relied on standard string replacement (single-quote escaping), which can be bypassed in PowerShell, leading to arbitrary command execution. This new approach guarantees safety by transferring the payload purely via safe Base64 characters.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| const b64 = Buffer.from(tmpFile.replace(/\//g, "\\")).toString("base64"); | ||
| runCommand( | ||
| "powershell", | ||
| [ | ||
| "-Command", | ||
| `(New-Object Media.SoundPlayer '${escapedPath}').PlaySync()`, | ||
| `$path = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('${b64}')); (New-Object Media.SoundPlayer $path).PlaySync()`, | ||
| ], |
There was a problem hiding this comment.
Unnecessary Complexity: Base64 Encoding of File Path for PowerShell
The code encodes the file path as base64 and then decodes it in PowerShell to use with Media.SoundPlayer. This adds unnecessary complexity and may introduce errors if the decoding fails or if the path contains unexpected characters. Unless there is a strong reason (such as avoiding problematic characters in the path), it is preferable to use the sanitized file path directly:
runCommand(
"powershell",
[
"-Command",
`(New-Object Media.SoundPlayer '${tmpFile.replace(/\//g, "\\")}').PlaySync()`
],
60000,
);Recommendation:
Replace the base64 encoding/decoding with direct usage of the sanitized file path to improve maintainability and reduce the risk of errors.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a crucial security update by patching a critical command injection vulnerability affecting audio playback on Windows. The core change involves a robust redesign of how file paths are handled when executing PowerShell commands, moving from vulnerable string escaping to a secure Base64 encoding and decoding mechanism. This significantly enhances the system's resilience against malicious input and ensures the integrity of command execution. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent fix for the critical command injection vulnerability in playAudio. Using Base64 encoding to pass data to PowerShell is the correct and robust solution. While reviewing this change, I found the same vulnerable escaping pattern (.replace(/'/g, "''")) in two other functions within src/api/sandbox-routes.ts: performType and performKeypress. The performType function is currently exploitable. To fully address this critical security risk, I strongly recommend applying the same Base64 encoding fix to these functions as well, consistent with the excellent prevention strategy documented in the new sentinel.md file.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: A command injection vulnerability existed when passing the audio temporary file path (which incorporates the user-controllable audio format extension) to
powershell -Commandon Windows platforms viaexecSyncandexecFileSync. The code relied on standard single-quote escaping, which is insufficient for PowerShell and can be bypassed.🎯 Impact: An attacker could craft a malicious audio playback payload with a specially formatted
formatstring to break out of the command substitution and execute arbitrary commands on the host system when running on Windows.🔧 Fix: Changed the payload delivery mechanism. The file path is now encoded as a Base64 string in Node.js before being passed to PowerShell. Inside the PowerShell block, the Base64 string is decoded back to a string natively. This bypasses the need for escaping entirely, making it mathematically impossible to break out of the string boundary using quotes.
✅ Verification: Ran integration tests (
src/services/__tests__/sandbox-e2e.test.ts) and unit tests (src/api/__tests__/sandbox-routes.test.ts) successfully. A journal entry was also created documenting the learning.PR created automatically by Jules for task 3194332998388628493 started by @Dexploarer