RD-T39 Trying to fix electron build #94
Conversation
📝 WalkthroughWalkthroughThe Electron main process is enhanced to implement custom protocol handling for production environments. Import signature expanded to include Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@electron/main.js`:
- Around line 61-75: The protocol handler for protocol.handle('app') currently
builds filePath from requestUrl.pathname and joins it with distPath allowing
path traversal; fix by resolving the candidate path using path.resolve(distPath,
filePath) (or similar) into a variable like fullPath and verify that fullPath
starts with the resolved distPath (e.g., path.resolve(distPath) as a prefix)
before calling net.fetch; if the check fails, return an error response (or a
404) instead of reading the file. Ensure you update the logic around filePath,
fullPath, distPath and protocol.handle to perform the resolve-and-validate step
to confine requests to dist/.
🧹 Nitpick comments (1)
electron/main.js (1)
34-36: DisablingcontextIsolationandwebSecurityin dev is risky.While these settings are safe in production (
nodeIntegration: false,contextIsolation: true,webSecurity: true), in dev mode the renderer gets full Node access with no context isolation and disabled web security. If the dev server ever loads untrusted content (e.g., a third-party script, an ad, or even a compromised localhost resource), it can access the full Node.js API.Consider keeping
contextIsolation: truein all modes and communicating with the main process viaipcRendererthrough the preload script. This would also make dev behavior closer to prod, reducing surprises.
| if (!isDev) { | ||
| const distPath = path.join(__dirname, '../dist'); | ||
| protocol.handle('app', (request) => { | ||
| const requestUrl = new URL(request.url); | ||
| let filePath = decodeURIComponent(requestUrl.pathname); | ||
|
|
||
| // Normalize the path: remove leading slashes/dots | ||
| filePath = filePath.replace(/^\/+/, ''); | ||
| if (!filePath || filePath === '.' || filePath === './') { | ||
| filePath = 'index.html'; | ||
| } | ||
|
|
||
| const fullPath = path.join(distPath, filePath); | ||
| return net.fetch(url.pathToFileURL(fullPath).toString()); | ||
| }); |
There was a problem hiding this comment.
Path traversal vulnerability — the protocol handler does not confine requests to dist/.
filePath is derived from the request URL's pathname and only has leading slashes stripped. A request such as app://./../../etc/passwd will resolve to a path outside distPath because path.join happily resolves .. segments. Since the scheme has supportFetchAPI: true, any JS running in the renderer (including via an XSS vector) can fetch() arbitrary local files.
Add a check that the resolved path stays within the dist/ directory:
🔒 Proposed fix to prevent path traversal
const distPath = path.join(__dirname, '../dist');
protocol.handle('app', (request) => {
const requestUrl = new URL(request.url);
let filePath = decodeURIComponent(requestUrl.pathname);
// Normalize the path: remove leading slashes/dots
filePath = filePath.replace(/^\/+/, '');
if (!filePath || filePath === '.' || filePath === './') {
filePath = 'index.html';
}
- const fullPath = path.join(distPath, filePath);
+ const fullPath = path.resolve(distPath, filePath);
+
+ // Prevent path traversal outside dist/
+ if (!fullPath.startsWith(distPath + path.sep) && fullPath !== distPath) {
+ return new Response('Forbidden', { status: 403 });
+ }
+
return net.fetch(url.pathToFileURL(fullPath).toString());
});🤖 Prompt for AI Agents
In `@electron/main.js` around lines 61 - 75, The protocol handler for
protocol.handle('app') currently builds filePath from requestUrl.pathname and
joins it with distPath allowing path traversal; fix by resolving the candidate
path using path.resolve(distPath, filePath) (or similar) into a variable like
fullPath and verify that fullPath starts with the resolved distPath (e.g.,
path.resolve(distPath) as a prefix) before calling net.fetch; if the check
fails, return an error response (or a 404) instead of reading the file. Ensure
you update the logic around filePath, fullPath, distPath and protocol.handle to
perform the resolve-and-validate step to confine requests to dist/.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit