feat: Browser Extension Integration + System Health/i18n Improvements - #91
feat: Browser Extension Integration + System Health/i18n Improvements#91chrisriv10 wants to merge 9 commits into
Conversation
- Add IPC handler 'credential-leak:notify' for browser extension - Add second-instance protocol handler (soterios://) - Scaffold browser-extension/ with manifest v3, background, content, popup, options - Native messaging host (native-host.js) bridges extension <-> desktop app - Install script tools/install-native-host.js
- Add 'Browser Extension Integration' feature toggle in Settings - IPC handler 'browserExtension:installNativeHost' installs native messaging host (Windows) - When enabled, installs native host for Chrome/Edge - i18n strings for install status
… sparkline - healthSummary.js: Return RTP, firewall, network stats, last scan - trayDashboard.html: Health score badge, RTP indicator, sparkline, quick scan button - trayDashboard.js: Already passes enhanced summary
- Add missing health.label.* keys to match dashboard usage - Translate English strings in it, tr, ru, pt-BR, ko, ja locales - Fix key structure (health.label.* vs health.malware.label)
- Add health.reason.* keys (noScan, noThreats, threatsFound, scanToday, scanDaysAgo, diskLowSpace, diskNoVolumes, diskHealthy, memoryUsage, cpuLoad, uptimeToday, uptimeDays, uptimeWeeks, uptimeLong, rtpActive, rtpDisabled, firewallActive, firewallDisabled) - Translate for: ar, de, es, fr, it, ja, ko, nl, pl, pt-BR, ru, tr - Fix key structure mismatches (health.label.* vs health.malware.label) - Spanish 'No threats found' now uses health.reason.noThreats
- Added health.reason.* keys for all locales - Added health.label.* keys for all locales - Fixed key structure (health.label.* vs health.malware.label) - Translated all English strings in it, tr, ru, pt-BR, ko, ja, nl, de, pl, ar, hi, fr - Spanish now has health.reason.noThreats for 'No threats found in the most recent scan'
📝 WalkthroughWalkthroughAdds a Manifest V3 browser extension with HIBP checks, password-field indicators, options, and native messaging. Extends tray health data and UI with RTP, firewall, network, scan details, and quick scans. Adds single-instance protocol handling and updates health-related translations. ChangesBrowser extension UI and settings
Native messaging bridge and installation
Desktop event and feature wiring
Tray health data and dashboard
Localization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BrowserExtension
participant NativeHost
participant DesktopApp
User->>BrowserExtension: Check password or report credential leak
BrowserExtension->>NativeHost: Send framed native message
NativeHost->>DesktopApp: Forward leak payload or launch app
NativeHost-->>BrowserExtension: Return framed response
BrowserExtension-->>User: Show breach or operation status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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}
PR Summary by QodoIntegrate MV3 browser extension via native host + expand tray health dashboard and i18n
AI Description
Diagram
High-Level Assessment
Files changed (40)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| try { | ||
| const result = await chrome.runtime.sendMessage({ type: 'CHECK_PASSWORD', password }); | ||
| showResult(el, result); | ||
| } catch (err) { | ||
| console.error('[Soterios] Check failed:', err); | ||
| } |
There was a problem hiding this comment.
3. Missing check_password handler 🐞 Bug ≡ Correctness
browser-extension/content.js sends a CHECK_PASSWORD message to the extension runtime, but browser-extension/background.js never registers runtime.onMessage to handle it. Inline icon checks will fail because there is no receiver for the message.
Agent Prompt
### Issue description
`content.js` uses `chrome.runtime.sendMessage({ type: 'CHECK_PASSWORD', ... })`, but the service worker does not handle this message type.
### Issue Context
Without a `chrome.runtime.onMessage` listener in `background.js`, the sendMessage call will reject/fail and no result badge can be shown.
### Fix
Add `chrome.runtime.onMessage.addListener(...)` in `background.js` to handle `CHECK_PASSWORD` (perform the HIBP k-anonymity call and respond), or move the HIBP check into `content.js` / `popup.js` consistently.
### Fix Focus Areas
- browser-extension/background.js[1-3]
- browser-extension/content.js[43-48]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
src/ui/pages/trayDashboard.html-218-220 (1)
218-220: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNetwork sparkline reads fields that
getTrayHealthSummarynever emits.This branch checks
summary.network.rx/summary.network.txand passes them todrawSparkline, butgetTrayHealthSummary(src/main/healthSummary.js Lines 43-52) returnsnetwork.rxKBs,network.txKBs, andnetwork.history— there is norx/txarray. As a result this condition is always false and the sparkline never renders. Additionally,drawSparkline'sformat()treats inputs as B/s whilehistoryis already in KB/s, so units would be wrong even if wired correctly.Align the consumer to the producer's
network.history(single series) or extendgetTrayHealthSummaryto also emit separaterx/txarrays.🤖 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/ui/pages/trayDashboard.html` around lines 218 - 220, Update the network sparkline branch in the tray dashboard to consume the `network.history` data emitted by `getTrayHealthSummary` instead of nonexistent `rx`/`tx` fields, adapting the `drawSparkline` call to its single-series shape. Ensure `drawSparkline` formats these history values as KB/s rather than B/s, preserving the producer’s units.tools/build-icons.js-19-21 (1)
19-21: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail the build when icon generation fails.
The catch block only logs the error, so the script exits successfully and packaging can emit an extension missing required icon sizes. Propagate the failure or set a non-zero exit code after the loop.
🤖 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 `@tools/build-icons.js` around lines 19 - 21, Update the catch block in the icon generation loop to preserve the failure status by propagating the error or setting a non-zero process exit code after generation fails. Keep the existing error logging, and ensure the build cannot exit successfully when any required icon generation fails.browser-extension/package.json-7-9 (1)
7-9: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix script paths relative to
browser-extension/package.json.npm runs these scripts with
browser-extensionas the working directory, but the helpers aretools/build-icons.jsandtools/install-native-host.jsat the repository root. Lines 7 and 9 therefore resolve to nonexistent paths, while line 8 attempts to enterbrowser-extensionagain.🤖 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` around lines 7 - 9, Update the build:icons, package, and install:host scripts in package.json to resolve helper paths from the repository root when invoked with browser-extension as the working directory. Remove the redundant directory change in package and reference the root-level tools scripts using the appropriate parent-relative paths.src/i18n/locales/pt-BR.json-807-819 (1)
807-819: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the duplicate health keys from all affected locales. These additions trigger Biome’s duplicate-object-key errors and make the effective translation depend on declaration order.
src/i18n/locales/pt-BR.json#L807-L819: remove the earlier copies or merge these Portuguese values into the existing keys.src/i18n/locales/ru.json#L803-L815: remove the earlier copies or merge these Russian values into the existing keys.src/i18n/locales/tr.json#L803-L815: remove the earlier copies or merge these Turkish values into the existing keys.🤖 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/pt-BR.json` around lines 807 - 819, Remove or merge the duplicate health translation keys in src/i18n/locales/pt-BR.json lines 807-819, src/i18n/locales/ru.json lines 803-815, and src/i18n/locales/tr.json lines 803-815. Preserve each locale’s intended translations under a single definition for every health.scanRecency, health.disk, health.memory, health.load, health.uptime, health.rtp, and health.firewall key.Source: Linters/SAST tools
browser-extension/package.json-8-8 (1)
8-8: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep the release archive limited to extension files.
After fixing the working-directory issue,
zip -r ... .will includenode_modulescreated for the build dependency. It also requires a Unixzipexecutable, which is not available in standard Windows environments. Use an explicit staging/file list and a cross-platform archiver.🤖 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.json “package” script to stage only browser-extension release files, explicitly excluding build dependencies such as node_modules, icons/*.svg, tools, and other non-extension artifacts. Replace the Unix-specific zip command with a cross-platform archiver already supported by the project, while preserving the icon build and generated soterios-extension.zip output.tools/build-icons.js-17-17 (1)
17-17: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid
npx -yin the icon build step
svgexportisn’t declared inpackage.json, so this can download and execute a registry package during packaging. Thecatchalso lets icon generation fail silently, which can ship an incomplete build. Use a declared local binary and fail the build on conversion errors.🤖 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 `@tools/build-icons.js` at line 17, Update the icon generation flow around the svgexport execSync call to use a declared local svgexport dependency/binary instead of npx -y, and make conversion errors propagate rather than being swallowed by the catch handler. Ensure the build exits unsuccessfully when any icon conversion fails.Source: Linters/SAST tools
src/i18n/locales/ja.json-765-778 (1)
765-778: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove duplicate locale-key declarations across both files.
The new translation blocks redeclare keys that already exist, triggering Biome’s duplicate-key errors and making effective values depend on parser behavior.
src/i18n/locales/ja.json#L765-L778: remove the duplicatehealth.malware.highdefinition and retain one canonical value.src/i18n/locales/ko.json#L804-L816: remove the duplicated health subkeys and retain one canonical definition for each key.🤖 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 765 - 778, Remove the duplicate locale-key declarations from src/i18n/locales/ja.json lines 765-778, keeping one canonical health.malware.high value. Also remove the duplicated health subkeys from src/i18n/locales/ko.json lines 804-816, retaining one canonical definition for each key so both locale files contain no duplicate keys.Source: Linters/SAST tools
src/ui/js/pages/settings.js-122-128 (1)
122-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the duplicate Network Perimeter Map toggle.
This duplicates
#networkPerimeterMapTogglefrom Lines 115-121. Only the first duplicate receives the event listener at Line 401, so the second visible control does nothing.🤖 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/ui/js/pages/settings.js` around lines 122 - 128, Remove the duplicate toggle-row containing `#networkPerimeterMapToggle` from the settings markup, preserving the earlier instance that receives the event listener. Keep the existing label, description, and functional toggle behavior represented by the first control.browser-extension/manifest.json-18-18 (1)
18-18: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAuthorize the localhost health endpoint.
browser-extension/popup.jsfetcheshttp://localhost:17234/api/health, butbrowser-extension/manifest.jsononly grantshttps://api.pwnedpasswords.com/*. Addhttp://localhost:17234/*so the popup can detect the desktop app when it is running.🤖 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/manifest.json` at line 18, Add the localhost origin pattern http://localhost:17234/* to the manifest host_permissions alongside the existing API permission, enabling popup.js to reach the desktop app health endpoint without changing the current permission.browser-extension/manifest.json-17-20 (1)
17-20: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegister the content script and add a localhost host permission.
browser-extension/content.jsis never loaded becausemanifest.jsonhas nocontent_scriptsentry, so the password-field overlay cannot appear.browser-extension/popup.jscallshttp://localhost:17234/api/health, but the manifest only grantshttps://api.pwnedpasswords.com/*; add a matching localhost host permission or move that check behind the background script.🤖 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/manifest.json` around lines 17 - 20, Update the manifest configuration to register browser-extension/content.js as a content script on the intended pages so the password-field overlay loads, and add host permission coverage for http://localhost:17234/* to support the health request made by browser-extension/popup.js. Preserve the existing storage and Have I Been Pwned permissions.src/main/ipcHandlers.js-937-941 (1)
937-941: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPersist the resolved native-host manifest before registering it.
manifest.allowed_originsis only updated in memory, butreg addstill points Chrome/Edge atbrowser-extension/native-host-manifest.json, which keeps<EXTENSION_ID>on disk. Write the updated manifest to a writable installed-host path and register that file instead.🤖 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/ipcHandlers.js` around lines 937 - 941, Update the native-host registration flow around manifest parsing in ipcHandlers.js to persist the resolved manifest, including the substituted extension ID, to a writable installed-host path before invoking reg add. Use that persisted file path in the registry command instead of the source browser-extension/native-host-manifest.json path, while preserving the existing manifest name and allowed_origins handling.browser-extension/content.js-94-99 (1)
94-99: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFix the icon cleanup path.
passwordFieldsis aWeakMap, so theSETTINGS_UPDATEDhandler will throw onforEach/clear. The blur handler also only removes the icon DOM node; it leavesdata-soterios-idand the window listeners behind, so the field can’t be re-iconed and the listeners accumulate. Use a shared cleanup helper that removes the listeners, deletes the entry, and clears the dataset key.🤖 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` around lines 94 - 99, Replace the inline blur cleanup near the passwordFields.set call with a shared cleanup helper that removes the icon, unregisters the scroll and resize listeners, deletes the input from the passwordFields WeakMap, and clears its data-soterios-id attribute. Update the SETTINGS_UPDATED cleanup path to use this helper instead of calling forEach or clear on passwordFields, while preserving one-time cleanup behavior.browser-extension/popup.js-13-20 (1)
13-20: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTreat HIBP HTTP errors as failures
A non-OK response still gets parsed and can fall through toreturn 0, so rate limits or outages show as “Not found in breaches.” Checkresp.okbefore reading the body.🤖 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/popup.js` around lines 13 - 20, Update the HIBP fetch flow around resp and the response parsing loop to check resp.ok immediately after fetch and before calling resp.text(). Treat non-OK responses as failures using the existing error-handling path, rather than parsing the body or returning 0; preserve the current suffix matching and successful-response behavior.browser-extension/background.js-1-3 (1)
1-3: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd a
CHECK_PASSWORDmessage handler inbrowser-extension/background.js.
browser-extension/content.jssends{ type: 'CHECK_PASSWORD' }, but this worker only initializes storage. Return{ pwned, count }here soshowResultreceives the expected payload.🤖 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/background.js` around lines 1 - 3, Add a CHECK_PASSWORD message handler in the background worker alongside the existing onInstalled listener. Handle messages sent by content.js, perform the password lookup using the existing project mechanism, and respond asynchronously with an object containing the expected pwned and count fields so showResult receives the correct payload.src/ui/js/pages/settings.js-422-424 (1)
422-424: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRollback the stored
browserExtensionflag on rejectionIf
window.api.invoke('browserExtension:installNativeHost')rejects afterbrowserExtension: trueis already saved, the catch only resets the checkbox. PersistbrowserExtension: falsethere too so reopening settings doesn’t still show the integration enabled.🤖 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/ui/js/pages/settings.js` around lines 422 - 424, Update the catch handler for the browserExtension:installNativeHost invocation to persist browserExtension as false when installation is rejected, in addition to resetting event.target.checked. Keep the existing error status message behavior unchanged.browser-extension/native-host.js-83-86 (1)
83-86: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCredential-leak acknowledgements are disconnected from desktop delivery. Both host implementations report success despite not confirming that the desktop application received the event.
browser-extension/native-host.js#L83-L86: transmit the leak payload through the desktop IPC/protocol contract and acknowledge only after acceptance.browser-extension-host.js#L76-L80: reconnect or return an explicit delivery failure instead of dropping the event with{ ok: true }.🤖 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 83 - 86, Update the CREDENTIAL_LEAK handling in browser-extension/native-host.js at lines 83-86 to send the leak payload through the desktop IPC/protocol contract and acknowledge with LEAK_NOTIFIED only after delivery is accepted; update browser-extension-host.js at lines 76-80 to reconnect and deliver the event, or return an explicit delivery failure instead of reporting { ok: true } when delivery is not confirmed.browser-extension/native-host.js-57-75 (1)
57-75: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset the launch state on exit and surface spawn failures.
desktopProcstays truthy after the spawned launcher exits, so later calls are skipped even though the app is gone. Clear the reference onexit/close, and reject onerrorinstead of resolving unconditionally soAPP_OPENEDisn’t reported after a failed launch.🤖 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 57 - 75, Update launchDesktopApp so desktopProc is cleared when the spawned launcher emits exit or close, allowing subsequent launches. In the existing error handler, log the failure, reset desktopProc, and reject the promise; ensure the delayed resolve only occurs for successful launches so APP_OPENED is not reported after spawn failure.tools/install-native-host.js-47-55 (1)
47-55: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInstall a platform-appropriate native-host launcher. The non-Windows path still writes
browser-extension/native-host-manifest.jsonunchanged, and that manifest points tonative-host.bat, which macOS/Linux can’t execute. Use a platform-specific manifest and point non-Windows installs at an executable launcher such asbrowser-extension/native-host.jswith execute permissions.🤖 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 `@tools/install-native-host.js` around lines 47 - 55, Update the non-Windows installation branch around manifest writing to create a platform-specific manifest whose native host path points to the executable browser-extension/native-host.js launcher instead of native-host.bat. Ensure the launcher has execute permissions before installing it, while preserving the existing Windows manifest behavior and destination handling.browser-extension/native-host.js-61-67 (1)
61-67: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid shell-interpolating
process.env.SOTERIOS_APP_PATH.spawn(..., { shell: true })turns that env value into a command string on Windows/Linux, so quotes or shell metacharacters can break out and run arbitrary commands. Use platform-specific binaries with argument arrays andshell: false;desktopProcshould also be cleared on child exit so later launches still work.🤖 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 61 - 67, Update the desktop launch logic around desktopProc to avoid constructing a shell command from DESKTOP_APP: select platform-specific executables and argument arrays, invoke spawn with shell: false, and preserve the existing Windows, macOS, and Linux launch behavior. Attach an exit/close handler that clears desktopProc when the child terminates so subsequent launches can start a new process.Source: Linters/SAST tools
tools/install-native-host.js-11-32 (1)
11-32: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrite the resolved manifest before Windows registration. In both
tools/install-native-host.jsandsrc/main/ipcHandlers.js,<EXTENSION_ID>is replaced only in memory, but Chrome/Edge is still pointed atbrowser-extension/native-host-manifest.json, so the placeholder stays on disk. Generate a substituted copy and register that file instead.🤖 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 `@tools/install-native-host.js` around lines 11 - 32, Write the substituted manifest containing the resolved allowed_origins to a generated file before registration, and point the Windows registry command in main() to that generated file instead of the unresolved native-host-manifest.json. Apply the corresponding manifest-registration update in browser-extension-host.json (lines 6-8) so it also references the resolved copy; update tools/install-native-host.js (lines 11-32) directly, while preserving the existing extension ID substitution.tools/install-native-host.js-11-11 (1)
11-11: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrite a resolved, platform-specific manifest before registering it.
EXTENSION_IDstill falls back toYOUR_EXTENSION_ID_HERE, and Windows registersbrowser-extension/native-host-manifest.jsonwithout persisting the updatedallowed_origins, so the browser keeps the placeholder origin. The Linux/macOS install path also writes the same template manifest, whosepathstill points atnative-host.bat. Reject placeholder IDs and emit the manifest to an install location per platform.🤖 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 `@tools/install-native-host.js` at line 11, Update the install flow around EXTENSION_ID and platform-specific registration to reject the YOUR_EXTENSION_ID_HERE placeholder, resolve allowed_origins from the actual extension ID, and write a platform-specific manifest to the platform’s install location before registering it. Ensure Windows persists the resolved manifest instead of registering the template, and Linux/macOS use a native-host executable path rather than the template’s native-host.bat path.
🟡 Minor comments (11)
src/main/healthSummary.js-45-52 (1)
45-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard against
NaNin sparkline samples and rename the shadowinglatest.
h.tx_bytes + h.rx_byteslacks the|| 0fallback used just above forrxKBs/txKBs; a missing field yieldsNaN, which will break the sparkline scaling downstream. The innerlatestalso shadows the outer scan-reportlatestfrom Line 9, which is confusing.🐛 Proposed fix
- const history = db.getNetworkHistory ? db.getNetworkHistory(24 * 60) : []; // last 24h, 1 sample per min - if (history.length) { - const latest = history[history.length - 1]; - network.rxKBs = Math.round((latest.rx_bytes || 0) / 1024); - network.txKBs = Math.round((latest.tx_bytes || 0) / 1024); - network.history = history.slice(-60).map(h => (h.tx_bytes + h.rx_bytes) / 1024); // last 60 samples for sparkline + const history = db.getNetworkHistory ? db.getNetworkHistory(24 * 60) : []; // last 24h, 1 sample per min + if (history.length) { + const latestSample = history[history.length - 1]; + network.rxKBs = Math.round((latestSample.rx_bytes || 0) / 1024); + network.txKBs = Math.round((latestSample.tx_bytes || 0) / 1024); + network.history = history.slice(-60).map(h => ((h.tx_bytes || 0) + (h.rx_bytes || 0)) / 1024); // last 60 samples }🤖 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/healthSummary.js` around lines 45 - 52, Update the network history mapping in the health-summary logic to default both h.tx_bytes and h.rx_bytes to zero before calculating each sparkline sample, preventing NaN values. Rename the inner latest variable used for the newest history entry to avoid shadowing the outer scan-report latest.src/i18n/locales/tr.json-905-909 (1)
905-909: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign Turkish health placeholders and percent formatting with the dashboard contract.
src/ui/js/pages/dashboard.jssuppliespct, notusage, to the disk reason translations. Also,%{pct}%produces an extra percent sign. Use{pct}%or%{pct}consistently for the target Turkish phrasing.🤖 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` around lines 905 - 909, Update the Turkish health translation entries around health.reason.diskHealthy and health.reason.memoryUsage to use the dashboard contract’s pct placeholder instead of usage, and remove the duplicated percent sign by using one consistent {pct}% or %{pct} format. Preserve the intended Turkish wording and apply the corrected placeholder formatting to the affected percentage messages.src/i18n/locales/en.json-124-124 (1)
124-124: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the error argument or remove the placeholder.
settings.browserExtension.installFailedincludes{error}, but the call site can invoke it without interpolation args, so the UI may show the placeholder literally.🤖 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/en.json` at line 124, Update the settings.browserExtension.installFailed translation and its call site so they agree: pass the actual error interpolation argument whenever the message is used, or remove the {error} placeholder if no error is available. Ensure the UI never displays the placeholder literally.src/i18n/locales/nl.json-900-900 (1)
900-900: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate
scanIndicator.scanninginto Dutch.
"Scanning…"remains English while the other scan-indicator statuses are localized. Use a Dutch equivalent such as"Scannen…".🤖 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/nl.json` at line 900, Translate the scanIndicator.scanning value in the Dutch locale from English to Dutch, using “Scannen…” while preserving the existing key and ellipsis style.src/i18n/locales/ja.json-866-866 (1)
866-866: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the incomplete scan-time phrase.
"過去 1 以内"is missing the unit (日), so the UI displays malformed Japanese. It should express “within the last day.”🤖 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` at line 866, Update the Japanese translation for health.reason.scanToday so the incomplete “過去 1 以内” phrase includes the day unit and clearly expresses “within the last day.”src/i18n/locales/ja.json-767-767 (1)
767-767: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a natural Japanese translation for scan recency.
"スキャン時効性"is not a natural label for recency and can be misunderstood. Use wording such as “スキャンの最新性” or “最後のスキャン” instead.🤖 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` at line 767, Update the Japanese translation for the health.label.scanRecency key from the unnatural “スキャン時効性” to a natural label such as “スキャンの最新性” or “最後のスキャン”.src/i18n/locales/it.json-790-804 (1)
790-804: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPolish the Italian health translations.
Several strings have incorrect grammar (
"Nessun scansione","nell'ultimo scansione"), awkward placeholder text ("Trovata/e"), or leave"Recency"untranslated.Also applies to: 886-890
🤖 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/it.json` around lines 790 - 804, Polish the Italian health translations in the malware and scan-recency entries, including the corresponding entries around the referenced later section. Correct article and gender agreement for “scansione,” replace awkward “Trovata/e” wording with natural Italian while preserving {count} and {days}, and translate “Recency” consistently in the relevant labels.src/i18n/locales/de.json-885-885 (1)
885-885: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate
scanIndicator.threatsFound.This value is still English (
"{count} threat(s) found"), so German users see untranslated text and an awkward literal plural marker.🤖 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/de.json` at line 885, Update the German locale value for scanIndicator.threatsFound to a natural German translation, replacing the English text and literal “(s)” marker while preserving the {count} interpolation placeholder.browser-extension/popup.js-41-51 (1)
41-51: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
AbortSignal.timeout()here
fetchignores thetimeoutoption, so this health check can hang and overlap with the 30s interval. Abort it after 1s instead.🤖 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/popup.js` around lines 41 - 51, Update checkConnection to replace fetch’s unsupported timeout option with an AbortSignal.timeout(1000) signal, ensuring the health request aborts after one second while preserving the existing success and catch handling.browser-extension/native-host.bat-5-5 (1)
5-5: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winQuote the batch variable assignment.
An installation path containing shell metacharacters can alter parsing here. Use the safe assignment form.
Proposed fix
- set NODE_PATH=%~dp0..\..\node_modules + set "NODE_PATH=%~dp0..\..\node_modules"🤖 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.bat` at line 5, Update the NODE_PATH assignment in the batch script to quote the path value safely, preserving the existing %~dp0 relative-path resolution while preventing shell metacharacters in the installation path from altering parsing.Source: Linters/SAST tools
browser-extension/options.html-38-38 (1)
38-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a switch role to these toggle buttons.
aria-checkedon a plainbuttonwon’t expose the on/off state correctly to assistive tech; userole="switch"on all four controls, or replace them with native checkboxes.🤖 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/options.html` at line 38, Add role="switch" to all four toggle buttons in the options UI, including the control identified by id="hibpEnabled", while preserving their existing aria-checked state and labels.
🧹 Nitpick comments (3)
src/ui/pages/trayDashboard.js (1)
61-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the summary-rendering logic.
loadSummary(Lines 61-104) re-implements the exact score/RTP/firewall/network/last-scan rendering already present in thetray:summaryhandler (Lines 1-59). Extract a singlerenderSummary(summary)and call it from both the event handler andloadSummaryto prevent the two paths from drifting.🤖 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/ui/pages/trayDashboard.js` around lines 61 - 104, Extract the shared score, detail, RTP, firewall, network, and last-scan DOM update logic from loadSummary into a renderSummary(summary) function. Replace the duplicated rendering in both the tray:summary event handler and loadSummary with calls to renderSummary, while preserving each path’s existing loading and error handling.src/main/healthSummary.js (2)
34-40: 🚀 Performance & Scalability | 🔵 Trivial
netshis spawned on every tray summary refresh.
getTrayHealthSummaryis invoked on the tray refresh cycle (~15s), so this launches anetshchild process each time. On non-Windows platforms it fails fast (caught, falls back toactive:false), but on Windows it's a recurring process spawn just for firewall state. Consider caching the result for a short TTL or reading it less frequently.🤖 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/healthSummary.js` around lines 34 - 40, Update the firewall-state logic in getTrayHealthSummary to avoid invoking netsh on every tray refresh. Cache the resolved firewall status with a short time-to-live, reuse the cached value while valid, and only rerun the existing execFileAsync check after the cache expires; preserve the current active:false fallback when the command fails.Source: Linters/SAST tools
24-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDrop the unused
RealTimeWatcherrequire
rtpcomes entirely fromdb.getSetting, so this import only adds a silent failure path and can force{ enabled: false }if the module load fails.🤖 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/healthSummary.js` around lines 24 - 30, Remove the unused RealTimeWatcher require from the health summary initialization, leaving rtp.enabled sourced directly from db.getSetting('feature.realtimeProtection', false). Preserve the existing fallback behavior only for failures in the settings lookup.
🤖 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.
Major comments:
In `@browser-extension/background.js`:
- Around line 1-3: Add a CHECK_PASSWORD message handler in the background worker
alongside the existing onInstalled listener. Handle messages sent by content.js,
perform the password lookup using the existing project mechanism, and respond
asynchronously with an object containing the expected pwned and count fields so
showResult receives the correct payload.
In `@browser-extension/content.js`:
- Around line 94-99: Replace the inline blur cleanup near the passwordFields.set
call with a shared cleanup helper that removes the icon, unregisters the scroll
and resize listeners, deletes the input from the passwordFields WeakMap, and
clears its data-soterios-id attribute. Update the SETTINGS_UPDATED cleanup path
to use this helper instead of calling forEach or clear on passwordFields, while
preserving one-time cleanup behavior.
In `@browser-extension/manifest.json`:
- Line 18: Add the localhost origin pattern http://localhost:17234/* to the
manifest host_permissions alongside the existing API permission, enabling
popup.js to reach the desktop app health endpoint without changing the current
permission.
- Around line 17-20: Update the manifest configuration to register
browser-extension/content.js as a content script on the intended pages so the
password-field overlay loads, and add host permission coverage for
http://localhost:17234/* to support the health request made by
browser-extension/popup.js. Preserve the existing storage and Have I Been Pwned
permissions.
In `@browser-extension/native-host.js`:
- Around line 83-86: Update the CREDENTIAL_LEAK handling in
browser-extension/native-host.js at lines 83-86 to send the leak payload through
the desktop IPC/protocol contract and acknowledge with LEAK_NOTIFIED only after
delivery is accepted; update browser-extension-host.js at lines 76-80 to
reconnect and deliver the event, or return an explicit delivery failure instead
of reporting { ok: true } when delivery is not confirmed.
- Around line 57-75: Update launchDesktopApp so desktopProc is cleared when the
spawned launcher emits exit or close, allowing subsequent launches. In the
existing error handler, log the failure, reset desktopProc, and reject the
promise; ensure the delayed resolve only occurs for successful launches so
APP_OPENED is not reported after spawn failure.
- Around line 61-67: Update the desktop launch logic around desktopProc to avoid
constructing a shell command from DESKTOP_APP: select platform-specific
executables and argument arrays, invoke spawn with shell: false, and preserve
the existing Windows, macOS, and Linux launch behavior. Attach an exit/close
handler that clears desktopProc when the child terminates so subsequent launches
can start a new process.
In `@browser-extension/package.json`:
- Around line 7-9: Update the build:icons, package, and install:host scripts in
package.json to resolve helper paths from the repository root when invoked with
browser-extension as the working directory. Remove the redundant directory
change in package and reference the root-level tools scripts using the
appropriate parent-relative paths.
- Line 8: Update the package.json “package” script to stage only
browser-extension release files, explicitly excluding build dependencies such as
node_modules, icons/*.svg, tools, and other non-extension artifacts. Replace the
Unix-specific zip command with a cross-platform archiver already supported by
the project, while preserving the icon build and generated
soterios-extension.zip output.
In `@browser-extension/popup.js`:
- Around line 13-20: Update the HIBP fetch flow around resp and the response
parsing loop to check resp.ok immediately after fetch and before calling
resp.text(). Treat non-OK responses as failures using the existing
error-handling path, rather than parsing the body or returning 0; preserve the
current suffix matching and successful-response behavior.
In `@src/i18n/locales/ja.json`:
- Around line 765-778: Remove the duplicate locale-key declarations from
src/i18n/locales/ja.json lines 765-778, keeping one canonical
health.malware.high value. Also remove the duplicated health subkeys from
src/i18n/locales/ko.json lines 804-816, retaining one canonical definition for
each key so both locale files contain no duplicate keys.
In `@src/i18n/locales/pt-BR.json`:
- Around line 807-819: Remove or merge the duplicate health translation keys in
src/i18n/locales/pt-BR.json lines 807-819, src/i18n/locales/ru.json lines
803-815, and src/i18n/locales/tr.json lines 803-815. Preserve each locale’s
intended translations under a single definition for every health.scanRecency,
health.disk, health.memory, health.load, health.uptime, health.rtp, and
health.firewall key.
In `@src/main/ipcHandlers.js`:
- Around line 937-941: Update the native-host registration flow around manifest
parsing in ipcHandlers.js to persist the resolved manifest, including the
substituted extension ID, to a writable installed-host path before invoking reg
add. Use that persisted file path in the registry command instead of the source
browser-extension/native-host-manifest.json path, while preserving the existing
manifest name and allowed_origins handling.
In `@src/ui/js/pages/settings.js`:
- Around line 122-128: Remove the duplicate toggle-row containing
`#networkPerimeterMapToggle` from the settings markup, preserving the earlier
instance that receives the event listener. Keep the existing label, description,
and functional toggle behavior represented by the first control.
- Around line 422-424: Update the catch handler for the
browserExtension:installNativeHost invocation to persist browserExtension as
false when installation is rejected, in addition to resetting
event.target.checked. Keep the existing error status message behavior unchanged.
In `@src/ui/pages/trayDashboard.html`:
- Around line 218-220: Update the network sparkline branch in the tray dashboard
to consume the `network.history` data emitted by `getTrayHealthSummary` instead
of nonexistent `rx`/`tx` fields, adapting the `drawSparkline` call to its
single-series shape. Ensure `drawSparkline` formats these history values as KB/s
rather than B/s, preserving the producer’s units.
In `@tools/build-icons.js`:
- Around line 19-21: Update the catch block in the icon generation loop to
preserve the failure status by propagating the error or setting a non-zero
process exit code after generation fails. Keep the existing error logging, and
ensure the build cannot exit successfully when any required icon generation
fails.
- Line 17: Update the icon generation flow around the svgexport execSync call to
use a declared local svgexport dependency/binary instead of npx -y, and make
conversion errors propagate rather than being swallowed by the catch handler.
Ensure the build exits unsuccessfully when any icon conversion fails.
In `@tools/install-native-host.js`:
- Around line 47-55: Update the non-Windows installation branch around manifest
writing to create a platform-specific manifest whose native host path points to
the executable browser-extension/native-host.js launcher instead of
native-host.bat. Ensure the launcher has execute permissions before installing
it, while preserving the existing Windows manifest behavior and destination
handling.
- Around line 11-32: Write the substituted manifest containing the resolved
allowed_origins to a generated file before registration, and point the Windows
registry command in main() to that generated file instead of the unresolved
native-host-manifest.json. Apply the corresponding manifest-registration update
in browser-extension-host.json (lines 6-8) so it also references the resolved
copy; update tools/install-native-host.js (lines 11-32) directly, while
preserving the existing extension ID substitution.
- Line 11: Update the install flow around EXTENSION_ID and platform-specific
registration to reject the YOUR_EXTENSION_ID_HERE placeholder, resolve
allowed_origins from the actual extension ID, and write a platform-specific
manifest to the platform’s install location before registering it. Ensure
Windows persists the resolved manifest instead of registering the template, and
Linux/macOS use a native-host executable path rather than the template’s
native-host.bat path.
---
Minor comments:
In `@browser-extension/native-host.bat`:
- Line 5: Update the NODE_PATH assignment in the batch script to quote the path
value safely, preserving the existing %~dp0 relative-path resolution while
preventing shell metacharacters in the installation path from altering parsing.
In `@browser-extension/options.html`:
- Line 38: Add role="switch" to all four toggle buttons in the options UI,
including the control identified by id="hibpEnabled", while preserving their
existing aria-checked state and labels.
In `@browser-extension/popup.js`:
- Around line 41-51: Update checkConnection to replace fetch’s unsupported
timeout option with an AbortSignal.timeout(1000) signal, ensuring the health
request aborts after one second while preserving the existing success and catch
handling.
In `@src/i18n/locales/de.json`:
- Line 885: Update the German locale value for scanIndicator.threatsFound to a
natural German translation, replacing the English text and literal “(s)” marker
while preserving the {count} interpolation placeholder.
In `@src/i18n/locales/en.json`:
- Line 124: Update the settings.browserExtension.installFailed translation and
its call site so they agree: pass the actual error interpolation argument
whenever the message is used, or remove the {error} placeholder if no error is
available. Ensure the UI never displays the placeholder literally.
In `@src/i18n/locales/it.json`:
- Around line 790-804: Polish the Italian health translations in the malware and
scan-recency entries, including the corresponding entries around the referenced
later section. Correct article and gender agreement for “scansione,” replace
awkward “Trovata/e” wording with natural Italian while preserving {count} and
{days}, and translate “Recency” consistently in the relevant labels.
In `@src/i18n/locales/ja.json`:
- Line 866: Update the Japanese translation for health.reason.scanToday so the
incomplete “過去 1 以内” phrase includes the day unit and clearly expresses “within
the last day.”
- Line 767: Update the Japanese translation for the health.label.scanRecency key
from the unnatural “スキャン時効性” to a natural label such as “スキャンの最新性” or “最後のスキャン”.
In `@src/i18n/locales/nl.json`:
- Line 900: Translate the scanIndicator.scanning value in the Dutch locale from
English to Dutch, using “Scannen…” while preserving the existing key and
ellipsis style.
In `@src/i18n/locales/tr.json`:
- Around line 905-909: Update the Turkish health translation entries around
health.reason.diskHealthy and health.reason.memoryUsage to use the dashboard
contract’s pct placeholder instead of usage, and remove the duplicated percent
sign by using one consistent {pct}% or %{pct} format. Preserve the intended
Turkish wording and apply the corrected placeholder formatting to the affected
percentage messages.
In `@src/main/healthSummary.js`:
- Around line 45-52: Update the network history mapping in the health-summary
logic to default both h.tx_bytes and h.rx_bytes to zero before calculating each
sparkline sample, preventing NaN values. Rename the inner latest variable used
for the newest history entry to avoid shadowing the outer scan-report latest.
---
Nitpick comments:
In `@src/main/healthSummary.js`:
- Around line 34-40: Update the firewall-state logic in getTrayHealthSummary to
avoid invoking netsh on every tray refresh. Cache the resolved firewall status
with a short time-to-live, reuse the cached value while valid, and only rerun
the existing execFileAsync check after the cache expires; preserve the current
active:false fallback when the command fails.
- Around line 24-30: Remove the unused RealTimeWatcher require from the health
summary initialization, leaving rtp.enabled sourced directly from
db.getSetting('feature.realtimeProtection', false). Preserve the existing
fallback behavior only for failures in the settings lookup.
In `@src/ui/pages/trayDashboard.js`:
- Around line 61-104: Extract the shared score, detail, RTP, firewall, network,
and last-scan DOM update logic from loadSummary into a renderSummary(summary)
function. Replace the duplicated rendering in both the tray:summary event
handler and loadSummary with calls to renderSummary, while preserving each
path’s existing loading and error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b0ce2a5-4611-4a41-bd23-cb2a90d330d5
⛔ Files ignored due to path filters (5)
browser-extension/icons/icon.svgis excluded by!**/*.svgbrowser-extension/icons/icon128.pngis excluded by!**/*.pngbrowser-extension/icons/icon16.pngis excluded by!**/*.pngbrowser-extension/icons/icon32.pngis excluded by!**/*.pngbrowser-extension/icons/icon48.pngis excluded by!**/*.png
📒 Files selected for processing (35)
browser-extension-host.jsbrowser-extension-host.jsonbrowser-extension/background.jsbrowser-extension/content.jsbrowser-extension/manifest.jsonbrowser-extension/native-host-manifest.jsonbrowser-extension/native-host.batbrowser-extension/native-host.jsbrowser-extension/options.htmlbrowser-extension/options.jsbrowser-extension/package.jsonbrowser-extension/popup.htmlbrowser-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/main/healthSummary.jssrc/main/ipcHandlers.jssrc/main/main.jssrc/ui/js/pages/settings.jssrc/ui/pages/trayDashboard.htmlsrc/ui/pages/trayDashboard.jstools/build-icons.jstools/install-native-host.js
- Fix WeakMap -> Map in content.js (forEach/clear support) - Add content_scripts to manifest.json - Add CHECK_PASSWORD handler in background.js - Add nativeMessaging permission to manifest.json - Fix installer to write updated manifest back to disk - Fix duplicate toggle IDs in settings.js - Fix tray sparkline data mismatch (healthSummary.js -> tray) - Fix Turkish locale JSON (extra brace) - Fix fetch timeout in popup.js (use AbortController) - Fix shell injection in native-host.js (no shell, validate env) - Add CHECK_PASSWORD handler in background.js - Add nativeMessaging permission - Fix installer to write manifest back to disk - Fix duplicate toggle IDs - Fix tray sparkline data shape - Fix Turkish locale JSON - Fix fetch timeout with AbortController - Fix shell injection in native-host.js
| const cmd = isWin ? 'cmd' : resolvedPath; | ||
| const options = { shell: false, detached: true }; | ||
|
|
||
| desktopProc = spawn(cmd, args, options); |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/ipcHandlers.js (1)
769-780: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject invalid breach counts before persisting an alert.
payload.countis trusted without validation, so0, negative,NaN, or non-numeric values still create adangeralert. Normalize it to a finite positive integer before callingdb.addAlert.🤖 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/ipcHandlers.js` around lines 769 - 780, Validate and normalize payload.count in the credential-leak:notify handler before constructing or persisting the alert: accept only finite positive integers, and return an appropriate failure response for invalid values. Use the normalized count consistently in the alert message, detail, and metadata, including the db.addAlert call.browser-extension/native-host.js (3)
91-113: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
CREDENTIAL_LEAK/OPEN_APPhandlers don't guard againstlaunchDesktopApp()rejecting.Unlike the default branch (which sends an
ERRORframe for unknown types), theCREDENTIAL_LEAK/OPEN_APPcasesawait launchDesktopApp()with no try/catch. If it rejects (missingDESKTOP_APP, missing file, or spawn error once Line 80-89 is fixed to reject), the rejection escapes to the process-levelunhandledRejectionhandler, which only logs — no response frame is ever sent back to the extension, leavingbackground.js's corresponding message-port wait hanging indefinitely.case 'CREDENTIAL_LEAK': try { await launchDesktopApp(); send({ type: 'LEAK_NOTIFIED' }); } catch (e) { send({ type: 'ERROR', error: e.message, original: msg }); } break;🤖 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 91 - 113, Update the CREDENTIAL_LEAK and OPEN_APP branches in handleMessage to catch launchDesktopApp() rejections and send an ERROR response containing the failure message and original request. Preserve the existing success response types and ensure each branch completes without allowing the rejection to escape to the process-level handler.
80-89: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSpawn failures never reject the promise, and
desktopProcis never cleared on normal exit.
desktopProc.on('error', ...)only logs and nullsdesktopProc; it never callsreject, so the fixedsetTimeout(resolve, 1500)always resolves the promise regardless of outcome — callers (handleMessage) will report success (LEAK_NOTIFIED/APP_OPENED) even when the launch failed. There's also noexit/closelistener, so once the desktop app terminates normally,desktopProcstays non-null forever and the top-of-function guard (if (desktopProc) return Promise.resolve();) permanently skips relaunching on subsequentCREDENTIAL_LEAK/OPEN_APPmessages.🔧 Proposed fix
desktopProc.unref(); - desktopProc.on('error', e => { - log('Desktop app launch error:', e.message); - desktopProc = null; - }); - - setTimeout(resolve, 1500); + const timer = setTimeout(resolve, 1500); + + desktopProc.on('error', e => { + log('Desktop app launch error:', e.message); + desktopProc = null; + clearTimeout(timer); + reject(e); + }); + + desktopProc.on('exit', () => { + desktopProc = null; + });🤖 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 80 - 89, Update the desktop process launch promise around desktopProc.on('error') so spawn failures call reject with the launch error instead of allowing the fixed timeout to resolve successfully. Add an exit or close listener that clears desktopProc when the child terminates, while preserving the existing launch-success resolution and top-of-function guard behavior.
115-123: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCatch rejected handler promises and send an error reply.
readMessages()callshandleMessage()withoutawaitor.catch(), so a failedlaunchDesktopApp()only reachesunhandledRejectionand the extension gets no response. Handle the promise rejection here and send theERRORframe instead.🤖 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 115 - 123, Update readMessages() so the handleMessage() promise is awaited or has a rejection handler that logs the failure and sends an ERROR frame containing the error message, ensuring launchDesktopApp() failures receive a response instead of only reaching the global unhandledRejection handler.
🤖 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 5-11: Update the CHECK_PASSWORD handler’s
checkPassword(msg.password) promise chain to catch all rejections, including
failures before the internal fetch handling, and call sendResponse with an
appropriate error response so the content script always receives a reply while
preserving the async return behavior.
- Around line 34-63: Add an abort timeout to the fetch request in checkPassword
so stalled HIBP calls terminate and the existing catch path returns an error
instead of leaving CHECK_PASSWORD unresolved. Create and pass an AbortSignal to
fetch, ensure the timer is cleaned up after completion, and preserve the current
response parsing and error handling behavior.
In `@browser-extension/native-host.js`:
- Around line 74-79: Update the desktop launch logic around the spawn call to
invoke resolvedPath directly with no arguments, removing the platform-specific
cmd/start command construction. Preserve detached execution with shell disabled
and add windowsHide: true to the spawn options.
In `@src/main/ipcHandlers.js`:
- Around line 940-941: Update native-host manifest generation in
src/main/ipcHandlers.js lines 940-941 and tools/install-native-host.js lines
28-29 to be idempotent: validate SOTERIOS_EXT_ID, load an immutable template or
replace the extension origin on every run, then persist/register/copy the
freshly generated manifest. Apply the same logic in both installers so stale or
placeholder origins cannot survive subsequent installations.
- Around line 937-941: Move manifest loading, JSON parsing, allowed_origins
validation and replacement, and the writeFileSync call into the existing try
block in the IPC handler. Validate that manifest.allowed_origins contains a
usable first entry before calling replace, and preserve the structured { ok:
false, error } response for read, parse, validation, and write failures.
---
Outside diff comments:
In `@browser-extension/native-host.js`:
- Around line 91-113: Update the CREDENTIAL_LEAK and OPEN_APP branches in
handleMessage to catch launchDesktopApp() rejections and send an ERROR response
containing the failure message and original request. Preserve the existing
success response types and ensure each branch completes without allowing the
rejection to escape to the process-level handler.
- Around line 80-89: Update the desktop process launch promise around
desktopProc.on('error') so spawn failures call reject with the launch error
instead of allowing the fixed timeout to resolve successfully. Add an exit or
close listener that clears desktopProc when the child terminates, while
preserving the existing launch-success resolution and top-of-function guard
behavior.
- Around line 115-123: Update readMessages() so the handleMessage() promise is
awaited or has a rejection handler that logs the failure and sends an ERROR
frame containing the error message, ensuring launchDesktopApp() failures receive
a response instead of only reaching the global unhandledRejection handler.
In `@src/main/ipcHandlers.js`:
- Around line 769-780: Validate and normalize payload.count in the
credential-leak:notify handler before constructing or persisting the alert:
accept only finite positive integers, and return an appropriate failure response
for invalid values. Use the normalized count consistently in the alert message,
detail, and metadata, including the db.addAlert call.
🪄 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: a42f6a0b-acad-4c42-bc42-0ab60733fd30
📒 Files selected for processing (10)
browser-extension/background.jsbrowser-extension/content.jsbrowser-extension/manifest.jsonbrowser-extension/native-host.jsbrowser-extension/popup.jssrc/i18n/locales/tr.jsonsrc/main/healthSummary.jssrc/main/ipcHandlers.jssrc/ui/js/pages/settings.jstools/install-native-host.js
💤 Files with no reviewable changes (2)
- src/i18n/locales/tr.json
- src/ui/js/pages/settings.js
🚧 Files skipped from review as they are similar to previous changes (3)
- browser-extension/manifest.json
- src/main/healthSummary.js
- browser-extension/content.js
| // Handle CHECK_PASSWORD from content script | ||
| chrome.runtime.onMessage.addListener((msg, sender, sendResponse) => { | ||
| if (msg.type === 'CHECK_PASSWORD' && msg.password) { | ||
| checkPassword(msg.password).then(sendResponse); | ||
| return true; // async response | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect relevant files with line numbers
printf '\n--- browser-extension/background.js ---\n'
cat -n browser-extension/background.js | sed -n '1,120p'
printf '\n--- Search for CHECK_PASSWORD and sendResponse usage ---\n'
rg -n "CHECK_PASSWORD|sendResponse|checkPassword\\(" browser-extension -SRepository: chrisriv10/Soterios
Length of output: 3570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect surrounding code paths that may catch/reject the promise
printf '\n--- browser-extension/content-script.js ---\n'
if [ -f browser-extension/content-script.js ]; then
cat -n browser-extension/content-script.js | sed -n '1,220p'
fi
printf '\n--- browser-extension/manifest.json ---\n'
if [ -f browser-extension/manifest.json ]; then
cat -n browser-extension/manifest.json | sed -n '1,220p'
fiRepository: chrisriv10/Soterios
Length of output: 1237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the exact background.js slice around the handler and password checker.
sed -n '1,120p' browser-extension/background.jsRepository: chrisriv10/Soterios
Length of output: 2300
Add a rejection handler to this message reply.
checkPassword() can still reject before its internal fetch catch runs (for example, crypto.subtle.digest failures), and checkPassword(...).then(sendResponse) will drop the response in that case. Chain a .catch(...) so the content script always gets a reply.
🤖 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/background.js` around lines 5 - 11, Update the
CHECK_PASSWORD handler’s checkPassword(msg.password) promise chain to catch all
rejections, including failures before the internal fetch handling, and call
sendResponse with an appropriate error response so the content script always
receives a reply while preserving the async return behavior.
| async function checkPassword(password) { | ||
| const HIBP_API = 'https://api.pwnedpasswords.com/range/'; | ||
| const encoder = new TextEncoder(); | ||
| const data = encoder.encode(password); | ||
| const hashBuffer = await crypto.subtle.digest('SHA-1', data); | ||
| const hash = Array.from(new Uint8Array(hashBuffer)) | ||
| .map(b => b.toString(16).padStart(2, '0')) | ||
| .join('') | ||
| .toUpperCase(); | ||
|
|
||
| const prefix = hash.slice(0, 5); | ||
| const suffix = hash.slice(5); | ||
|
|
||
| try { | ||
| const resp = await fetch(`${HIBP_API}${prefix}`); | ||
| const text = await resp.text(); | ||
| const lines = text.trim().split('\n'); | ||
|
|
||
| for (const line of lines) { | ||
| const [suf, count] = line.split(':'); | ||
| if (suf === suffix) { | ||
| return { pwned: true, count: parseInt(count, 10) }; | ||
| } | ||
| } | ||
| return { pwned: false, count: 0 }; | ||
| } catch (e) { | ||
| console.error('[Soterios] HIBP check failed:', e); | ||
| return { error: e.message }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and surrounding handlers
ast-grep outline browser-extension/background.js --view expanded || true
printf '\n--- file stats ---\n'
wc -l browser-extension/background.js
printf '\n--- relevant lines ---\n'
sed -n '1,220p' browser-extension/background.js
printf '\n--- search for CHECK_PASSWORD / timeout / AbortController ---\n'
rg -n "CHECK_PASSWORD|AbortController|setTimeout|fetch\\(" browser-extension -SRepository: chrisriv10/Soterios
Length of output: 3749
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how the response path behaves if checkPassword throws/returns error
rg -n "checkPassword\\(|chrome\\.runtime\\.onMessage|sendResponse|return true|CHECK_PASSWORD" browser-extension/background.js -n -SRepository: chrisriv10/Soterios
Length of output: 468
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the message handler and response lifecycle around checkPassword
cat -n browser-extension/background.js | sed -n '1,220p'Repository: chrisriv10/Soterios
Length of output: 2811
Add a timeout to the HIBP request. If api.pwnedpasswords.com stalls, checkPassword() never resolves and the CHECK_PASSWORD caller waits indefinitely.
🤖 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/background.js` around lines 34 - 63, Add an abort timeout
to the fetch request in checkPassword so stalled HIBP calls terminate and the
existing catch path returns an error instead of leaving CHECK_PASSWORD
unresolved. Create and pass an AbortSignal to fetch, ensure the timer is cleaned
up after completion, and preserve the current response parsing and error
handling behavior.
| 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); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and relevant symbols first.
git ls-files browser-extension/native-host.js
printf '\n--- outline ---\n'
ast-grep outline browser-extension/native-host.js --view expanded || true
printf '\n--- numbered excerpt ---\n'
cat -n browser-extension/native-host.js | sed -n '1,180p'Repository: chrisriv10/Soterios
Length of output: 4612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any other desktop launch or spawn patterns in the extension.
rg -n "spawn\\(|DESKTOP_APP|launchDesktopApp|desktopProc|unhandledRejection|uncaughtException" browser-extensionRepository: chrisriv10/Soterios
Length of output: 1226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the reviewed launch pattern is a direct executable spawn or uses cmd/start elsewhere.
python3 - <<'PY'
from pathlib import Path
p = Path('browser-extension/native-host.js')
text = p.read_text()
start = text.index('function launchDesktopApp')
print(text[start:start+1200])
PYRepository: chrisriv10/Soterios
Length of output: 1358
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read just the surrounding lines for the launch and message/error handlers.
sed -n '55,130p' browser-extension/native-host.js | nl -ba -v55Repository: chrisriv10/Soterios
Length of output: 197
🌐 Web query:
Node.js child_process.spawn Windows cmd.exe /c start quoting semantics detached shell:false argv first argument
💡 Result:
When using child_process.spawn to invoke 'cmd.exe' with '/c' 'start' on Windows with shell: false, you are responsible for manually handling argument quoting to satisfy the command-line parsing rules of cmd.exe [1][2]. Because 'start' is a shell builtin, it interprets its first quoted argument as a window title if it contains spaces [1]. To robustly use 'start' in this context: 1. Pass an empty string '""' as the first argument to 'start'. This serves as a dummy window title, ensuring that any subsequent arguments (like file paths with spaces) are not misinterpreted as the window title [1]. 2. If your arguments contain spaces or special characters, you must ensure they are properly quoted for cmd.exe [2][3]. 3. The option windowsVerbatimArguments: true can be used if you want to bypass Node.js's internal automatic quoting/escaping and pass the argument string exactly as-is to the Windows process [2][3]. This is often useful when you are already manually managing all required quotes for cmd.exe [2]. Example implementation: const { spawn } = require('node:child_process'); // The '""' is an empty window title, which prevents 'start' // from consuming the following path as the title. const child = spawn('cmd.exe', ['/c', 'start', '""', 'path to file.txt'], { windowsVerbatimArguments: true // Optional: use if you are manually handling all quotes }); Be aware that shell: true automatically handles some of these complexities by invoking the shell, but it introduces security risks if the input is not sanitized, as it may allow for arbitrary command injection [4]. Always prioritize explicitly passing arguments as an array rather than a single string when possible [3][4].
Citations:
- 1: https://stackoverflow.com/questions/27518718/node-js-executing-command-line-opening-a-file
- 2: Quotes are not handled correctly when child_process.spawn() parses args nodejs/node#5060
- 3: https://nodejs.org/docs/latest-v24.x/api/child_process.html
- 4: https://nodejs.org/api/child_process.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any code that reads launch-time argv or references the desktop app path as an argument.
rg -n "process\.argv|argv\\[1\\]|argv\\[2\\]|DESKTOP_APP|SOTERIOS_APP_PATH|launchDesktopApp|OPEN_APP|CREDENTIAL_LEAK" .Repository: chrisriv10/Soterios
Length of output: 1674
Spawn the desktop app directly. spawn(resolvedPath, [resolvedPath], ...) passes the executable path as the app’s first argument on non-Windows, and the cmd /c start branch still routes the launch through cmd.exe’s parser. spawn(resolvedPath, [], { shell: false, detached: true, windowsHide: true }) avoids both issues.
🧰 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 - 79, Update the desktop
launch logic around the spawn call to invoke resolvedPath directly with no
arguments, removing the platform-specific cmd/start command construction.
Preserve detached execution with shell disabled and add windowsHide: true to the
spawn options.
| 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)]; | ||
| // Write updated manifest back to disk so Chrome/Edge reads the correct ID | ||
| fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include manifest preparation in the structured error boundary.
JSON.parse, allowed_origins[0].replace(...), and writeFileSync run before the try block. Malformed or read-only manifests therefore reject the IPC call instead of returning { ok: false, error }; validate the manifest and move these operations inside the same try.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 940-940: 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/ipcHandlers.js` around lines 937 - 941, Move manifest loading, JSON
parsing, allowed_origins validation and replacement, and the writeFileSync call
into the existing try block in the IPC handler. Validate that
manifest.allowed_origins contains a usable first entry before calling replace,
and preserve the structured { ok: false, error } response for read, parse,
validation, and write failures.
| // Write updated manifest back to disk so Chrome/Edge reads the correct ID | ||
| fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Generate native-host manifests idempotently instead of persisting one-time substitutions.
Both installers mutate the template in place, so subsequent installations can retain a stale or placeholder extension origin.
src/main/ipcHandlers.js#L940-L941: validateSOTERIOS_EXT_IDand generate the persisted manifest from an immutable template or replace the current origin every run.tools/install-native-host.js#L28-L29: apply the same idempotent generation logic before registering or copying the manifest.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 940-940: 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)
📍 Affects 2 files
src/main/ipcHandlers.js#L940-L941(this comment)tools/install-native-host.js#L28-L29
🤖 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/ipcHandlers.js` around lines 940 - 941, Update native-host manifest
generation in src/main/ipcHandlers.js lines 940-941 and
tools/install-native-host.js lines 28-29 to be idempotent: validate
SOTERIOS_EXT_ID, load an immutable template or replace the extension origin on
every run, then persist/register/copy the freshly generated manifest. Apply the
same logic in both installers so stale or placeholder origins cannot survive
subsequent installations.
…factoring conflicts
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Superseded by #96 - clean rebase on main with security fixes and unit tests. |
2 participants
Summary
Complete browser extension ecosystem for credential leak monitoring, major tray dashboard enhancements, and full i18n coverage for the health score system across 14 locales.
Browser Extension (Manifest V3)
browser-extension/— Complete browser extension including:background.js)content.js)popup.html,popup.js)options.html,options.js)native-host.js)tools/install-native-host.js)Desktop Integration
soterios://protocol handlercredential-leak:notifybrowserExtension:installNativeHostTray Dashboard Enhancements
System Health Score i18n (14 Locales)
Translation Key Structure
Updated to use:
health.label.*health.reason.*This matches the dashboard's expected translation structure.
Added
health.reason.*KeysnoScannoThreatsthreatsFoundscanTodayscanDaysAgodiskLowSpacediskNoVolumesdiskHealthymemoryUsagecpuLoaduptimeTodayuptimeDaysuptimeWeeksuptimeLongrtpActivertpDisabledfirewallActivefirewallDisabledLocalization
health.reason.noThreats"No se encontraron amenazas en el escaneo más reciente"Completed translations for all 14 locales:
en)es)fr)de)it)tr)ru)pt-BR)ko)ja)zh-CN)nl)pl)ar)Testing
Files Changed
Summary by CodeRabbit
New Features
Improvements