Open
Conversation
…b to block stored XSS Both components render agent-generated markdown that does not require raw HTML passthrough. Removing allowRawHtml ensures rehype-raw is not enabled, preventing any injected HTML/script tags in history files from executing in the Electron renderer.
…path as defense-in-depth When allowRawHtml is enabled, content is now pre-sanitized with DOMPurify.sanitize() before passing to ReactMarkdown. This strips script tags, event handlers, iframe injections, and javascript: URLs even when raw HTML passthrough is active. Uses the same DOMPurify pattern already established in TerminalOutput.tsx.
…pts to external files Adds a strict CSP meta tag to index.html with script-src 'self' to block inline script injection. Moves splash screen and React DevTools scripts to external files in src/renderer/public/ for CSP compliance. Adds a Vite transformIndexHtml hook to relax the CSP in dev mode so HMR and React Refresh inline scripts continue to work.
…permission denial - Add sandbox: true to webPreferences for OS-level renderer process sandboxing - Add setWindowOpenHandler to deny all popup/new-window requests - Add will-navigate handler to block navigation away from the app - Add setPermissionRequestHandler to deny all browser permissions (camera, mic, etc.) - Add 6 new tests covering all security hardening behaviors
… http/https/mailto
📝 WalkthroughWalkthroughThis PR implements comprehensive security hardening across the main process, IPC handlers, and renderer components. Changes include enabling renderer sandboxing, blocking popups and permissions, restricting navigation by protocol, implementing path traversal protections in marketplace handlers, adding HTML sanitization via DOMPurify, enforcing Content-Security-Policy headers, and adding extensive test coverage for security features. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryComprehensive security hardening across multiple attack surfaces in the Electron application. The changes implement defense-in-depth protections including Content Security Policy, Electron sandbox mode, protocol whitelisting, path traversal guards, and XSS sanitization. Key Changes:
One Logic Issue Found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Renderer Process] -->|External URL| B{shell:openExternal}
B -->|Validate Protocol| C{Protocol Check}
C -->|http/https/mailto| D[Allow - Open in Browser]
C -->|file:/javascript:/data:| E[Reject - Throw Error]
A -->|window.open| F{setWindowOpenHandler}
F -->|Any URL| G[Deny All Popups]
A -->|Navigation Attempt| H{will-navigate Handler}
H -->|Production| I{Protocol Check}
H -->|Development| J{Origin Check}
I -->|file://| K[Allow App Navigation]
I -->|Other| L[Block - preventDefault]
J -->|Dev Server Origin| K
J -->|Other Origin| L
A -->|Permission Request| M{setPermissionRequestHandler}
M -->|camera/mic/geo/etc| N[Deny All Permissions]
A -->|Local File Read| O{marketplace:getDocument}
O -->|Validate Filename| P{Contains .. ?}
P -->|Yes| Q[Reject - Invalid Filename]
P -->|No| R{validateSafePath}
R -->|Outside Base| S[Reject - Path Traversal]
R -->|Inside Base| T[Allow - Read File]
A -->|Render Markdown| U{allowRawHtml?}
U -->|true| V[DOMPurify.sanitize]
U -->|false| W[Standard Markdown]
V -->|Strip XSS| X[Safe HTML Output]
W -->|No HTML| X
Y[index.html] -->|CSP Meta Tag| Z[Content Security Policy]
Z -->|Production| AA[Strict: script-src 'self']
Z -->|Development| AB[Relaxed: unsafe-inline for HMR]
Last reviewed commit: 7ee177a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+350
to
+356
| function validateSafePath(basePath: string, requestedFile: string): string { | ||
| const realBase = path.resolve(basePath); | ||
| const resolved = path.resolve(basePath, requestedFile); | ||
| if (!resolved.startsWith(realBase + path.sep) && resolved !== realBase) { | ||
| throw new MarketplaceFetchError(`Path traversal blocked: ${requestedFile}`); | ||
| } | ||
| return resolved; |
There was a problem hiding this comment.
edge case: validateSafePath could allow traversal if basePath="/Users/foo" and someone creates /Users/foobar/ directory, since "/Users/foobar/file" would start with "/Users/foo". should use path.relative() and check it doesn't start with .. instead
Suggested change
| function validateSafePath(basePath: string, requestedFile: string): string { | |
| const realBase = path.resolve(basePath); | |
| const resolved = path.resolve(basePath, requestedFile); | |
| if (!resolved.startsWith(realBase + path.sep) && resolved !== realBase) { | |
| throw new MarketplaceFetchError(`Path traversal blocked: ${requestedFile}`); | |
| } | |
| return resolved; | |
| function validateSafePath(basePath: string, requestedFile: string): string { | |
| const realBase = path.resolve(basePath); | |
| const resolved = path.resolve(basePath, requestedFile); | |
| const relative = path.relative(realBase, resolved); | |
| if (relative.startsWith('..') || path.isAbsolute(relative)) { | |
| throw new MarketplaceFetchError(`Path traversal blocked: ${requestedFile}`); | |
| } | |
| return resolved; | |
| } |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/MarkdownRenderer.tsx (1)
323-332:⚠️ Potential issue | 🟠 MajorAvoid silently swallowing URL conversion errors.
The empty catch hides unexpected issues; either remove the try/catch or report via Sentry and rethrow.🔧 Suggested fix (remove silent catch)
- // Attempt to convert non-standard URLs (e.g. git@host:user/repo) - try { - const converted = href.startsWith('git@') - ? href.replace(/^git@/, 'https://').replace(/:([^/])/, '/$1').replace(/\.git$/, '') - : href; - if (/^https?:\/\//.test(converted)) { - window.maestro.shell.openExternal(converted); - } - } catch { - // Silently ignore unparseable URLs - } + // Attempt to convert non-standard URLs (e.g. git@host:user/repo) + const converted = href.startsWith('git@') + ? href.replace(/^git@/, 'https://').replace(/:([^/])/, '/$1').replace(/\.git$/, '') + : href; + if (/^https?:\/\//.test(converted)) { + window.maestro.shell.openExternal(converted); + }As per coding guidelines, "Do NOT silently swallow exceptions with try-catch-console.error blocks. Let unhandled exceptions bubble up to Sentry for error tracking."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MarkdownRenderer.tsx` around lines 323 - 332, The current try/catch around the git-to-https conversion swallows all errors silently; remove the empty catch so errors from the conversion and the window.maestro.shell.openExternal call can bubble up to your global error handler (or, if you prefer explicit handling, catch the error, report it to Sentry/telemetry with context including the href and the converted value, then rethrow). Update the block that performs the href conversion (the ternary conversion and the subsequent /^https?:\/\// check and call to window.maestro.shell.openExternal) accordingly so no exceptions are discarded silently.
🧹 Nitpick comments (2)
src/main/app-lifecycle/window-manager.ts (1)
162-193: Good security hardening with one consideration.The navigation and permission restrictions are well-implemented:
- Popup blocking prevents malicious window.open calls
- Permission denial is appropriate for a terminal app
- Navigation restriction in dev mode correctly allows same-origin dev server requests
Minor consideration: In production mode (line 181), the check allows any
file://URL, not just the app's own files. While the risk is low given sandbox mode and other restrictions, a more precise check could verify the URL starts with the app's file path:♻️ Optional: Tighten file:// URL validation
} else { - // In production, only allow file:// protocol (the built app) - if (parsedUrl.protocol === 'file:') return; + // In production, only allow file:// URLs within the app's renderer path + if (parsedUrl.protocol === 'file:' && url.startsWith(`file://${rendererPath.replace(/\\/g, '/')}`)) return; }Note: This would require
rendererPathto be accessible in scope (it's already available fromdeps).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/app-lifecycle/window-manager.ts` around lines 162 - 193, The production navigation check in mainWindow.webContents.on('will-navigate') currently allows any file:// URL; tighten it to only permit navigation to the app's own renderer files by verifying the target URL starts with the app renderer path (use the existing rendererPath from deps) instead of just checking parsedUrl.protocol === 'file:'. Update the will-navigate handler to construct a normalized file:// base (or compare paths after converting URL to file path) using rendererPath and only return (allow) when parsedUrl.href or its resolved filesystem path is within that rendererPath; otherwise call event.preventDefault() and log as before.src/main/ipc/handlers/system.ts (1)
193-208: Good security hardening for URL protocol validation.The implementation correctly:
- Validates URL format before processing
- Restricts protocols to a safe allowlist (http:, https:, mailto:)
- Blocks dangerous schemes like
file:,javascript:, anddata:The IIFE pattern for URL parsing (lines 199-205) is functional but unconventional. A simpler approach would be:
♻️ Optional: Simplify URL parsing
- const parsed = (() => { - try { - return new URL(url); - } catch { - throw new Error(`Invalid URL: ${url}`); - } - })(); + let parsed: URL; + try { + parsed = new URL(url); + } catch { + throw new Error(`Invalid URL: ${url}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/system.ts` around lines 193 - 208, The URL parsing uses an unnecessary IIFE; simplify the parsing in the ipcMain.handle('shell:openExternal') handler by replacing the IIFE that assigns parsed with a straightforward try/catch that constructs new URL(url) and assigns to the existing parsed variable (or declares parsed) and throws the same Invalid URL error on failure; keep ALLOWED_PROTOCOLS and subsequent protocol check unchanged and ensure parsed.protocol is used afterward.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/public/splash.js`:
- Around line 7-21: Replace the direct assignments to window.onerror and
window.onunhandledrejection with event listeners registered via
window.addEventListener('error', ...) and
window.addEventListener('unhandledrejection', ...), update the handler callbacks
that reference progressText (preserve the same UI color/text behavior), and in
each handler call Sentry.captureException(...) with the error/event.reason (same
pattern used in src/renderer/main.tsx) in addition to console.error; ensure you
do not return false from the error handler so other listeners remain unaffected.
---
Outside diff comments:
In `@src/renderer/components/MarkdownRenderer.tsx`:
- Around line 323-332: The current try/catch around the git-to-https conversion
swallows all errors silently; remove the empty catch so errors from the
conversion and the window.maestro.shell.openExternal call can bubble up to your
global error handler (or, if you prefer explicit handling, catch the error,
report it to Sentry/telemetry with context including the href and the converted
value, then rethrow). Update the block that performs the href conversion (the
ternary conversion and the subsequent /^https?:\/\// check and call to
window.maestro.shell.openExternal) accordingly so no exceptions are discarded
silently.
---
Nitpick comments:
In `@src/main/app-lifecycle/window-manager.ts`:
- Around line 162-193: The production navigation check in
mainWindow.webContents.on('will-navigate') currently allows any file:// URL;
tighten it to only permit navigation to the app's own renderer files by
verifying the target URL starts with the app renderer path (use the existing
rendererPath from deps) instead of just checking parsedUrl.protocol === 'file:'.
Update the will-navigate handler to construct a normalized file:// base (or
compare paths after converting URL to file path) using rendererPath and only
return (allow) when parsedUrl.href or its resolved filesystem path is within
that rendererPath; otherwise call event.preventDefault() and log as before.
In `@src/main/ipc/handlers/system.ts`:
- Around line 193-208: The URL parsing uses an unnecessary IIFE; simplify the
parsing in the ipcMain.handle('shell:openExternal') handler by replacing the
IIFE that assigns parsed with a straightforward try/catch that constructs new
URL(url) and assigns to the existing parsed variable (or declares parsed) and
throws the same Invalid URL error on failure; keep ALLOWED_PROTOCOLS and
subsequent protocol check unchanged and ensure parsed.protocol is used
afterward.
Comment on lines
+7
to
+21
| window.onerror = function(message, source, lineno, colno, error) { | ||
| if (progressText) { | ||
| progressText.style.color = '#ff6b6b'; | ||
| progressText.textContent = 'Error: ' + (error?.message || message); | ||
| } | ||
| console.error('[Splash] Error:', message, source, lineno, colno, error); | ||
| return false; | ||
| }; | ||
| window.onunhandledrejection = function(event) { | ||
| if (progressText) { | ||
| progressText.style.color = '#ff6b6b'; | ||
| progressText.textContent = 'Error: ' + (event.reason?.message || String(event.reason)); | ||
| } | ||
| console.error('[Splash] Unhandled rejection:', event.reason); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Sentry utilities and error handling
rg -n "src/utils/sentry|captureException|captureMessage" --type=js --type=ts -A 2Repository: RunMaestro/Maestro
Length of output: 11431
🏁 Script executed:
# Search for global error handler patterns
rg -n "window\\.onerror|onunhandledrejection|addEventListener.*error|addEventListener.*unhandledrejection" --type=js --type=ts -B 2 -A 2Repository: RunMaestro/Maestro
Length of output: 1387
🏁 Script executed:
# Check the actual file and verify its contents
cat -n src/renderer/public/splash.jsRepository: RunMaestro/Maestro
Length of output: 2739
🏁 Script executed:
# Verify if src/utils/sentry exists
ls -la src/utils/sentry* 2>/dev/null || echo "File not found in src/utils/"
fd sentry src/Repository: RunMaestro/Maestro
Length of output: 115
Use addEventListener for error handlers to avoid clobbering existing handlers, and report errors to Sentry.
Direct assignment of window.onerror and window.onunhandledrejection prevents other event listeners (like those set up in main.tsx) from being triggered. Additionally, errors are only logged to console and not reported to Sentry, violating the coding guideline to use Sentry for error tracking.
Replace the direct assignments with addEventListener and report errors via Sentry.captureException() (from the Sentry SDK, following the pattern in src/renderer/main.tsx):
🔧 Example refactor
- window.onerror = function(message, source, lineno, colno, error) {
+ window.addEventListener('error', function(event) {
+ var error = event.error;
+ var message = event.message;
if (progressText) {
progressText.style.color = '#ff6b6b';
- progressText.textContent = 'Error: ' + (error?.message || message);
+ progressText.textContent = 'Error: ' + (error?.message || message);
}
- console.error('[Splash] Error:', message, source, lineno, colno, error);
- return false;
- };
- window.onunhandledrejection = function(event) {
+ if (window.Sentry) window.Sentry.captureException(error ?? new Error(message));
+ });
+ window.addEventListener('unhandledrejection', function(event) {
if (progressText) {
progressText.style.color = '#ff6b6b';
progressText.textContent = 'Error: ' + (event.reason?.message || String(event.reason));
}
- console.error('[Splash] Unhandled rejection:', event.reason);
- };
+ if (window.Sentry) window.Sentry.captureException(event.reason);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| window.onerror = function(message, source, lineno, colno, error) { | |
| if (progressText) { | |
| progressText.style.color = '#ff6b6b'; | |
| progressText.textContent = 'Error: ' + (error?.message || message); | |
| } | |
| console.error('[Splash] Error:', message, source, lineno, colno, error); | |
| return false; | |
| }; | |
| window.onunhandledrejection = function(event) { | |
| if (progressText) { | |
| progressText.style.color = '#ff6b6b'; | |
| progressText.textContent = 'Error: ' + (event.reason?.message || String(event.reason)); | |
| } | |
| console.error('[Splash] Unhandled rejection:', event.reason); | |
| }; | |
| window.addEventListener('error', function(event) { | |
| var error = event.error; | |
| var message = event.message; | |
| if (progressText) { | |
| progressText.style.color = '#ff6b6b'; | |
| progressText.textContent = 'Error: ' + (error?.message || message); | |
| } | |
| if (window.Sentry) window.Sentry.captureException(error ?? new Error(message)); | |
| }); | |
| window.addEventListener('unhandledrejection', function(event) { | |
| if (progressText) { | |
| progressText.style.color = '#ff6b6b'; | |
| progressText.textContent = 'Error: ' + (event.reason?.message || String(event.reason)); | |
| } | |
| if (window.Sentry) window.Sentry.captureException(event.reason); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/public/splash.js` around lines 7 - 21, Replace the direct
assignments to window.onerror and window.onunhandledrejection with event
listeners registered via window.addEventListener('error', ...) and
window.addEventListener('unhandledrejection', ...), update the handler callbacks
that reference progressText (preserve the same UI color/text behavior), and in
each handler call Sentry.captureException(...) with the error/event.reason (same
pattern used in src/renderer/main.tsx) in addition to console.error; ensure you
do not return false from the error handler so other listeners remain unaffected.
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 by CodeRabbit
New Features
Bug Fixes
Tests