Fix/pr 7714 7715 review issues#7755
Conversation
…sbCard stub - Add missing import for OsArtifact and OsDownloads from OsDownloads.tsx - Add "All downloads" nav link pointing to #downloads anchor - Remove undefined FeaturedUsbCard stub (pre-existing bug from checkpoint commit) - All changes from releases-page agent now fully integrated on main branch Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. 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 |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Use osascript to pop native macOS auth dialog, dd writes via raw disk for speed | ||
| const ddCmd = `dd if=${imagePath} of=${rawDisk} bs=1m`; | ||
| await execFileAsync("osascript", [ | ||
| "-e", | ||
| `do shell script "${ddCmd}" with administrator privileges`, | ||
| ]); |
There was a problem hiding this comment.
osascript elevated command
imagePath and rawDisk are interpolated directly into an AppleScript do shell script "…" with administrator privileges string. Because imagePath is derived from image.id — which encodes an untrusted GitHub release tag_name and asset.name — a release asset containing a double-quote breaks out of the inner shell script and executes arbitrary commands as root. Paths used in this command must be shell-quoted before interpolation.
| await new Promise<void>((resolve, reject) => { | ||
| const proc = spawn("powershell.exe", [ | ||
| "-NonInteractive", | ||
| "-NoProfile", | ||
| "-Command", | ||
| `Start-Process diskpart.exe -ArgumentList '/s','${scriptPath}' -Verb RunAs -Wait`, | ||
| ]); | ||
| proc.on("close", (code) => { | ||
| if (code === 0) resolve(); | ||
| else reject(new Error(`diskpart exited with code ${code ?? "?"}`)); | ||
| }); | ||
| proc.on("error", reject); | ||
| }); | ||
|
|
||
| // Use dd.exe (from Git for Windows or PATH) to write the image | ||
| await new Promise<void>((resolve, reject) => { | ||
| const physicalDrive = `\\\\.\\PhysicalDrive${diskNumber}`; | ||
| const proc = spawn("powershell.exe", [ | ||
| "-NonInteractive", | ||
| "-NoProfile", | ||
| "-Command", | ||
| `Start-Process dd.exe -ArgumentList 'if=${imagePath}','of=${physicalDrive}','bs=4M','--progress' -Verb RunAs -Wait`, | ||
| ]); |
There was a problem hiding this comment.
scriptPath and imagePath are string-interpolated inside '…'-delimited PowerShell -ArgumentList values launched with -Verb RunAs (elevated). A single quote inside either path terminates the argument, allowing injection of arbitrary PowerShell tokens in an administrator process. Both paths should be escaped (PowerShell single-quote escape is '') before interpolation.
| let received = 0; | ||
| const writeStream = require("node:fs").createWriteStream(destPath); |
There was a problem hiding this comment.
require() call in ESM context
The file uses ES module import statements at the top but falls back to CommonJS require("node:fs").createWriteStream(destPath) inside the download callback. In a strict ESM environment this will throw a ReferenceError. The fix is to add createWriteStream to the top-level import. The same pattern appears in macos-backend.ts (line 273) and windows-backend.ts (line 153).
| function doRequest(requestUrl: string): void { | ||
| const protocol = requestUrl.startsWith("https://") ? https : http; | ||
| protocol | ||
| .get( | ||
| requestUrl, | ||
| { headers: { "User-Agent": "elizaos-usb-installer/1.0" } }, | ||
| (res) => { | ||
| if ( | ||
| res.statusCode === 301 || | ||
| res.statusCode === 302 || | ||
| res.statusCode === 307 || | ||
| res.statusCode === 308 | ||
| ) { | ||
| const location = res.headers.location; | ||
| if (!location) { | ||
| reject( | ||
| new Error( | ||
| `Redirect with no location header from ${requestUrl}`, | ||
| ), | ||
| ); | ||
| return; | ||
| } | ||
| doRequest(location); | ||
| return; |
There was a problem hiding this comment.
downloadFile
doRequest calls itself recursively on every 3xx redirect with no depth counter. A redirect cycle will cause an unhandled stack overflow, crashing the Electron process mid-installation. The same pattern is copied into macos-backend.ts and windows-backend.ts. A simple counter with a hard limit (e.g. 10) would prevent this.
| checksumSha256: | ||
| "0000000000000000000000000000000000000000000000000000000000000000", | ||
| sizeBytes: asset.size, | ||
| minUsbSizeBytes: Math.max(asset.size * 1.2, 8 * 1024 ** 3), | ||
| manifestVersion: 1, |
There was a problem hiding this comment.
All images returned by fetchGitHubIsoImages use a placeholder checksumSha256 of 64 zeros, and executeWritePlan in every backend explicitly skips checksum validation when this sentinel is present. The downloaded ISO is written to removable media with no integrity check, leaving it vulnerable to a MITM attack.
1 participant
Relates to
Risks
Background
What does this PR do?
What kind of change is this?
Documentation changes needed?
Testing
Where should a reviewer start?
Detailed testing steps
Greptile Summary
This PR resolves review feedback from PRs #7714 and #7715, adding new Electron-based USB installer backends for all three platforms, an AOSP Android flasher package, Solana wallet sign-up support, hardware-SKU-aware Stripe checkout redirects, and
elizaos.aidomain additions across CORS and allowed-origins lists.linux-backend.ts,macos-backend.ts,windows-backend.ts): Each backend enumerates removable drives, fetches ISO images from GitHub releases, downloads them, and writes to the selected drive using platform-native tooling. The macOS and Windows backends construct elevated shell/PowerShell commands by interpolating file paths without escaping, creating an injection risk at the privilege boundary.aosp-flasher): UsesspawnSyncwith noshell:true, so device serial numbers flow safely as execvp arguments.hardwareSku-aware success/cancel redirect URLs, expands CORS and allowed-origins to includeelizaos.aidomains, and redirects the cloud-frontend/checkoutroute toelizaos.ai/checkout.Confidence Score: 3/5
The cloud and AOSP flasher changes are safe to merge. The USB installer backends for macOS and Windows contain unescaped path interpolation inside elevated shell commands and should not ship without fixes.
The macOS backend constructs an AppleScript
do shell scriptcommand with admin privileges by interpolating a path derived from GitHub API data. The Windows backend has the same structural problem with single-quote-delimited PowerShell arguments in an elevatedStart-Process. These are real execution paths that run with elevated privileges. The AOSP flasher, Stripe checkout, and cloud frontend changes are well-implemented and carry no equivalent risk.packages/os-usb-installer/src/backend/macos-backend.ts and packages/os-usb-installer/src/backend/windows-backend.ts need shell/PowerShell argument escaping before merge.
Security Review
macos-backend.ts:imagePathfrom GitHub API data is interpolated unescaped into an AppleScriptdo shell script "…" with administrator privilegesstring. A double-quote in an asset name executes arbitrary commands as root.windows-backend.ts:scriptPathandimagePathare embedded in single-quote-delimited PowerShell-ArgumentListvalues launched with-Verb RunAs. A single quote in either path injects arbitrary PowerShell tokens in an elevated process.checksumSha256to 64 zeros for GitHub-sourced images and skip validation. A MITM attacker could serve a malicious image written directly to user drives.Important Files Changed
osascriptdd command.Start-Processcalls.require()in ESM context, and skips checksum verification.spawnSyncwithoutshell:true; device serials flow safely as execvp arguments.findOrCreateSolanaUserByWalletAddressfollowing the same race-condition-safe pattern as the EVM version.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD UI["InstallerApp.tsx"] -->|listRemovableDrives / listImages| BE subgraph BE["Platform Backend"] L["LinuxUsbInstallerBackend pkexec dd"] M["MacOsUsbInstallerBackend osascript admin"] W["WindowsUsbInstallerBackend PS RunAs"] end BE -->|createWritePlan| Plan["WritePlan"] Plan -->|executeWritePlan| Steps subgraph Steps["Write Steps"] R["resolve-image"] --> C["checksum skipped if zeros"] C --> Wr["write dd"] Wr --> V["verify"] V --> Done["complete"] end GH["GitHub Releases API"] -->|fetchGitHubIsoImages| BE style M fill:#f99,stroke:#c33 style W fill:#f99,stroke:#c33 style C fill:#ffd,stroke:#cc0Reviews (1): Last reviewed commit: "feat(os-homepage): wire OsDownloads comp..." | Re-trigger Greptile