Conversation
… main.js and ipc handlers.js
…ents in main.js and ipc handlers.js
… to streamline shutdown process.
📝 WalkthroughWalkthroughThe Electron main process undergoes significant restructuring with single-instance lock enforcement, early database and IPC initialization, addition of a protocol-based media file handler, reorganized startup sequencing, enhanced window lifecycle management, and unified error handling with improved logging. Version bumped to 0.35.1. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/main.js`:
- Around line 298-303: The code currently validates the lexical path with
resolveAndValidatePath() but then reads the file without resolving symlinks,
allowing a symlink inside an allowed base to point outside; change the flow to
resolve symlinks first (e.g. use fs.realpath or fs.promises.realpath) to obtain
the canonical path, then validate that canonical path against allowedBases with
resolveAndValidatePath() (or add a new validateRealPath function), and only
after validation call readFile() using the canonical path; update filePath to
the resolved real path and keep the same logging (logger.warn) and response
behavior if validation fails.
- Around line 395-406: The callbacks that call refreshTray() (onStatusChange and
onProjectListChange) currently drop its returned promise, risking unhandled
rejections; modify those handlers to await or explicitly handle refreshTray()'s
promise and catch errors (e.g., call refreshTray().catch(err =>
processLogger.error(...)) or use an async wrapper and try/catch) so failures
from refreshTray(), its DB calls or updateTrayMenu() are logged and do not
crash/stop tray updates; ensure you update both onStatusChange and
onProjectListChange handlers and any other locations that call refreshTray()
without handling its promise.
- Around line 268-290: The current fallback adds permissive drive/root bases
when configuredExternalMediaBases is empty, enabling full-drive reads; change
the logic in the block that builds allowedBases (the branch referencing
configuredExternalMediaBases, driveRoot, EXTERNAL_MEDIA_DIRS_ENV_KEY, and
hasLoggedPermissiveExternalMediaMode) to reject external media requests when
SELFHOST_MEDIA_ALLOWED_DIRS is unset: do not push driveRoot or Windows root into
allowedBases, instead leave allowedBases empty (or set a flag to disable
external-media handling) and emit a clear warning/error instructing operators to
set EXTERNAL_MEDIA_DIRS_ENV_KEY; ensure resolveAndValidatePath (the
path-checking code) will then deny all external media access rather than
allowing the permissive root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da7813fb-0adc-4f2f-ae6b-f31f92234681
📒 Files selected for processing (2)
electron/main.jspackage.json
| if (configuredExternalMediaBases.length > 0) { | ||
| allowedBases.push(...configuredExternalMediaBases); | ||
| } else { | ||
| // Backward-compatible permissive mode: allow readable files on the resolved drive root. | ||
| const driveRoot = path.parse(path.resolve(filePath)).root; | ||
| if (driveRoot) { | ||
| allowedBases.push(driveRoot); | ||
| } | ||
| if ( | ||
| process.platform === "win32" && | ||
| hostname && | ||
| hostname.length === 1 && | ||
| /^[a-zA-Z]$/.test(hostname) | ||
| ) { | ||
| allowedBases.push(path.resolve(`${hostname}:\\`)); | ||
| } | ||
|
|
||
| if (!hasLoggedPermissiveExternalMediaMode) { | ||
| logger.warn( | ||
| `[Media Protocol] External media requests are using permissive drive-root mode. Set ${EXTERNAL_MEDIA_DIRS_ENV_KEY} to a ${path.delimiter}-separated list of allowed directories to restrict access.` | ||
| ); | ||
| hasLoggedPermissiveExternalMediaMode = true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the key files mentioned in the review
git ls-files | grep -E "electron/main.js|ProjectSettingsDialog.jsx"Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 132
🏁 Script executed:
# Check the current state of electron/main.js around lines 268-290
wc -l electron/main.jsRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 95
🏁 Script executed:
# Read electron/main.js around lines 268-290 to verify the code snippet
sed -n '260,310p' electron/main.js | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 2515
🏁 Script executed:
# Read the protocol handler context - lines 219-336
sed -n '219,336p' electron/main.js | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 5867
🏁 Script executed:
# Check for EXTERNAL_MEDIA_DIRS_ENV_KEY or SELFHOST_MEDIA_ALLOWED_DIRS definition
rg "EXTERNAL_MEDIA_DIRS_ENV_KEY|SELFHOST_MEDIA_ALLOWED_DIRS" electron/main.jsRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 442
🏁 Script executed:
# Read src/components/ProjectSettingsDialog.jsx around lines 1-50
sed -n '1,50p' src/components/ProjectSettingsDialog.jsx | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 1840
🏁 Script executed:
# Search for iconPreview usage in ProjectSettingsDialog.jsx
rg "iconPreview|media://" src/components/ProjectSettingsDialog.jsx -A 3 -B 1Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 1355
🏁 Script executed:
# Find where resolveAndValidatePath is defined and check its implementation
rg "resolveAndValidatePath" electron/main.js -A 10 -B 2Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 1008
🏁 Script executed:
# Search for any symlink-related comments or handling in the protocol handler
rg -i "symlink" electron/main.jsRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 57
🏁 Script executed:
# Check the full context to understand configuredExternalMediaBases initialization
sed -n '1,220p' electron/main.js | tail -50 | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 1704
🏁 Script executed:
# Find the isPathWithinBase function implementation
rg "isPathWithinBase" electron/main.js -A 5 -B 2Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 1153
🏁 Script executed:
# Get more context about where configuredExternalMediaBases is set
sed -n '1,50p' electron/main.js | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 2235
🏁 Script executed:
# Check if formData.icon or icon paths can be set to arbitrary local filesystem paths
rg -i "formData\.icon|icon.*path" src/components/ProjectSettingsDialog.jsx -B 3 -A 3 | head -60Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 1225
🏁 Script executed:
# Check the complete isPathWithinBase implementation
sed -n '44,55p' electron/main.js | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 601
🏁 Script executed:
# Check if there's any symlink resolution in the validation
rg "realpath|resolveSymlink" electron/main.jsRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 57
🏁 Script executed:
# Verify what happens with fs.promises.access - does it follow symlinks?
# Let's look at how filePath is used after validation
sed -n '85,95p' electron/main.js | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 508
🏁 Script executed:
# Check if there are any tests or documentation about the expected behavior
find . -type f \( -name "*.test.js" -o -name "*.spec.js" -o -name "README*" \) | head -10Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 86
🏁 Script executed:
# Let's check if fs.promises.readFile follows symlinks (it should)
# and verify the full file reading flow
sed -n '85,115p' electron/main.js | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 1310
🏁 Script executed:
# Check if there are any symlink checks or real path resolution elsewhere
rg "realpath|readlink|symlink" electron/main.js -iRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 57
🏁 Script executed:
# Verify the exact behavior: does the validation happen BEFORE symlink resolution?
# Let's trace the flow from filePath assignment through to file read
sed -n '75,95p' electron/main.js | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 765
🏁 Script executed:
# Let's see the complete file reading and media handler code section
sed -n '76,118p' electron/main.js | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 1603
🏁 Script executed:
# Check if there's any file picker dialog or input validation for the icon field
rg "ipcMain\|dialog\|file picker" electron/main.js -i -B 2 -A 5 | head -40Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 57
🏁 Script executed:
# Verify: when permissive mode is active, what exactly is allowedBases set to?
# Let's trace through for a Windows example
sed -n '50,73p' electron/main.js | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 1047
🏁 Script executed:
# Check the complete media protocol handler - lines 219-310
sed -n '219,310p' electron/main.js | cat -nRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 4728
🏁 Script executed:
# Check if there's any environment variable setup or documentation
cat README.md | head -100Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 6618
🏁 Script executed:
# Verify the exact line numbers for the protocol handler setup
grep -n "protocol.handle" electron/main.jsRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 130
🏁 Script executed:
# Let's verify the exact behavior when permissive mode activates
# Specifically, what does driveRoot return on different platforms
python3 << 'EOF'
import os
import sys
# Simulate what Node.js path.parse().root returns
def test_path_root():
# On POSIX (Linux/Mac), root would be "/"
# On Windows, root would be "C:\" or similar
# Example paths:
test_cases = [
"/etc/passwd", # POSIX - root is "/"
"/home/user/file.txt", # POSIX - root is "/"
"C:\\Windows\\System32", # Windows - root is "C:\\"
"D:\\folder\\file.txt", # Windows - root is "D:\\"
]
for path in test_cases:
if path.startswith("/"):
root = "/"
else:
# Windows path - extract drive letter
if len(path) > 1 and path[1] == ":":
root = path[0:2] + "\\"
print(f"Path: {path} -> Root: {root}")
test_path_root()
EOFRepository: DevRoots-Studio/SelfHost-Helper
Length of output: 217
🏁 Script executed:
# Verify if path.resolve follows symlinks in Node.js behavior
# Check if there are any other safety checks we missed
sed -n '219,310p' electron/main.js | grep -i "symlink\|realpath\|resolve"Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 731
🏁 Script executed:
# Confirm the flow one more time: does user input from formData.icon go directly into the URL?
# Check formData usage in ProjectSettingsDialog
rg "formData\|setFormData" src/components/ProjectSettingsDialog.jsx -A 2 | head -50Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 57
🏁 Script executed:
# Final verification: confirm the exact flow from user input to file read
# Check how icon paths are stored and retrieved from formData
rg "icon.*=" src/components/ProjectSettingsDialog.jsx | head -20Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 139
🏁 Script executed:
# Verify there's no additional validation on the icon path before it's used in media:// URL
sed -n '100,200p' src/components/ProjectSettingsDialog.jsx | grep -A 10 "media://"Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 57
Reject external media requests when SELFHOST_MEDIA_ALLOWED_DIRS is not configured.
When SELFHOST_MEDIA_ALLOWED_DIRS is unset, the code falls back to permissive mode that allowlists the entire drive on Windows (C:\) and filesystem root on POSIX (/). Combined with the renderer constructing media:// URLs from user-controlled formData.icon in ProjectSettingsDialog.jsx, this becomes a local file disclosure vulnerability. Any absolute path will pass validation since resolveAndValidatePath only checks if the path is within the permitted bases—and when the base is the entire drive root, all files are readable. Since the media:// protocol is registered with supportFetchAPI: true, this exposes a fetch-capable file read primitive.
Suggested fix
if (configuredExternalMediaBases.length > 0) {
allowedBases.push(...configuredExternalMediaBases);
} else {
- // Backward-compatible permissive mode: allow readable files on the resolved drive root.
- const driveRoot = path.parse(path.resolve(filePath)).root;
- if (driveRoot) {
- allowedBases.push(driveRoot);
- }
- if (
- process.platform === "win32" &&
- hostname &&
- hostname.length === 1 &&
- /^[a-zA-Z]$/.test(hostname)
- ) {
- allowedBases.push(path.resolve(`${hostname}:\\`));
- }
-
- if (!hasLoggedPermissiveExternalMediaMode) {
- logger.warn(
- `[Media Protocol] External media requests are using permissive drive-root mode. Set ${EXTERNAL_MEDIA_DIRS_ENV_KEY} to a ${path.delimiter}-separated list of allowed directories to restrict access.`
- );
- hasLoggedPermissiveExternalMediaMode = true;
- }
+ logger.warn(
+ `[Media Protocol] Rejecting external media request because ${EXTERNAL_MEDIA_DIRS_ENV_KEY} is not configured.`
+ );
+ return new Response("Forbidden", { status: 403 });
}📝 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.
| if (configuredExternalMediaBases.length > 0) { | |
| allowedBases.push(...configuredExternalMediaBases); | |
| } else { | |
| // Backward-compatible permissive mode: allow readable files on the resolved drive root. | |
| const driveRoot = path.parse(path.resolve(filePath)).root; | |
| if (driveRoot) { | |
| allowedBases.push(driveRoot); | |
| } | |
| if ( | |
| process.platform === "win32" && | |
| hostname && | |
| hostname.length === 1 && | |
| /^[a-zA-Z]$/.test(hostname) | |
| ) { | |
| allowedBases.push(path.resolve(`${hostname}:\\`)); | |
| } | |
| if (!hasLoggedPermissiveExternalMediaMode) { | |
| logger.warn( | |
| `[Media Protocol] External media requests are using permissive drive-root mode. Set ${EXTERNAL_MEDIA_DIRS_ENV_KEY} to a ${path.delimiter}-separated list of allowed directories to restrict access.` | |
| ); | |
| hasLoggedPermissiveExternalMediaMode = true; | |
| } | |
| if (configuredExternalMediaBases.length > 0) { | |
| allowedBases.push(...configuredExternalMediaBases); | |
| } else { | |
| logger.warn( | |
| `[Media Protocol] Rejecting external media request because ${EXTERNAL_MEDIA_DIRS_ENV_KEY} is not configured.` | |
| ); | |
| return new Response("Forbidden", { status: 403 }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/main.js` around lines 268 - 290, The current fallback adds
permissive drive/root bases when configuredExternalMediaBases is empty, enabling
full-drive reads; change the logic in the block that builds allowedBases (the
branch referencing configuredExternalMediaBases, driveRoot,
EXTERNAL_MEDIA_DIRS_ENV_KEY, and hasLoggedPermissiveExternalMediaMode) to reject
external media requests when SELFHOST_MEDIA_ALLOWED_DIRS is unset: do not push
driveRoot or Windows root into allowedBases, instead leave allowedBases empty
(or set a flag to disable external-media handling) and emit a clear
warning/error instructing operators to set EXTERNAL_MEDIA_DIRS_ENV_KEY; ensure
resolveAndValidatePath (the path-checking code) will then deny all external
media access rather than allowing the permissive root.
| const resolvedFilePath = resolveAndValidatePath(filePath, allowedBases); | ||
| if (!resolvedFilePath) { | ||
| logger.warn(`[Media Protocol] Blocked path outside allowed bases: ${filePath}`); | ||
| return new Response("Forbidden", { status: 403 }); | ||
| } | ||
| filePath = resolvedFilePath; |
There was a problem hiding this comment.
Resolve symlinks before enforcing the allowlist.
resolveAndValidatePath() only checks the lexical path. A symlink inside an allowed base still passes this test, and readFile() will then follow it to a target outside the base.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/main.js` around lines 298 - 303, The code currently validates the
lexical path with resolveAndValidatePath() but then reads the file without
resolving symlinks, allowing a symlink inside an allowed base to point outside;
change the flow to resolve symlinks first (e.g. use fs.realpath or
fs.promises.realpath) to obtain the canonical path, then validate that canonical
path against allowedBases with resolveAndValidatePath() (or add a new
validateRealPath function), and only after validation call readFile() using the
canonical path; update filePath to the resolved real path and keep the same
logging (logger.warn) and response behavior if validation fails.
| // Update tray on status changes | ||
| onStatusChange(() => { | ||
| refreshTray(); | ||
| }); | ||
|
|
||
| // Update tray on project list changes (add/delete) | ||
| onProjectListChange(() => { | ||
| // Update tray on project list changes (add/delete) | ||
| onProjectListChange(() => { | ||
| refreshTray(); | ||
| }); | ||
|
|
||
| // Initial tray setup | ||
| refreshTray(); |
There was a problem hiding this comment.
Catch refreshTray() failures.
These callbacks drop the promise returned by refreshTray(). If a DB call or updateTrayMenu() throws, the main process gets an unhandled rejection and tray updates stop degrading cleanly.
Possible fix
const refreshTray = async () => {
logger.debug("[Tray] Refreshing menu state.");
const [projects, categories] = await Promise.all([getProjects(), getCategories()]);
const runningIds = getRunningProjects();
updateTrayMenu(
projects,
categories,
runningIds,
startProject,
stopProject,
restartProject,
startAllProjects,
stopAllProjects
);
};
+ const safeRefreshTray = () => {
+ void refreshTray().catch((error) => {
+ logger.error("[Tray] Failed to refresh menu:", error);
+ });
+ };
tray = initTray(window, () => {
isQuitting = true;
app.quit();
});
// Update tray on status changes
onStatusChange(() => {
- refreshTray();
+ safeRefreshTray();
});
// Update tray on project list changes (add/delete)
onProjectListChange(() => {
- refreshTray();
+ safeRefreshTray();
});
// Initial tray setup
- refreshTray();
+ safeRefreshTray();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/main.js` around lines 395 - 406, The callbacks that call
refreshTray() (onStatusChange and onProjectListChange) currently drop its
returned promise, risking unhandled rejections; modify those handlers to await
or explicitly handle refreshTray()'s promise and catch errors (e.g., call
refreshTray().catch(err => processLogger.error(...)) or use an async wrapper and
try/catch) so failures from refreshTray(), its DB calls or updateTrayMenu() are
logged and do not crash/stop tray updates; ensure you update both onStatusChange
and onProjectListChange handlers and any other locations that call refreshTray()
without handling its promise.
1 participant
Summary by CodeRabbit
Bug Fixes
Chores