feat: add browser extension integration to main branch - #96
Conversation
- Add browser extension files (background, content, native-host, popup) - Add tray dashboard enhancements (health score, RTP, quick scan, network sparkline) - Add health score i18n translations for all locales - Add credential leak notification IPC handler - Add browser extension native host installation IPC handler - Add browserExtension API to preload
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe browser extension now uses direct local health checks and persistent native-host stdin parsing. Desktop IPC handles credential alerts and Windows native-host installation. Packaging scripts changed, while locale files remove Emergency Lockdown strings and add selected health translations. ChangesBrowser extension integration
Localization updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| desktopProc = spawn(cmd, args, options); | ||
| desktopProc.unref(); | ||
| desktopProc = spawn(cmd, args, options); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/i18n/locales/ja.json (1)
202-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove or restore the lockdown navigation contract.
src/ui/pages/shell.html:106-113still requestsnav.lockdown. Removing this key from these locale files leaves the visible navigation item untranslated.
src/i18n/locales/ja.json#L202-L205: retainnav.lockdownor remove the shell item.src/i18n/locales/pt-BR.json#L202-L205: retainnav.lockdownor remove the shell item.src/i18n/locales/ru.json#L202-L205: retainnav.lockdownor remove the shell item.src/i18n/locales/tr.json#L202-L205: retainnav.lockdownor remove the shell item.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/locales/ja.json` around lines 202 - 205, Restore the nav.lockdown translation key in src/i18n/locales/ja.json (lines 202-205), src/i18n/locales/pt-BR.json (lines 202-205), src/i18n/locales/ru.json (lines 202-205), and src/i18n/locales/tr.json (lines 202-205), or consistently remove the corresponding navigation item requested by src/ui/pages/shell.html. Ensure the shell navigation does not request an untranslated key.
🧹 Nitpick comments (4)
browser-extension/package.json (1)
8-8: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid requiring a platform-specific
zipexecutable.This npm script now depends on a bare
zipcommand. Windows environments do not guarantee that executable. Keep the existing Node-based packager or use a cross-platform ZIP dependency, or verify that every release environment installszip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@browser-extension/package.json` at line 8, Update the package script’s archive step to avoid relying on the platform-specific bare zip executable. Reuse the existing Node-based packager or an installed cross-platform ZIP dependency, while preserving the current icon build, archive contents, and exclusions.browser-extension/native-host.js (2)
55-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the unused
pendingandmsgIdbookkeeping.
pendingandmsgIdare never read or written after this declaration. They remain from the removed named-pipe request/response flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@browser-extension/native-host.js` around lines 55 - 57, Remove the unused pending Map and msgId declaration near desktopProc in native-host.js, leaving desktopProc and the remaining native-host initialization unchanged.
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
readlineinterface.
rlis never used. The parser reads raw frames through theprocess.stdindatahandler at line 35.readline.createInterfaceattaches a second consumer to the same stream and switches stdin into flowing mode. stdin carries binary length-prefixed frames here, so line-based decoding serves no purpose.Delete the interface and the
readlinerequire at line 8.♻️ Proposed cleanup
function readMessages() { - const rl = readline.createInterface({ - input: process.stdin, - terminal: false - }); - let buffer = Buffer.alloc(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@browser-extension/native-host.js` around lines 28 - 33, Remove the unused readline import and the readline.createInterface assignment to rl; keep the raw process.stdin data-handler parsing and buffer initialization unchanged.src/preload/preload.js (1)
51-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused preload bridge method.
No renderer caller uses
window.api.browserExtension.installNativeHost; settings invokeswindow.api.invoke('browserExtension:installNativeHost')directly. Use the typed method from the settings flow or drop the exposed method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/preload/preload.js` around lines 51 - 52, Remove the unused installNativeHost method from the browserExtension preload bridge, while preserving the settings flow’s direct typed invocation through window.api.invoke('browserExtension:installNativeHost').
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser-extension/background.js`:
- Around line 1-2: Update the chrome.runtime.onInstalled listener so it seeds
externalLookupsEnabled only when the setting is absent, or restricts
initialization to the initial install reason. Preserve an existing false value
across extension and browser updates, rather than unconditionally writing true
in the onInstalled callback.
In `@browser-extension/content.js`:
- Line 97: In browser-extension/content.js, add a shared removeIcon(input, icon)
helper that removes the scroll/resize updatePos listeners, deletes the
passwordFields entry and input.dataset.soteriosId, then removes the icon. At
browser-extension/content.js:97, replace the direct icon removal in the blur
handler with this helper; at browser-extension/content.js:136, use it for each
stored entry before clearing passwordFields.
In `@browser-extension/native-host.js`:
- Around line 59-66: Update launchDesktopApp to use the module-level DESKTOP_APP
constant instead of process.env.DESKTOP_APP, and ensure its default resolves to
a valid installed executable path or is handled as a protocol URL before
fs.existsSync validation. Preserve the existing rejection behavior only when no
usable application target is available.
- Around line 45-51: Update the message-processing flow around handleMessage to
await its promise and catch asynchronous rejections, not just JSON.parse
failures. Report rejected handleMessage errors through the existing native-host
protocol so the host remains running and the extension receives an error
message.
- Around line 74-80: Update the launch flow around desktopProc and spawn to
invoke resolvedPath directly on Windows instead of using cmd with /c start,
while preserving detached execution and unref behavior. Also update the
surrounding completion/error handling so the launch promise rejects when the
spawn error handler fires rather than resolving unconditionally after the
timeout.
In `@browser-extension/package.json`:
- Line 8: Update the package script’s zip command to restore exclusions for
node_modules, package.json, and package-lock.json, while preserving the existing
exclusions for .DS_Store, icons/*.svg, and tools/*.
- Around line 7-9: Update the package scripts so their paths resolve from the
browser-extension directory: remove the nested cd browser-extension from
package, change install:host to invoke ../tools/install-native-host.js, and
update build:icons to use ../tools/build-icons.js while preserving the existing
zip exclusions.
In `@browser-extension/popup.js`:
- Around line 43-51: Update browser-extension/manifest.json to grant host
permission for http://localhost:17234/* so the fetch in the popup health-check
flow can reach the desktop app, and ensure the desktop app exposes a GET
/api/health endpoint compatible with that request.
In `@src/i18n/locales/ja.json`:
- Line 765: Remove duplicate health.malware.* JSON keys across all affected
locales: in src/i18n/locales/ja.json lines 765-765, update the existing
health.malware.high entry rather than adding a second definition; in
src/i18n/locales/pt-BR.json lines 789-793, retain one health.malware.*
definition with Portuguese values, and in lines 807-832 remove the repeated
English definitions while preserving those Portuguese values; in
src/i18n/locales/ru.json lines 803-828 and src/i18n/locales/tr.json lines
803-828, remove the English blocks that overwrite the existing Russian and
Turkish values.
In `@src/i18n/locales/tr.json`:
- Around line 789-793: Translate the five malware localization entries under
health.malware in tr.json into Turkish, including the {count} placeholder in
both threat-count messages; do not leave the English fallback text unless
English is explicitly intended.
- Line 812: Update the health.load.reason translation in tr.json to use the
dashboard’s pct placeholder format {pct}% instead of the current %{pct}%,
preserving the intended percentage suffix without the extra leading percent
sign.
In `@src/main/ipc/system.js`:
- Line 386: Validate SOTERIOS_EXT_ID in the handler around extId before
modifying allowed_origins or registering the manifest; reject both an unset
value and the literal placeholder YOUR_EXTENSION_ID_HERE with a structured error
response and do not report ok: true. Preserve the existing registration and
success flow for valid extension IDs, using the response shape expected by the
settings toggle.
- Around line 385-390: Update the manifest setup around the manifest read,
allowed_origins rewrite, and registry command so the packaged manifest remains
unchanged: read it as a template, resolve the extension ID without assuming a
nonempty allowed_origins array, write the resolved manifest under
app.getPath('userData'), and register that generated path. Move the read,
rewrite, and write operations inside the existing try block so failures return
the established structured { ok: false, error } result instead of escaping the
IPC handler.
- Around line 356-365: Update the validation at the start of the alert-building
flow near the password check to coerce payload.count to a finite non-negative
integer and reject invalid values before constructing alert. Reuse the validated
count for the message, detail, and metadata fields in the alert object.
- Around line 390-395: Replace the shell-string execSync calls in the registry
setup block with execFileSync('reg', argument arrays), passing manifestPath
unchanged without backslash doubling. Before constructing regPath and
regPathEdge, validate manifest.name against /^[a-z0-9_.]+$/ and avoid using it
if invalid; preserve the existing registry operations for valid names.
---
Outside diff comments:
In `@src/i18n/locales/ja.json`:
- Around line 202-205: Restore the nav.lockdown translation key in
src/i18n/locales/ja.json (lines 202-205), src/i18n/locales/pt-BR.json (lines
202-205), src/i18n/locales/ru.json (lines 202-205), and src/i18n/locales/tr.json
(lines 202-205), or consistently remove the corresponding navigation item
requested by src/ui/pages/shell.html. Ensure the shell navigation does not
request an untranslated key.
---
Nitpick comments:
In `@browser-extension/native-host.js`:
- Around line 55-57: Remove the unused pending Map and msgId declaration near
desktopProc in native-host.js, leaving desktopProc and the remaining native-host
initialization unchanged.
- Around line 28-33: Remove the unused readline import and the
readline.createInterface assignment to rl; keep the raw process.stdin
data-handler parsing and buffer initialization unchanged.
In `@browser-extension/package.json`:
- Line 8: Update the package script’s archive step to avoid relying on the
platform-specific bare zip executable. Reuse the existing Node-based packager or
an installed cross-platform ZIP dependency, while preserving the current icon
build, archive contents, and exclusions.
In `@src/preload/preload.js`:
- Around line 51-52: Remove the unused installNativeHost method from the
browserExtension preload bridge, while preserving the settings flow’s direct
typed invocation through
window.api.invoke('browserExtension:installNativeHost').
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33ca8401-695c-46ef-b876-2f3e6de3a868
📒 Files selected for processing (22)
browser-extension/background.jsbrowser-extension/content.jsbrowser-extension/native-host.jsbrowser-extension/package.jsonbrowser-extension/popup.jssrc/i18n/locales/ar.jsonsrc/i18n/locales/de.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/hi.jsonsrc/i18n/locales/it.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/ko.jsonsrc/i18n/locales/nl.jsonsrc/i18n/locales/pl.jsonsrc/i18n/locales/pt-BR.jsonsrc/i18n/locales/ru.jsonsrc/i18n/locales/tr.jsonsrc/i18n/locales/zh-CN.jsonsrc/main/ipc/system.jssrc/preload/preload.js
💤 Files with no reviewable changes (11)
- src/i18n/locales/zh-CN.json
- src/i18n/locales/hi.json
- src/i18n/locales/es.json
- src/i18n/locales/ar.json
- src/i18n/locales/de.json
- src/i18n/locales/ko.json
- src/i18n/locales/it.json
- src/i18n/locales/fr.json
- src/i18n/locales/en.json
- src/i18n/locales/pl.json
- src/i18n/locales/nl.json
| input.addEventListener('input', autoCheckHandler); | ||
| icon._soteriosHandlers.autoCheckHandler = autoCheckHandler; | ||
| } | ||
| input.addEventListener('blur', () => setTimeout(() => icon.remove(), 200), { once: true }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Icon teardown is incomplete at both cleanup sites, so fields permanently lose their icon.
addIconToField registers a updatePos listener on window for scroll and resize (lines 95-96), stores an entry in passwordFields (line 99), and sets input.dataset.soteriosId (line 87). Both cleanup paths now call only icon.remove(), so none of those three are reversed. One missing teardown routine causes all of the following:
- The
updatePosclosure keepsiconandinputreachable, so the detached icon never gets collected, and thewindowlistener list grows with every password field. - The
passwordFieldsentry retains both nodes for the page lifetime. input.dataset.soteriosIdstays set.addIconToFieldreturns early at line 84 on that dataset value, andscanForPasswordFieldsexcludes[data-soterios-id]at line 103. After the first blur, the field never gets an icon again, even when the user focuses it again.
Add one removeIcon(input, icon) helper that removes the listeners, deletes the map entry, deletes dataset.soteriosId, and removes the icon. Call it from both sites.
browser-extension/content.js#L97: call the helper from theblurhandler instead oficon.remove().browser-extension/content.js#L136: call the helper for each entry instead oficon.remove(), then clear the map.
🐛 Proposed shared teardown helper
function addIconToField(input) {
@@
const updatePos = () => positionIcon(icon, input);
window.addEventListener('scroll', updatePos, true);
window.addEventListener('resize', updatePos);
- input.addEventListener('blur', () => setTimeout(() => icon.remove(), 200), { once: true });
+ icon._cleanup = () => {
+ window.removeEventListener('scroll', updatePos, true);
+ window.removeEventListener('resize', updatePos);
+ delete input.dataset.soteriosId;
+ passwordFields.delete(input);
+ icon.remove();
+ };
+ input.addEventListener('blur', () => setTimeout(() => icon._cleanup(), 200), { once: true });
passwordFields.set(input, icon);
}- passwordFields.forEach((icon, input) => icon.remove());
+ passwordFields.forEach((icon) => icon._cleanup());📝 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.
| input.addEventListener('blur', () => setTimeout(() => icon.remove(), 200), { once: true }); | |
| const updatePos = () => positionIcon(icon, input); | |
| window.addEventListener('scroll', updatePos, true); | |
| window.addEventListener('resize', updatePos); | |
| icon._cleanup = () => { | |
| window.removeEventListener('scroll', updatePos, true); | |
| window.removeEventListener('resize', updatePos); | |
| delete input.dataset.soteriosId; | |
| passwordFields.delete(input); | |
| icon.remove(); | |
| }; | |
| input.addEventListener('blur', () => setTimeout(() => icon._cleanup(), 200), { once: true }); | |
| passwordFields.set(input, icon); |
| input.addEventListener('blur', () => setTimeout(() => icon.remove(), 200), { once: true }); | |
| passwordFields.forEach((icon) => icon._cleanup()); |
📍 Affects 1 file
browser-extension/content.js#L97-L97(this comment)browser-extension/content.js#L136-L136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@browser-extension/content.js` at line 97, In browser-extension/content.js,
add a shared removeIcon(input, icon) helper that removes the scroll/resize
updatePos listeners, deletes the passwordFields entry and
input.dataset.soteriosId, then removes the icon. At
browser-extension/content.js:97, replace the direct icon removal in the blur
handler with this helper; at browser-extension/content.js:136, use it for each
stored entry before clearing passwordFields.
| try { | ||
| const msg = JSON.parse(json); | ||
| handleMessage(msg); | ||
| } catch (e) { | ||
| log('Parse error:', e.message); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Handle rejections from handleMessage to keep the host alive.
handleMessage is async, and line 47 calls it without awaiting it or attaching a catch. The try block only catches JSON.parse errors, because the returned promise settles after the block exits.
handleMessage awaits launchDesktopApp at lines 96 and 105, and that promise rejects when the path is unset or missing. The rejection is then unhandled. Node terminates the process on an unhandled rejection by default. The native host exits, and background.js onDisconnect at line 19 clears nativePort. The extension loses the host on the first CREDENTIAL_LEAK, and the extension receives no error message.
Catch the rejection and report it over the protocol.
🛡️ Proposed fix to report failures instead of exiting
try {
const msg = JSON.parse(json);
- handleMessage(msg);
+ Promise.resolve(handleMessage(msg)).catch(e => {
+ log('Handler error:', e.message);
+ send({ type: 'ERROR', error: e.message, original: msg });
+ });
} catch (e) {
log('Parse error:', e.message);
}📝 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.
| try { | |
| const msg = JSON.parse(json); | |
| handleMessage(msg); | |
| } catch (e) { | |
| log('Parse error:', e.message); | |
| } | |
| } | |
| try { | |
| const msg = JSON.parse(json); | |
| Promise.resolve(handleMessage(msg)).catch(e => { | |
| log('Handler error:', e.message); | |
| send({ type: 'ERROR', error: e.message, original: msg }); | |
| }); | |
| } catch (e) { | |
| log('Parse error:', e.message); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@browser-extension/native-host.js` around lines 45 - 51, Update the
message-processing flow around handleMessage to await its promise and catch
asynchronous rejections, not just JSON.parse failures. Report rejected
handleMessage errors through the existing native-host protocol so the host
remains running and the extension receives an error message.
| function launchDesktopApp() { | ||
| if (desktopProc) return Promise.resolve(); | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| const appPath = process.env.DESKTOP_APP || 'soterios://'; | ||
|
|
||
| // Check if it's a protocol URL or an executable path | ||
| const isProtocolUrl = appPath.startsWith('soterios://') || appPath.startsWith('http://') || appPath.startsWith('https://'); | ||
|
|
||
| if (isProtocolUrl) { | ||
| // Launch using OS-appropriate protocol handler | ||
| const isWin = process.platform === 'win32'; | ||
| const args = isWin ? ['/c', 'start', '', appPath] : ['open', appPath]; | ||
| const cmd = isWin ? 'cmd' : (process.platform === 'darwin' ? 'open' : 'xdg-open'); | ||
| const options = { shell: false, detached: true }; | ||
|
|
||
| desktopProc = spawn(cmd, args, options); | ||
| desktopProc.unref(); | ||
|
|
||
| desktopProc.on('error', e => { | ||
| log('Desktop app launch error:', e.message); | ||
| desktopProc = null; | ||
| }); | ||
|
|
||
| setTimeout(resolve, 1500); | ||
| } else { | ||
| // Launch as executable path | ||
| const resolvedPath = path.resolve(appPath); | ||
| if (!fs.existsSync(resolvedPath)) { | ||
| return reject(new Error('Desktop app not found at: ' + resolvedPath)); | ||
| } | ||
| const appPath = process.env.DESKTOP_APP; | ||
| if (!appPath) { | ||
| return reject(new Error('DESKTOP_APP environment variable not set')); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
launchDesktopApp reads the wrong environment variable, so it always fails.
Line 12 defines DESKTOP_APP from SOTERIOS_APP_PATH. Line 63 reads process.env.DESKTOP_APP instead, and the DESKTOP_APP constant is never read anywhere. process.env.DESKTOP_APP is a different variable that nothing sets.
The result is that launchDesktopApp rejects with "DESKTOP_APP environment variable not set" on every call. Both CREDENTIAL_LEAK (line 96) and OPEN_APP (line 105) therefore never launch the desktop app.
A native-messaging host inherits only the browser process environment, so SOTERIOS_APP_PATH is also unlikely to be present. The removed protocol-URL fallback means the 'soterios://' default at line 12 is now unreachable. Add a resolved default path, or restore the protocol fallback, so the host works without an environment variable.
🐛 Proposed fix to use the resolved constant
return new Promise((resolve, reject) => {
- const appPath = process.env.DESKTOP_APP;
+ const appPath = DESKTOP_APP;
if (!appPath) {
- return reject(new Error('DESKTOP_APP environment variable not set'));
+ return reject(new Error('SOTERIOS_APP_PATH environment variable not set'));
}DESKTOP_APP still defaults to 'soterios://', which is not a filesystem path and fails the fs.existsSync check at line 70. Either point the default at the installed executable, or handle the protocol form before the path validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@browser-extension/native-host.js` around lines 59 - 66, Update
launchDesktopApp to use the module-level DESKTOP_APP constant instead of
process.env.DESKTOP_APP, and ensure its default resolves to a valid installed
executable path or is handled as a protocol URL before fs.existsSync validation.
Preserve the existing rejection behavior only when no usable application target
is available.
| const isWin = process.platform === 'win32'; | ||
| const args = isWin ? ['/c', 'start', '""', resolvedPath] : [resolvedPath]; | ||
| const cmd = isWin ? 'cmd' : resolvedPath; | ||
| const options = { shell: false, detached: true }; | ||
|
|
||
| desktopProc = spawn(cmd, args, options); | ||
| desktopProc.unref(); | ||
| desktopProc = spawn(cmd, args, options); | ||
| desktopProc.unref(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Launch the executable directly instead of through cmd /c start on Windows.
The comment at line 68 states the goal is to prevent command injection. shell: false stops Node from invoking a shell, but cmd is then spawned explicitly and parses /c start "" <resolvedPath> with its own rules. That reintroduces a command-execution surface for a path taken from the environment. CodeQL flags line 79 for this reason.
spawn starts a Windows executable directly, so cmd is not needed. Note also that setTimeout(resolve, 1500) at line 87 resolves even after the error handler fires, so callers treat a failed launch as a success.
🔒 Proposed fix to drop the `cmd` indirection
- const isWin = process.platform === 'win32';
- const args = isWin ? ['/c', 'start', '""', resolvedPath] : [resolvedPath];
- const cmd = isWin ? 'cmd' : resolvedPath;
- const options = { shell: false, detached: true };
-
- desktopProc = spawn(cmd, args, options);
+ desktopProc = spawn(resolvedPath, [], { shell: false, detached: true, stdio: 'ignore' });
desktopProc.unref();
+ let settled = false;
desktopProc.on('error', e => {
log('Desktop app launch error:', e.message);
desktopProc = null;
+ if (!settled) { settled = true; reject(e); }
});
- setTimeout(resolve, 1500);
+ setTimeout(() => { if (!settled) { settled = true; resolve(); } }, 1500);📝 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.
| const isWin = process.platform === 'win32'; | |
| const args = isWin ? ['/c', 'start', '""', resolvedPath] : [resolvedPath]; | |
| const cmd = isWin ? 'cmd' : resolvedPath; | |
| const options = { shell: false, detached: true }; | |
| desktopProc = spawn(cmd, args, options); | |
| desktopProc.unref(); | |
| desktopProc = spawn(cmd, args, options); | |
| desktopProc.unref(); | |
| desktopProc = spawn(resolvedPath, [], { shell: false, detached: true, stdio: 'ignore' }); | |
| desktopProc.unref(); | |
| let settled = false; | |
| desktopProc.on('error', e => { | |
| log('Desktop app launch error:', e.message); | |
| desktopProc = null; | |
| if (!settled) { settled = true; reject(e); } | |
| }); | |
| setTimeout(() => { if (!settled) { settled = true; resolve(); } }, 1500); |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 79-79: Shell command built from environment values
This shell command depends on an uncontrolled absolute path.
This shell command depends on an uncontrolled absolute path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@browser-extension/native-host.js` around lines 74 - 80, Update the launch
flow around desktopProc and spawn to invoke resolvedPath directly on Windows
instead of using cmd with /c start, while preserving detached execution and
unref behavior. Also update the surrounding completion/error handling so the
launch promise rejects when the spawn error handler fires rather than resolving
unconditionally after the timeout.
Source: Linters/SAST tools
| "health.memory.label": "Bellek Kullanımı", | ||
| "health.memory.reason": "{pct}% bellek kullanımda.", | ||
| "health.load.label": "CPU Yükü", | ||
| "health.load.reason": "CPU yükü %{pct}%.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the health.load.reason placeholder.
The dashboard passes { pct: ... }. This value uses %{pct}%, which renders an extra leading %. Use {pct}%.
Proposed fix
- "health.load.reason": "CPU yükü %{pct}%.",
+ "health.load.reason": "CPU yükü {pct}%.",📝 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.
| "health.load.reason": "CPU yükü %{pct}%.", | |
| "health.load.reason": "CPU yükü {pct}%.", |
🧰 Tools
🪛 Biome (2.5.5)
[error] 812-812: The key health.load.reason was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/tr.json` at line 812, Update the health.load.reason
translation in tr.json to use the dashboard’s pct placeholder format {pct}%
instead of the current %{pct}%, preserving the intended percentage suffix
without the extra leading percent sign.
| if (!payload?.password) return { ok: false, error: 'Missing password' }; | ||
| const sha = crypto.createHash('sha1').update(payload.password).digest('hex').toUpperCase(); | ||
| const alert = { | ||
| level: 'danger', | ||
| source: 'Browser Extension', | ||
| title: 'Credential Leak Detected', | ||
| message: `Password found in ${payload.count} breach${payload.count > 1 ? 'es' : ''} via browser extension`, | ||
| detail: `SHA-1 prefix: ${sha.slice(0, 5)}... | Breaches: ${payload.count}`, | ||
| timestamp: new Date().toISOString(), | ||
| metadata: { source: 'browser-extension', hashPrefix: sha.slice(0, 5), count: payload.count } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate payload.count before it reaches the alert text.
Line 356 validates only payload.password. payload.count is used at lines 362, 363, and 365 without validation. When the extension omits count, the persisted alert reads "Password found in undefined breach via browser extension".
Coerce count to a finite non-negative integer and reject invalid values.
🐛 Proposed fix
if (!payload?.password) return { ok: false, error: 'Missing password' };
+ const count = Number(payload.count);
+ if (!Number.isInteger(count) || count < 1) return { ok: false, error: 'Invalid breach count' };
const sha = crypto.createHash('sha1').update(payload.password).digest('hex').toUpperCase();
const alert = {
level: 'danger',
source: 'Browser Extension',
title: 'Credential Leak Detected',
- message: `Password found in ${payload.count} breach${payload.count > 1 ? 'es' : ''} via browser extension`,
- detail: `SHA-1 prefix: ${sha.slice(0, 5)}... | Breaches: ${payload.count}`,
+ message: `Password found in ${count} breach${count > 1 ? 'es' : ''} via browser extension`,
+ detail: `SHA-1 prefix: ${sha.slice(0, 5)}... | Breaches: ${count}`,
timestamp: new Date().toISOString(),
- metadata: { source: 'browser-extension', hashPrefix: sha.slice(0, 5), count: payload.count }
+ metadata: { source: 'browser-extension', hashPrefix: sha.slice(0, 5), count }
};📝 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 (!payload?.password) return { ok: false, error: 'Missing password' }; | |
| const sha = crypto.createHash('sha1').update(payload.password).digest('hex').toUpperCase(); | |
| const alert = { | |
| level: 'danger', | |
| source: 'Browser Extension', | |
| title: 'Credential Leak Detected', | |
| message: `Password found in ${payload.count} breach${payload.count > 1 ? 'es' : ''} via browser extension`, | |
| detail: `SHA-1 prefix: ${sha.slice(0, 5)}... | Breaches: ${payload.count}`, | |
| timestamp: new Date().toISOString(), | |
| metadata: { source: 'browser-extension', hashPrefix: sha.slice(0, 5), count: payload.count } | |
| if (!payload?.password) return { ok: false, error: 'Missing password' }; | |
| const count = Number(payload.count); | |
| if (!Number.isInteger(count) || count < 1) return { ok: false, error: 'Invalid breach count' }; | |
| const sha = crypto.createHash('sha1').update(payload.password).digest('hex').toUpperCase(); | |
| const alert = { | |
| level: 'danger', | |
| source: 'Browser Extension', | |
| title: 'Credential Leak Detected', | |
| message: `Password found in ${count} breach${count > 1 ? 'es' : ''} via browser extension`, | |
| detail: `SHA-1 prefix: ${sha.slice(0, 5)}... | Breaches: ${count}`, | |
| timestamp: new Date().toISOString(), | |
| metadata: { source: 'browser-extension', hashPrefix: sha.slice(0, 5), count } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 356-356: Avoid SHA1 security protocol
Context: crypto.createHash('sha1')
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm (SHA-1).
(avoid-crypto-sha1)
[warning] 356-356: Do not use weak hash functions (MD5/SHA1)
Context: crypto.createHash('sha1')
Note: [CWE-328] Use of Weak Hash.
(insecure-hash)
🪛 GitHub Check: CodeQL
[failure] 357-357: Use of password hash with insufficient computational effort
Password from an access to password is hashed insecurely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc/system.js` around lines 356 - 365, Update the validation at the
start of the alert-building flow near the password check to coerce payload.count
to a finite non-negative integer and reject invalid values before constructing
alert. Reuse the validated count for the message, detail, and metadata fields in
the alert object.
| const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')); | ||
| const extId = process.env.SOTERIOS_EXT_ID || 'YOUR_EXTENSION_ID_HERE'; | ||
| manifest.allowed_origins = [manifest.allowed_origins[0].replace('<EXTENSION_ID>', extId)]; | ||
| fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2)); | ||
| const regPath = `HKCU\\Software\\Google\\Chrome\\NativeMessagingHosts\\${manifest.name}`; | ||
| const regCmd = `reg add "${regPath}" /ve /t REG_SZ /d "${manifestPath.replace(/\\/g, '\\\\')}" /f`; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
The in-place manifest rewrite is destructive and can leave an unrepairable state.
Line 387 consumes the <EXTENSION_ID> placeholder and line 388 overwrites the packaged template. Three failure modes follow:
- The rewrite is not idempotent. After the first run the placeholder is gone, so a later run cannot change the extension ID. The value written first is permanent.
manifest.allowed_origins[0]throws aTypeErrorwhenallowed_originsis missing or empty. Lines 385-390 sit outside thetryat line 391, so the throw rejects the IPC call. The caller atsrc/ui/js/pages/settings.js:403-429expects a structured{ ok: false, error }result and instead lands in its genericcatch.__dirnamepoints into the install directory. Under Program Files that directory is read-only, sofs.writeFileSyncthrowsEPERM. That throw also escapes thetry.
Keep the packaged manifest as a read-only template. Write the resolved manifest into app.getPath('userData') and register that path. Wrap the read, rewrite, and write in the try so every failure returns a structured error.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 387-387: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc/system.js` around lines 385 - 390, Update the manifest setup
around the manifest read, allowed_origins rewrite, and registry command so the
packaged manifest remains unchanged: read it as a template, resolve the
extension ID without assuming a nonempty allowed_origins array, write the
resolved manifest under app.getPath('userData'), and register that generated
path. Move the read, rewrite, and write operations inside the existing try block
so failures return the established structured { ok: false, error } result
instead of escaping the IPC handler.
| return { ok: false, error: 'Extension files not found. Reinstall Soterios.' }; | ||
| } | ||
| const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')); | ||
| const extId = process.env.SOTERIOS_EXT_ID || 'YOUR_EXTENSION_ID_HERE'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not register the placeholder extension ID and report success.
When SOTERIOS_EXT_ID is unset, extId becomes the literal 'YOUR_EXTENSION_ID_HERE'. The handler writes that value into allowed_origins, registers the manifest, and returns { ok: true }. Chrome then refuses the native-messaging connection for every real extension ID.
The settings toggle at src/ui/js/pages/settings.js:403-429 shows the installed message on result.ok, so the user sees success while the integration cannot work. Reject an unset or placeholder extension ID with a structured error.
🐛 Proposed fix
- const extId = process.env.SOTERIOS_EXT_ID || 'YOUR_EXTENSION_ID_HERE';
+ const extId = process.env.SOTERIOS_EXT_ID;
+ if (!extId || !/^[a-p]{32}$/.test(extId)) {
+ return { ok: false, error: 'Browser extension ID is not configured.' };
+ }📝 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.
| const extId = process.env.SOTERIOS_EXT_ID || 'YOUR_EXTENSION_ID_HERE'; | |
| const extId = process.env.SOTERIOS_EXT_ID; | |
| if (!extId || !/^[a-p]{32}$/.test(extId)) { | |
| return { ok: false, error: 'Browser extension ID is not configured.' }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc/system.js` at line 386, Validate SOTERIOS_EXT_ID in the handler
around extId before modifying allowed_origins or registering the manifest;
reject both an unset value and the literal placeholder YOUR_EXTENSION_ID_HERE
with a structured error response and do not report ok: true. Preserve the
existing registration and success flow for valid extension IDs, using the
response shape expected by the settings toggle.
| const regCmd = `reg add "${regPath}" /ve /t REG_SZ /d "${manifestPath.replace(/\\/g, '\\\\')}" /f`; | ||
| try { | ||
| execSync(regCmd, { stdio: 'ignore' }); | ||
| const regPathEdge = `HKCU\\Software\\Microsoft\\Edge\\NativeMessagingHosts\\${manifest.name}`; | ||
| const regCmdEdge = `reg add "${regPathEdge}" /ve /t REG_SZ /d "${manifestPath.replace(/\\/g, '\\\\')}" /f`; | ||
| try { execSync(regCmdEdge, { stdio: 'ignore' }); } catch (_) {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use execFileSync with an argument array, and remove the backslash doubling.
Two problems exist in these commands.
First, manifestPath.replace(/\\/g, '\\\\') doubles every backslash. reg add receives the /d value as a real process argument, not as a C string literal, so no escaping is required. The registry value is written as C:\\Users\\... instead of C:\Users\..., and Chrome cannot resolve the manifest path.
Second, regPath embeds manifest.name, which is read from a JSON file at line 385, and the whole command is passed to execSync as one shell string. OpenGrep flags lines 392 and 395 for this. execFileSync with an argument array removes the shell and the quoting concerns together.
🔒 Proposed fix
- const regPath = `HKCU\\Software\\Google\\Chrome\\NativeMessagingHosts\\${manifest.name}`;
- const regCmd = `reg add "${regPath}" /ve /t REG_SZ /d "${manifestPath.replace(/\\/g, '\\\\')}" /f`;
+ const { execFileSync } = require('child_process');
+ const regArgs = (root) => [
+ 'add', `HKCU\\Software\\${root}\\NativeMessagingHosts\\${manifest.name}`,
+ '/ve', '/t', 'REG_SZ', '/d', manifestPath, '/f',
+ ];
try {
- execSync(regCmd, { stdio: 'ignore' });
- const regPathEdge = `HKCU\\Software\\Microsoft\\Edge\\NativeMessagingHosts\\${manifest.name}`;
- const regCmdEdge = `reg add "${regPathEdge}" /ve /t REG_SZ /d "${manifestPath.replace(/\\/g, '\\\\')}" /f`;
- try { execSync(regCmdEdge, { stdio: 'ignore' }); } catch (_) {}
+ execFileSync('reg', regArgs('Google\\Chrome'), { stdio: 'ignore' });
+ try { execFileSync('reg', regArgs('Microsoft\\Edge'), { stdio: 'ignore' }); } catch (_) {}
return { ok: true };Validate manifest.name against /^[a-z0-9_.]+$/ before use, so a tampered manifest cannot redirect the registry key.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 392-392: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 395-395: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc/system.js` around lines 390 - 395, Replace the shell-string
execSync calls in the registry setup block with execFileSync('reg', argument
arrays), passing manifestPath unchanged without backslash doubling. Before
constructing regPath and regPathEdge, validate manifest.name against
/^[a-z0-9_.]+$/ and avoid using it if invalid; preserve the existing registry
operations for valid names.
Source: Linters/SAST tools
Only set externalLookupsEnabled to true on initial install when the value is undefined. This prevents user preferences from being overridden on extension or browser updates.
- Test credential-leak:notify IPC handler - Test browserExtension:installNativeHost IPC handler - Test background.js onInstalled behavior (security fix)
2 participants
replaces #91
Summary by CodeRabbit
New Features
Changes