Feature/emergency lockdown - #92
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'
- 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
- Custom NSIS installer (installer.nsi) with modern UI - Custom welcome/finish banner images (welcome.bmp, welcome-banner.bmp, finish-banner.bmp) - Custom NSH include file with modern styling - Updated package.json to use custom installer script - Banner images generated from Soterios branding
…ct with electron-builder
- Remove MUI_WELCOMEFINISHPAGE_BITMAP, MUI_UNWELCOMEFINISHPAGE_BITMAP, MUI_ICON, MUI_UNICON from nsh (electron-builder defines these) - Remove MUI_WELCOMEPAGE_SHOW_LICENSE from nsh (electron-builder defines) - Remove MUI_ICON, MUI_UNICON from nsh (electron-builder defines) - Keep only non-conflicting custom definitions (colors, text, custom pages) - Update installer.nsi to properly include nsh without conflicts
…Font instead; remove conflicting MUI definitions from nsh
- Update SVG icon for emergency lockdown in sidebar (shield with lock) - Add click handler in router.js to show alert when clicking lockdown nav item - Alert shows emergency lockdown icon and description
…settings - Remove sidebar click alert from router.js - Add emergencyLockdown feature toggle in Settings page - Add i18n strings for emergency lockdown feature
- Correct browser-extension/package.json script paths to reference root-level tooling (../tools/) instead of invalid local paths, and replace the ad-hoc zip packaging command with the dedicated package-extension.js tool for reliable, cross-platform extension builds. - Remove stray duplicate English translation entries from pt-BR and ru locale files that were incorrectly present alongside localized translations, cleaning up i18n data.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a Chrome extension with HIBP password checks and native messaging, introduces Windows emergency lockdown controls with allowlists, expands tray health reporting, adds localized UI strings, and configures branded Windows installation and packaging workflows. ChangesBrowser extension and native messaging
Emergency lockdown
Tray health dashboard
Installer and runtime packaging
Estimated code review effort: 5 (Critical) | ~120 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}
PR Summary by QodoEmergency Lockdown, Browser Extension Integration, and Tray Health Dashboard
AI Description
Diagram
High-Level Assessment
Files changed (56)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (17)
build/installer.nsi-111-120 (1)
111-120: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist
InstallLocationfor future upgrades.Line 15 reads
InstallLocationto restore a prior custom directory, but this install section never writes that value. A later upgrade can default to Program Files and leave the previous installation behind.Proposed fix
WriteRegStr HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Soterios" "DisplayVersion" "${PRODUCT_VERSION}" + WriteRegStr HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Soterios" "InstallLocation" "$INSTDIR" WriteRegStr HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Soterios" "Publisher" "Christopher Rivera"🤖 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 `@build/installer.nsi` around lines 111 - 120, Update the installer registry-writing section alongside the existing uninstall values to persist the installation directory under the Soterios uninstall key. Write the current $INSTDIR as InstallLocation so the upgrade logic can restore a prior custom directory.build/convert-welcome.ps1-7-9 (1)
7-9: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRead the real SVG and rasterize it with an SVG-capable renderer
build/icon.svgisn’t present here, so this step fails immediately; even if it did exist,System.Drawing.Image.FromStream()can’t turn SVG text into a bitmap. Point it at the actual source SVG and use an SVG renderer before savingwelcome-banner.bmp.🤖 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 `@build/convert-welcome.ps1` around lines 7 - 9, Update the image conversion flow around $img and $bmp to read the actual SVG source instead of relying on the missing build/icon.svg, then rasterize it with an SVG-capable renderer before creating the 500×120 bitmap. Preserve the existing output path and BMP save format.browser-extension/background.js-1-3 (1)
1-3: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
onInstalledsilently re-enablesexternalLookupsEnabledon every update.
chrome.runtime.onInstalledfires oninstall,update, andchrome_update. This handler unconditionally forcesexternalLookupsEnabled: trueevery time, which will silently override a user's prior opt-out of external HIBP lookups whenever the extension auto-updates.🛡️ Proposed fix: only set default on first install
-chrome.runtime.onInstalled.addListener(() => { - chrome.storage.sync.set({ externalLookupsEnabled: true }); -}); +chrome.runtime.onInstalled.addListener((details) => { + if (details.reason === 'install') { + chrome.storage.sync.set({ externalLookupsEnabled: 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/background.js` around lines 1 - 3, Update the chrome.runtime.onInstalled handler to set externalLookupsEnabled only when the install reason is "install"; preserve existing user preferences for update and chrome_update events.src/security/EmergencyLockdown.js-153-207 (1)
153-207: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTOCTOU:
isLockdownguard is checked before, but only set after, the async lockdown work completes.
lockdown()checksthis.isLockedDownat entry (line 154) but only sets ittrueat line 194, after capturing state and disabling interfaces/stopping services. Two concurrent invocations (e.g. a double-clicked "Lockdown" button, or two rapid IPC calls) would both pass the guard, both capture/overwritesavedNetworkState/savedServicesState, and both attempt to disable/stop the same resources - corrupting the saved state thatrestore()later relies on to correctly reset the system.🔒 Suggested fix: set the guard before starting async work
async lockdown() { if (this.isLockedDown) { return { success: false, message: 'Already in lockdown mode' }; } + this.isLockedDown = true; try { // Save current state const interfaces = await this.getNetworkInterfaces(); const services = await this.getNonEssentialServices(); ... - this.isLockedDown = true; this.eventBus.emit('lockdown:changed', { locked: true, results }); ... return { success: true, results }; } catch (err) { + this.isLockedDown = false; throw new Error(`Lockdown failed: ${err.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 `@src/security/EmergencyLockdown.js` around lines 153 - 207, Update lockdown() to claim the lockdown state immediately after the existing isLockedDown guard and before any awaited state capture or resource changes, preventing concurrent invocations from proceeding. Preserve the already-locked response, and ensure the guard is reset if the lockdown operation fails so restore() does not receive corrupted or incomplete state.browser-extension/content.js-102-140 (1)
102-140: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
showIconandautoChecksettings.Initialization always injects icons, and disabling
showIcononly removes existing icons—the activeMutationObserveradds icons to later fields. Also, no input listener implements the advertisedautoCheckbehavior.🤖 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 102 - 140, Update init, the MutationObserver callback, and the SETTINGS_UPDATED handler to honor settings.showIcon before injecting or retaining icons, including fields added after initialization. Add the input handling used by addIconToField or its associated setup so settings.autoCheck controls whether password values trigger the advertised automatic check, and apply updated settings consistently to existing and future fields.src/ui/js/pages/lockdown.js-162-185 (1)
162-185: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle unsuccessful status responses.
When
getStatus()returns{ ok: false }, this method neither displays the error nor changes the initial disabled controls. The page remains stuck on “Checking” until navigation/reload. Surfaceresult.errorand provide a retry path while keeping unsafe actions disabled.🤖 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/lockdown.js` around lines 162 - 185, Update the status handling in the getStatus flow to add an explicit result.ok false branch that displays result.error, replaces the checking state with the lockdown error state, and provides a retry path. Keep unsafe controls disabled in this branch, while preserving the existing success handling and catch behavior.browser-extension/native-host.js-12-12 (1)
12-12: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse one provisioned desktop-app path contract.
DESKTOP_APPis initialized fromSOTERIOS_APP_PATHbut never used; launching instead requiresDESKTOP_APP. Sincenative-host.batsets neither variable,CREDENTIAL_LEAKandOPEN_APPreject with “DESKTOP_APP environment variable not set,” and the async handler sends no response.Make the launcher and host use one variable, and have installation provide a concrete executable path.
Also applies to: 63-66
🤖 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` at line 12, Unify the desktop application path contract by having the launcher and native host use the same variable, centered on DESKTOP_APP rather than the unused SOTERIOS_APP_PATH initialization. Update native-host.bat installation to provision DESKTOP_APP with the concrete executable path, and ensure CREDENTIAL_LEAK and OPEN_APP resolve that value so their async handlers return responses instead of rejecting when launched by the host.browser-extension/native-host-manifest.json-6-8 (1)
6-8: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInject the real extension ID before writing the native host manifest
browser-extension/native-host-manifest.jsonstill ships with<EXTENSION_ID>, and both install paths fall back toYOUR_EXTENSION_ID_HERE. No packaging step injects the packaged extension ID, so native messaging stays broken unless the env var is set manually. Wire the real ID intotools/install-native-host.js/src/main/ipcHandlers.jsbefore writing the manifest.🤖 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-manifest.json` around lines 6 - 8, Update tools/install-native-host.js and the relevant manifest-writing flow in src/main/ipcHandlers.js to resolve the packaged extension ID and replace the allowed_origins placeholder before writing browser-extension/native-host-manifest.json. Remove the YOUR_EXTENSION_ID_HERE fallback so both installation paths always write the actual extension ID and never persist <EXTENSION_ID>.browser-extension/native-host.bat-5-6 (1)
5-6: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBundle or reference a Node runtime for the native host
native-host.batstill callsnodedirectly, and the Windows installer only copiesdist\win-unpacked\*—it doesn’t provisionnode.exe. Unless Node is guaranteed on the user’s PATH, the registered browser host won’t start.🤖 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` around lines 5 - 6, Update native-host.bat and the Windows packaging/installer flow so the registered native host uses a bundled or explicitly provisioned Node runtime instead of relying on node being available on PATH. Ensure the runtime is included alongside the packaged native-host.js and that the batch script resolves and invokes that runtime reliably from its own directory.src/i18n/locales/ja.json-785-785 (1)
785-785: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the duplicate
health.malware.highkey.Biome reports this key as already declared. Keeping both definitions can silently apply last-write-wins behavior and fails the duplicate-key lint rule.
🤖 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 785, Remove the duplicate health.malware.high entry from the Japanese locale, keeping only the existing declaration and preserving the intended translation.Source: Linters/SAST tools
src/i18n/locales/ko.json-824-836 (1)
824-836: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the duplicate health-dimension keys.
Biome reports these keys as already declared. Keep one canonical definition for each key; the current duplicates can fail lint and make the loaded value depend on declaration order.
🤖 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/ko.json` around lines 824 - 836, Remove the duplicate health-dimension entries from the locale object, retaining exactly one canonical definition for each affected key such as health.disk, health.memory, health.load, health.uptime, health.rtp, and health.firewall. Preserve the existing translation values and object structure for the retained definitions.Source: Linters/SAST tools
browser-extension/popup.html-42-46 (1)
42-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGive the password field an accessible name.
“Check a Password” is a
<div>, not an associated<label>, so assistive technology may not identify the field reliably.Proposed fix
- <div class="label">Check a Password</div> + <label class="label" for="passwordInput">Check a Password</label>🤖 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.html` around lines 42 - 46, Associate the password input in the “Check a Password” section with a proper accessible name by replacing the non-label heading or adding an explicit label tied to passwordInput. Preserve the existing visual text and input behavior while ensuring assistive technologies can identify the field reliably.src/ui/pages/trayDashboard.html-103-129 (1)
103-129: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftNewly added
tray.*i18n keys are not used here — tray dashboard text is hardcoded in English.
en.json(same PR) addstray.systemHealth,tray.rtpActive,tray.rtpOff,tray.network,tray.quickScan,tray.openApp,tray.quit,tray.lastScanAgo, andtray.networkRxTx, but this markup/script hardcodes the equivalent English strings directly ("System Health", "RTP", "Quick Scan", "Open Soterios", "Quit", "Last scan {ago}"). Non-English users will always see English text in the tray, defeating the purpose of the added keys.Also applies to: 195-229
🤖 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 103 - 129, The tray dashboard markup and its associated script still hardcode user-facing English text instead of using the new tray.* localization keys. Update the visible labels and dynamic messages around the System Health, RTP status, Network, action buttons, last-scan text, and network RX/TX display to resolve through the existing i18n mechanism using tray.systemHealth, tray.rtpActive, tray.rtpOff, tray.network, tray.quickScan, tray.openApp, tray.quit, tray.lastScanAgo, and tray.networkRxTx.src/main/ipcHandlers.js-923-953 (1)
923-953: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNative host install is not idempotent — extension ID can never be updated after the first run, and other allowed_origins entries are silently dropped.
The handler reads
native-host-manifest.json, replaces<EXTENSION_ID>inallowed_origins[0], then writes the result back over the same file. On any subsequent run (e.g.SOTERIOS_EXT_IDchanges, or a re-install), the placeholder is already gone from the on-disk file, so.replace('<EXTENSION_ID>', extId)is a no-op and silently keeps the stale ID. Also,manifest.allowed_origins = [manifest.allowed_origins[0].replace(...)]unconditionally collapses the array to one element, dropping any other pre-existing origins.♻️ Suggested fix — don't mutate the shipped template
- 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)); + const template = JSON.parse(fs.readFileSync(manifestPath, 'utf8')); + const extId = process.env.SOTERIOS_EXT_ID || 'YOUR_EXTENSION_ID_HERE'; + const manifest = { + ...template, + allowed_origins: template.allowed_origins.map((o) => o.replace('<EXTENSION_ID>', extId)) + }; + // Write the resolved manifest to a stable, always-regenerated location (not the shipped template) + fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2));Keeping the template file untouched (or always re-reading a pristine copy) is required for re-installs/ID changes to work correctly.
🤖 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 923 - 953, Update the browserExtension:installNativeHost handler to preserve the shipped native-host-manifest.json template and make reinstalls use the current SOTERIOS_EXT_ID. Build a separate manifest object or output file for the registered host, replace the placeholder in every existing allowed_origins entry without collapsing the array, and register that generated manifest path while leaving the source template unchanged.src/main/ipcHandlers.js-927-950 (1)
927-950: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse
execFileSyncwith an argument array instead ofexecSyncwith a concatenated command string.Both ast-grep and OpenGrep flag this pattern.
manifest.nameandmanifestPathare interpolated directly into a shell string passed toexecSync; ifmanifest.nameever contains a"or other shell metacharacter, the quoting breaks and additional commands could be injected. ThemanifestPath.replace(/\\/g, '\\\\')escaping is also unnecessary forcmd.exe/reg.exe, which don't treat\as an escape character.🔒️ Suggested fix
- const regPath = `HKCU\\Software\\Google\\Chrome\\NativeMessagingHosts\\${manifest.name}`; - 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 (_) {} + const { execFileSync } = require('child_process'); + const args = (regPath) => ['add', regPath, '/ve', '/t', 'REG_SZ', '/d', manifestPath, '/f']; + execFileSync('reg', args(`HKCU\\Software\\Google\\Chrome\\NativeMessagingHosts\\${manifest.name}`), { stdio: 'ignore' }); + try { execFileSync('reg', args(`HKCU\\Software\\Microsoft\\Edge\\NativeMessagingHosts\\${manifest.name}`), { stdio: 'ignore' }); } catch (_) {} return { 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 `@src/main/ipcHandlers.js` around lines 927 - 950, Replace the vulnerable execSync command-string construction in the native-host registration flow with execFileSync calls that pass reg.exe arguments as arrays. Update both the Chrome registration using regPath and manifestPath, and the Edge registration using regPathEdge, while preserving the existing success and failure handling. Remove the manual backslash escaping and avoid interpolating manifest.name or manifestPath into a shell command.src/main/healthSummary.js-43-55 (1)
43-55: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
db.getNetworkStatsHistory(24)here, and readrx_sec/tx_secinstead ofrx_bytes/tx_bytes.db.getNetworkHistorydoesn’t exist, and these rows use per-second stats, so the tray currently falls back to empty data or renders zeros/NaN.🤖 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 43 - 55, Update the network history logic in the health summary to call db.getNetworkStatsHistory(24) instead of db.getNetworkHistory(24 * 60), and use each row’s rx_sec and tx_sec fields for latest values and sparkline/history mappings. Preserve the existing empty-data defaults and KB conversion while preventing undefined-field zeros or NaN results.src/main/healthSummary.js-22-30 (1)
22-30: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the live RTP status and the existing network history API.
src/main/healthSummary.js:24-30derivesrtp.enabledfromfeature.realtimeProtection, so the tray can show protection as active even whenrealtimeWatcher.getStatus()would report otherwise. PassrealtimeWatcherintogetTrayHealthSummaryand use the live status here.
src/main/healthSummary.js:45callsdb.getNetworkHistory, but the DB only exposesgetNetworkStatsHistory(hours, iface). That path falls back to[], so the tray network history stays empty; use the existing API with hours instead of minutes.🤖 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 22 - 30, The getTrayHealthSummary flow should use the live RealTimeWatcher status instead of the feature setting, so pass realtimeWatcher into getTrayHealthSummary and derive rtp.enabled from realtimeWatcher.getStatus(). Replace the unsupported db.getNetworkHistory call with the existing db.getNetworkStatsHistory API, passing the requested duration in hours and preserving the network history result.
🟡 Minor comments (15)
src/main/main.js-598-602 (1)
598-602: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle development protocol registration separately.
src/main/main.js:598-602—app.setAsDefaultProtocolClient('soterios')is the packaged-app form. Inprocess.defaultAppmode, passprocess.execPathand the entry script instead, or deep links won’t register correctly during development.🤖 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/main.js` around lines 598 - 602, Update the protocol registration inside the app.whenReady callback to handle process.defaultApp separately: in development, call app.setAsDefaultProtocolClient with process.execPath and the entry script arguments; retain the existing packaged-app registration for non-development runs.Source: MCP tools
src/main/main.js-584-586 (1)
584-586: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not force-exit after
app.quit()
app.quit()already follows the normal shutdown path and triggersbefore-quitcleanup;process.exit(0)can skip that cleanup and leave resources unflushed.Proposed fix
if (!gotTheLock) { app.quit(); - process.exit(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 `@src/main/main.js` around lines 584 - 586, Remove the process.exit(0) call from the !gotTheLock branch in the application startup flow, leaving app.quit() as the sole shutdown action so normal before-quit cleanup can complete.Source: MCP tools
build/installer.nsi-75-77 (1)
75-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBundle page artwork independently of
$INSTDIR. Installer pages execute before—or independently of—the application-file installation, while neither script copies these BMPs to$INSTDIR\build.
build/installer.nsi#L75-L77: extractfinish-banner.bmpto$PLUGINSDIRduring installer initialization and load it from that location.build/installer.nsh#L91-L93: extractwelcome-banner.bmpbefore the custom welcome page and load it from$PLUGINSDIR.🤖 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 `@build/installer.nsi` around lines 75 - 77, Bundle both page artwork files independently of $INSTDIR: in build/installer.nsi lines 75-77, extract finish-banner.bmp to $PLUGINSDIR during installer initialization and load it from there; in build/installer.nsh lines 91-93, extract welcome-banner.bmp to $PLUGINSDIR before the custom welcome page and load it from there.src/i18n/locales/ar.json-202-225 (1)
202-225: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the new lockdown strings into Arabic.
nav.lockdownand everylockdown.*value added here are English, unlike the surrounding Arabic catalog. Arabic users will see the emergency workflow untranslated.🤖 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/ar.json` around lines 202 - 225, Translate nav.lockdown and every lockdown.* value in the locale catalog into natural Arabic, preserving each existing key and its emergency-lockdown meaning. Replace only the English values; keep the surrounding Arabic translations and JSON structure intact.browser-extension/content.js-94-99 (1)
94-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClean up field state when removing the icon.
After blur, the icon is removed but
passwordFieldsanddata-soterios-idremain. Future scans skip that field permanently, so its icon never returns; scroll/resize listeners also remain attached.🤖 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, Update the blur cleanup associated with updatePos and passwordFields so removing the icon also removes the input from passwordFields, removes its data-soterios-id attribute, and detaches the scroll and resize listeners. Preserve the existing delayed icon removal and one-time blur behavior.src/ui/pages/shell.html-106-113 (1)
106-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate lockdown behind
features.emergencyLockdown
The nav item is always shown, and the service/IPC path is always registered, so the settings toggle only changes the settings UI. Hide the entry and skip creating/registering the lockdown service when the feature is off.🤖 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/shell.html` around lines 106 - 113, Gate the lockdown navigation item and its associated service/IPC registration on features.emergencyLockdown. Update the shell markup around the data-page="lockdown" nav item to hide it when disabled, and conditionally skip the lockdown service creation and registration while preserving existing behavior when enabled.src/i18n/locales/de.json-904-905 (1)
904-905: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate
scanIndicator.threatsFound.This German locale value is still
"${count} threat(s) found", causing the scan indicator to switch back to English for this outcome.🤖 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` around lines 904 - 905, Translate the German locale value for scanIndicator.threatsFound, preserving the {count} interpolation placeholder and expressing the threat-found message in German so this scan outcome does not fall back to English.src/i18n/locales/ja.json-886-886 (1)
886-886: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the missing Japanese time unit.
"過去 1 以内"omits日, producing incomplete text. Use wording equivalent to"過去 1 日以内に...".🤖 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 886, Update the Japanese translation value for “health.reason.scanToday” to include the missing 日 time unit, producing wording equivalent to “過去 1 日以内に実行されました。”src/i18n/locales/de.json-202-225 (1)
202-225: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocalize the emergency-lockdown strings in every non-English locale.
The same English
nav.lockdownandlockdown.*block was added to all seven locale files, so users selecting these languages will see the entire lockdown flow in English.
src/i18n/locales/de.json#L202-L225: Translatenav.lockdownand everylockdown.*value into German.src/i18n/locales/es.json#L202-L225: Translatenav.lockdownand everylockdown.*value into Spanish.src/i18n/locales/fr.json#L202-L225: Translatenav.lockdownand everylockdown.*value into French.src/i18n/locales/hi.json#L202-L225: Translatenav.lockdownand everylockdown.*value into Hindi.src/i18n/locales/it.json#L202-L225: Translatenav.lockdownand everylockdown.*value into Italian.src/i18n/locales/ja.json#L202-L225: Translatenav.lockdownand everylockdown.*value into Japanese.src/i18n/locales/ko.json#L202-L225: Translatenav.lockdownand everylockdown.*value into Korean.🤖 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` around lines 202 - 225, Translate nav.lockdown and every lockdown.* value into the target language while preserving all keys and JSON structure. Apply the localized translations in src/i18n/locales/de.json lines 202-225 (German), src/i18n/locales/es.json lines 202-225 (Spanish), src/i18n/locales/fr.json lines 202-225 (French), src/i18n/locales/hi.json lines 202-225 (Hindi), src/i18n/locales/it.json lines 202-225 (Italian), src/i18n/locales/ja.json lines 202-225 (Japanese), and src/i18n/locales/ko.json lines 202-225 (Korean).src/i18n/locales/it.json-810-824 (1)
810-824: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the Italian health-message grammar and pluralization.
These strings contain output such as “nell'ultimo scansione”, “Recency scansione”, and singular
{days} giornoforms. Use consistentscansionegender and plural-aware wording for interpolated counts.Also applies to: 906-910
🤖 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 810 - 824, Correct the Italian grammar in the health malware and scan-recency translation keys, including the related entries around the additional referenced section. Use feminine scansione forms such as “ultima scansione” and natural wording for scan recency, and make the daysAgo message plural-aware so one day and multiple days are grammatically correct while preserving the existing placeholders.browser-extension/options.js-23-28 (1)
23-28: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the save UI on storage write errors
chrome.storage.sync.setcan fail, but this callback still shows the saved message and sendsSETTINGS_UPDATED. Checkchrome.runtime.lastErrorfirst and only continue on success.🤖 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.js` around lines 23 - 28, Update the chrome.storage.sync.set callback to check chrome.runtime.lastError before updating the savedMsg element or sending the SETTINGS_UPDATED message; return immediately on error and preserve the existing success UI and notification flow.src/i18n/locales/nl.json-907-907 (1)
907-907: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTypo: "Wenig" should be "Weinig".
"Wenig" is German for "little"; the Dutch equivalent is "Weinig".
health.reason.diskLowSpacecurrently reads"Wenig ruimte op: {volumes} ({pct}% in gebruik).".🤖 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 907, Update the Dutch translation value for health.reason.diskLowSpace, replacing the incorrect “Wenig” wording with “Weinig” while preserving the existing placeholders and punctuation.src/i18n/locales/nl.json-207-225 (1)
207-225: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAll
lockdown.*strings are untranslated English placeholders.Every value in this block (title, description, status labels, confirmations, warning, etc.) is verbatim English, while the neighboring
nav.tools/nav.reports/nav.settings/nav.scanningkeys in the same diff are properly localized to Dutch. Dutch users will see the new Emergency Lockdown page entirely in English.🤖 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` around lines 207 - 225, Translate every value in the lockdown.* entries to Dutch, including the title, descriptions, status labels, actions, confirmations, progress messages, and warning, while preserving the existing keys and interpolation-free structure.src/main/ipcHandlers.js-769-784 (1)
769-784: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnvalidated
payload.countproduces broken alert text.Only
payload.passwordis checked; ifpayload.countis missing/non-numeric, the message becomes"Password found in undefined breach via browser extension"and the detail string embedsundefinedtoo.🐛 Proposed fix
ipcMain.handle('credential-leak:notify', async (_event, payload) => { if (!payload?.password) return { ok: false, error: 'Missing password' }; + const count = Number(payload.count) || 0; 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 } };🤖 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 - 784, Validate payload.count in the credential-leak:notify handler before constructing the alert, requiring a numeric breach count and returning the existing failure shape for invalid input. Use the validated count consistently in the message, detail, and metadata so no undefined or non-numeric values reach db.addAlert or the alert:new event.src/i18n/locales/tr.json-873-875 (1)
873-875: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
{pct}in the Turkish disk reasons
src/ui/js/pages/dashboard.jspasses{ pct: ... }for bothhealth.reason.diskLowSpaceandhealth.reason.diskHealthy, so{usage}will render literally intr.json. Replace it with{pct}.🤖 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 873 - 875, Update the Turkish translations for health.reason.diskLowSpace and health.reason.diskHealthy to use the {pct} placeholder instead of {usage}, matching the values passed by the dashboard.
🧹 Nitpick comments (7)
browser-extension/background.js (2)
1-1: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicated HIBP lookup logic in both files is missing an HTTP status check. Neither
checkPassword(background.js) norcheckPwned(popup.js) checksresp.okbefore parsing the HIBP range response; a failed/rate-limited request falls through to reporting the password as "not pwned" instead of surfacing an error.
browser-extension/background.js#L47-58: add aif (!resp.ok) throw new Error(...)(or return{ error }) before parsingtext.browser-extension/popup.js#L9-21: add the sameresp.okcheck incheckPwnedand propagate/display the error instead of returning0.🤖 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` at line 1, Update the HIBP response handling in checkPassword and checkPwned to validate resp.ok before parsing the range response; for unsuccessful or rate-limited requests, propagate an error through the existing result flow and display it instead of reporting the password as not pwned or returning 0.
47-58: 🎯 Functional Correctness | 🔵 TrivialMissing
resp.okcheck before parsing HIBP response.If the HIBP request fails or is rate-limited, the response body won't match the expected
hash:countformat, and the loop falls through toreturn { pwned: false, count: 0 }- silently reporting a leaked password as safe. See related note onpopup.js'scheckPwned, which has the identical gap.🤖 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 47 - 58, Check resp.ok immediately after the fetch in the HIBP request flow before reading or parsing the response body. Handle non-success responses explicitly rather than returning the safe result, and apply the same correction to popup.js’s checkPwned function.tools/install-native-host.js (2)
11-11: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilent fallback to placeholder
EXTENSION_ID.If
EXTENSION_IDisn't set, the script silently installs a native-messaging manifest withallowed_originspointing atYOUR_EXTENSION_ID_HERE, which will never match a real extension - failing without any warning to the operator.♻️ Suggested validation
const EXTENSION_ID = process.env.EXTENSION_ID || 'YOUR_EXTENSION_ID_HERE'; +if (EXTENSION_ID === 'YOUR_EXTENSION_ID_HERE') { + console.error('EXTENSION_ID environment variable not set. Aborting.'); + process.exit(1); +}🤖 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 EXTENSION_ID initialization in the install script to validate that process.env.EXTENSION_ID is provided and non-empty, and fail immediately with a clear operator-facing error instead of using YOUR_EXTENSION_ID_HERE as a fallback. Keep the existing manifest installation flow unchanged when a valid ID is supplied.
32-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer
execFileSyncwith an argument array for thereg addcalls.
manifest.name(read from a file on disk) is interpolated directly into a shell command string, flagged by OpenGrep as CWE-78. UsingexecFileSync('reg', ['add', regPath, '/ve', '/t', 'REG_SZ', '/d', manifestPath, '/f'])removes the shell-parsing surface entirely.🤖 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 32 - 49, The Chrome and Edge registration commands build shell strings with interpolated manifest data, creating command-injection risk. Update the registration blocks around regCmd and regCmdEdge to use execFileSync with the reg executable and an argument array containing the registry path, manifest path, and existing flags, while preserving their current success and failure handling.Source: Linters/SAST tools
browser-extension/popup.js (2)
9-21: 🎯 Functional Correctness | 🔵 TrivialMissing
resp.okcheck before parsing HIBP response.Same gap as
background.js'scheckPassword: a failed/rate-limited HIBP request silently resolves toreturn 0(not pwned) instead of surfacing an error, misleading the user into believing a leaked password is safe.🤖 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 9 - 21, Update checkPwned to validate resp.ok immediately after the HIBP fetch and before reading or parsing the response body; when the request fails, propagate an error instead of continuing to return 0, while preserving the existing matching and count parsing for successful responses.
57-80: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider clearing the password field after checking.
input.value(plaintext password) stays in the DOM after the check completes. Clearing it after use is a small privacy-hygiene improvement.🤖 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 57 - 80, Clear the plaintext password from the input after the check completes by resetting input.value in the finally block of the checkBtn click handler, ensuring it runs on both success and error while preserving the existing button and loader cleanup.src/ui/pages/trayDashboard.js (1)
1-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate render logic between the
tray:summarypush handler andloadSummary().Both blocks independently re-implement the same score/RTP/firewall/network/last-scan DOM updates (once event-driven, once via 15s polling in
loadSummary). Extracting a singlerender(summary)function used by both thetray:summarylistener andloadSummary()'s.then()would remove the duplication and prevent the two paths from silently drifting apart.🤖 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 1 - 104, The tray:summary listener and loadSummary duplicate summary DOM rendering logic. Extract the shared score, RTP, firewall, network, and last-scan updates into a single render(summary) function, then call it from both the tray:summary handler and loadSummary after retrieving the summary, preserving existing null and error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59b06034-9681-40de-bcf0-8cf5a09c1b52
⛔ Files ignored due to path filters (10)
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!**/*.pngbuild/finish-banner.bmpis excluded by!**/*.bmpbuild/finish-banner.svgis excluded by!**/*.svgbuild/welcome-banner.bmpis excluded by!**/*.bmpbuild/welcome-banner.svgis excluded by!**/*.svgbuild/welcome.bmpis excluded by!**/*.bmp
📒 Files selected for processing (46)
README.mdbrowser-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.jsbuild/convert-welcome.ps1build/installer.nshbuild/installer.nsipackage.jsonsrc/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/main/serviceRegistry.jssrc/preload/preload.jssrc/security/EmergencyLockdown.jssrc/ui/js/pages/lockdown.jssrc/ui/js/pages/settings.jssrc/ui/pages/shell.htmlsrc/ui/pages/trayDashboard.htmlsrc/ui/pages/trayDashboard.jstools/build-icons.jstools/install-native-host.jstools/package-extension.js
| Function un.onInit | ||
| UserInfo::GetAccountType | ||
| Pop $0 | ||
| StrCmp $0 "Admin" 0 +2 | ||
| StrCpy $IsAdmin 1 | ||
| FunctionEnd |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'File list around installer and NSIS-related files:\n'
git ls-files | rg '(^|/)(installer\.nsh|.*\.nsi|.*\.nsh|.*\.nsis)$' || true
printf '\nSearch for IsAdmin and Var declarations:\n'
rg -n --hidden --no-ignore-vcs '\bIsAdmin\b|\bVar\s+IsAdmin\b' .
printf '\nShow build/installer.nsh around the referenced lines:\n'
sed -n '180,250p' build/installer.nshRepository: chrisriv10/Soterios
Length of output: 3369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Search for IsAdmin declarations/usages:\n'
rg -n --hidden --no-ignore-vcs '\bIsAdmin\b' .
printf '\nShow build/installer.nsh around line 218:\n'
nl -ba build/installer.nsh | sed -n '214,228p'
printf '\nShow any NSIS Var declarations in relevant installer files:\n'
rg -n --hidden --no-ignore-vcs '^\s*Var\s+' buildRepository: chrisriv10/Soterios
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- build/installer.nsh around the assignment ---'
nl -ba build/installer.nsh | sed -n '216,226p'
echo '--- IsAdmin search ---'
rg -n '\bIsAdmin\b' build . || true
echo '--- Var declarations in installer files ---'
rg -n '^\s*Var\s+' build/*.nsh build/*.nsi 2>/dev/null || trueRepository: chrisriv10/Soterios
Length of output: 247
🌐 Web query:
NSIS Var declaration undeclared user variable compile error $IsAdmin
💡 Result:
The "undeclared user variable" compile error in NSIS occurs because all user-defined variables must be explicitly declared using the Var command before they are referenced in your script [1][2][3]. Unlike the built-in registers (e.g., $0, $R0), which are always available, custom variables like $IsAdmin do not exist until they are created [1][3]. To resolve this error, you must declare the variable at the top level of your script (or within a function/section using the /GLOBAL flag) [4][1]. How to Fix: Add the following line to the top of your script (outside of any Section or Function): Var IsAdmin If you need to declare it inside a Section or Function, use the /GLOBAL flag: Function.onInit Var /GLOBAL IsAdmin #... rest of your code FunctionEnd Why this happens: 1. Requirement for Declaration: NSIS strictly requires user variables to be registered via the Var command so the compiler can allocate memory for them [1][2]. 2. Scope: All variables in NSIS are global once declared [4][1]. If you attempt to use $IsAdmin without this declaration, the compiler treats it as an invalid or undefined identifier, resulting in the error [5][2]. 3. Common Misconception: If you are using code snippets from other projects (like NsisMultiUser.nsh or examples using the UAC plugin), ensure you are copying the variable declarations along with the logic [6][7][8]. Many examples assume these variables have already been declared elsewhere in the project [6]. Recommendation: Check your script for the Var IsAdmin line. If you are using plugins like UAC or UserInfo to determine admin status, remember that these plugins often populate a temporary register (e.g., $0) with the result [9][10][8]. You must then manually copy that result into your declared $IsAdmin variable using StrCpy if you intend to store it for later use [6].
Citations:
- 1: https://documentation.help/NSIS/Section4.2.html
- 2: https://nsis.sourceforge.io/Docs/Chapter4.html
- 3: https://nsis.sourceforge.io/Docs/Chapter2.html
- 4: https://nsis.sourceforge.io/Reference/Var
- 5: https://stackoverflow.com/questions/51823457/nsis-despite-preproccsor-get-warning-unknown-variable-constant-test-detected
- 6: https://github.com/Drizin/NsisMultiUser/blob/master/Include/NsisMultiUser.nsh
- 7: https://github.com/qutebrowser/qutebrowser/blob/master/misc/nsis/install.nsh
- 8: https://git.chylex.com/chylex/Nextcloud-Desktop/src/tag/v1.5.0-beta1-2nd/admin/win/nsi/nsis_uac/UAC_RealWorldFullyLoadedDualModeExample.nsi
- 9: https://nsis-dev.github.io/NSIS-Forums/html/t-329810.html
- 10: https://searchfox.org/firefox-main/source/browser/installer/windows/nsis/installer.nsi
🌐 Web query:
NSIS documentation Var declaration user variables compile error undeclared variable
💡 Result:
In NSIS (Nullsoft Scriptable Install System), a "variable undeclared" or "unknown variable" compile error occurs because you have attempted to use a user-defined variable without first declaring it using the Var command [1][2]. Unlike the built-in registers ($0-$9, $R0-$R9), which are available by default and do not require declaration, all user-defined variables must be explicitly declared before they are used in your script [3][2][4]. To resolve this error, ensure the following: 1. Declaration: Use the Var command to declare your variable [1][3]. Var MyVariableName StrCpy $MyVariableName "SomeValue" 2. Scope and Placement: - By default, all variables in NSIS are global [1][4]. - If you declare a variable inside a Function or Section, it is still global [1]. However, it is good practice to use the /GLOBAL flag when declaring variables inside these blocks to clarify their scope [1][5]. Var /GLOBAL MyVariableName 3. Usage: Ensure you use the $ prefix when referencing the variable (e.g., $MyVariableName) [3][4]. The declaration itself uses the name without the $ sign [1][4]. If you are using a variable that you believe is already declared, check for spelling discrepancies or ensure the Var declaration statement appears in the script before the code that attempts to access or assign a value to that variable [2].
Citations:
- 1: https://nsis.sourceforge.io/Reference/Var
- 2: https://stackoverflow.com/questions/78194844/cant-use-pop-to-get-control-variable
- 3: https://nsis.sourceforge.io/Docs/Chapter4.html
- 4: https://nsis.sourceforge.io/Docs/Chapter2.html
- 5: https://documentation.help/NSIS/Section4.2.html
Declare IsAdmin before using it. build/installer.nsh writes to $IsAdmin in Function un.onInit, but there’s no Var IsAdmin declaration anywhere in the script. NSIS treats that as an undeclared user variable, so the installer won’t compile; add the declaration or remove the assignment if it isn’t used.
🤖 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 `@build/installer.nsh` around lines 218 - 223, Declare the NSIS user variable
IsAdmin before Function un.onInit assigns to it, using the script’s existing
variable declaration section; preserve the current assignment and initialization
behavior.
| ; Product Definition | ||
| ; ============================================================ | ||
| Name "Soterios" | ||
| OutFile "Soterios-Setup-${PRODUCT_VERSION}.exe" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the installer scripts and surrounding lines.
for f in build/installer.nsi build/installer.nsh; do
echo "===== $f ====="
wc -l "$f"
echo "--- outline ---"
ast-grep outline "$f" --view expanded || true
echo "--- first 120 lines ---"
sed -n '1,120p' "$f" | cat -n
doneRepository: chrisriv10/Soterios
Length of output: 10529
🌐 Web query:
NSIS preprocessor !define order macro expansion OutFile "${PRODUCT_VERSION}" defined later
💡 Result:
In NSIS, the preprocessor processes the script in a single pass [1]. Compile-time commands like!define and OutFile are executed sequentially as the compiler encounters them in the script [2]. Because of this order-dependent evaluation, you cannot reference a symbol (${PRODUCT_VERSION}) in an OutFile command if that symbol is defined later in the script [2][3]. The symbol must be defined before it is used [2]. Key points regarding this behavior: 1. Compile-Time vs. Run-Time:!define creates a compile-time constant, not a run-time variable [4]. Its value is substituted into the script at compile time when the compiler encounters the ${SYMBOL} placeholder [4]. 2. Sequential Evaluation: The compiler maintains a list of defines [2]. If you use ${PRODUCT_VERSION} in your OutFile line, the compiler will look for that symbol in its current list of defines [2]. If it has not been encountered or defined yet (either via an earlier!define or a /D command line switch), the symbol will not be replaced, or may result in an empty value [2][4]. 3. Command Line Overrides: To handle dynamic versions or values defined after the script starts, you can pass definitions to the compiler via the command line using the /D switch (e.g., makensis /DPRODUCT_VERSION=1.2.3 installer.nsi) [2][5][6]. Defines passed this way are available immediately when the script begins execution [2]. If you need to use a value that is only known or calculated during the build process, the standard practice is to either pass it as a /D flag on the command line [6][7] or generate a small header file (.nsh) containing the!define that your main script then includes at the top [6][8][9].
Citations:
- 1: https://stackoverflow.com/questions/1262063/preprocessor-macro-expansion-to-another-preprocessor-directive
- 2: https://nsis.sourceforge.io/Docs/Chapter5.html
- 3: https://nsis.sourceforge.io/Docs/Chapter2.html
- 4: https://nsis-dev.github.io/NSIS-Forums/html/t-330751.html
- 5: https://stackoverflow.com/questions/15714812/using-nsis-command-line-to-globally-define-product-definitions
- 6: https://nsis-dev.github.io/NSIS-Forums/html/t-344804.html
- 7: https://nsis-dev.github.io/NSIS-Forums/html/t-230334.html
- 8: https://nsis-dev.github.io/NSIS-Forums/html/t-257871.html
- 9: https://nsis-dev.github.io/NSIS-Forums/html/t-157467.html
Move the PRODUCT_* defines above OutFile
build/installer.nsi:13 — ${PRODUCT_VERSION} is used before its !define, so the installer filename won’t include the version. Move the PRODUCT_* block above the first use.
🤖 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 `@build/installer.nsi` at line 13, Move the entire PRODUCT_* define block above
the OutFile directive so PRODUCT_VERSION is defined before it is interpolated in
the installer filename; preserve the existing define values and OutFile format.
| ; ============================================================ | ||
| ; Include Custom Branding (only non-conflicting definitions) | ||
| ; ============================================================ | ||
| !include "build/installer.nsh" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files build/installer.nsi build/installer.nsh
printf '\n== installer.nsi (first 220 lines) ==\n'
cat -n build/installer.nsi | sed -n '1,220p'
printf '\n== installer.nsh (first 260 lines) ==\n'
cat -n build/installer.nsh | sed -n '1,260p'Repository: chrisriv10/Soterios
Length of output: 18316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== AST outlines ==\n'
ast-grep outline build/installer.nsi --view expanded || true
printf '\n---\n'
ast-grep outline build/installer.nsh --view expanded || trueRepository: chrisriv10/Soterios
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Search for duplicated installer constructs ==\n'
rg -n --no-heading \
'^(Page|UninstPage|Section|Function|Var|OutFile|InstallDir|RequestExecutionLevel|Name|Icon|BrandingText|PageEx|ShowInstDetails|ShowUninstDetails)' \
build/installer.nsi build/installer.nsh
printf '\n== Search for include usage ==\n'
rg -n --no-heading '!include\s+"build/installer\.nsh"|!include\s+"installer\.nsh"|!include' build/installer.nsi build/installer.nshRepository: chrisriv10/Soterios
Length of output: 5426
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Relevant installer asset references ==\n'
rg -n --no-heading \
'(banner|header|icon|license|welcome|finish|MUI|nsDialogs|InstallDirRegKey|SectionEnd|Function onWelcomePageCreate|Function un\.onWelcomeCreate)' \
build/installer.nsi build/installer.nshRepository: chrisriv10/Soterios
Length of output: 3363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== build/installer.nsi =="
sed -n '1,220p' build/installer.nsi | nl -ba
echo
echo "== build/installer.nsh =="
sed -n '1,260p' build/installer.nsh | nl -baRepository: chrisriv10/Soterios
Length of output: 223
Keep one NSIS owner — build/installer.nsi:27 includes build/installer.nsh, but build/installer.nsh:48-237 also declares pages, variables, functions, and sections, so the combined script redefines the installer. Remove the include or reduce build/installer.nsh to shared helpers only.
📍 Affects 2 files
build/installer.nsi#L27-L27(this comment)build/installer.nsh#L48-L56
🤖 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 `@build/installer.nsi` at line 27, Keep a single NSIS installer owner: remove
the include of build/installer.nsh from build/installer.nsi, or reduce
build/installer.nsh to shared helpers that do not redeclare installer pages,
variables, functions, or sections. Update build/installer.nsh lines 48-56 and
its related declarations only if retaining it as a helper file.
| async getNetworkInterfaces() { | ||
| try { | ||
| const { stdout } = await execAsync('netsh interface show interface', { timeout: 5000 }); | ||
| const lines = stdout.split('\n'); | ||
| const interfaces = []; | ||
|
|
||
| for (const line of lines) { | ||
| const match = line.match(/^\s*(\S+)\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+)\s*$/); | ||
| if (match) { | ||
| const [, name, state, type, connectivity, comment] = match; | ||
| if (type === 'Ethernet' || type === 'Wi-Fi' || type === 'Wireless') { | ||
| interfaces.push({ | ||
| name: name.trim(), | ||
| state: state.trim(), | ||
| type: type.trim(), | ||
| connectivity: connectivity.trim() | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| return interfaces; | ||
| } catch (err) { | ||
| throw new Error(`Failed to get network interfaces: ${err.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What is the exact column layout and casing of "netsh interface show interface" output on modern Windows 10/11?
💡 Result:
The netsh interface show interface command on modern Windows 10 and Windows 11 systems outputs a table with four columns, using Title Case for the headers [1][2][3]. The exact column layout and headers are as follows: 1. Admin State 2. State 3. Type 4. Interface Name Beneath the headers is a separator line composed of hyphens [1][2][3]. The command outputs this information in a fixed-width, space-delimited text format, where each row corresponds to a specific network interface [1][2][4]. Example output structure: Admin State State Type Interface Name ------------------------------------------------------------------------- Enabled Connected Dedicated Wi-Fi Enabled Disconnected Dedicated Ethernet 2 Because the output is designed for human readability rather than strict machine parsing, the columns are separated by varying amounts of whitespace [4]. Automated scripts often parse this by skipping the header lines (the first two lines of output) and using whitespace-delimited tokenization to extract the interface name and status [4].
Citations:
- 1: https://superuser.com/questions/358766/get-active-network-interface-on-windows
- 2: https://www.cyberithub.com/31-most-useful-netsh-command-examples-in-windows/
- 3: https://quickbytesstuff.blogspot.com/2015/10/show-interface-ip-address-in-windows-10.html
- 4: https://stackoverflow.com/questions/42653754/how-to-get-and-use-network-adapter-name
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant ranges with line numbers.
wc -l src/security/EmergencyLockdown.js
sed -n '1,260p' src/security/EmergencyLockdown.js | cat -n
# Search for the state comparisons and any related parsing.
rg -n "state === 'connected'|STATE:|netsh interface show interface|getNetworkInterfaces|getNonEssentialServices|isLockedDown" src/security/EmergencyLockdown.jsRepository: chrisriv10/Soterios
Length of output: 10857
🏁 Script executed:
python3 - <<'PY'
import re
netsh_regex = re.compile(r'^\s*(\S+)\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+)\s*$')
sc_regex = re.compile(r'^\s+STATE:\s+(\d+)\s+(\w+)$')
samples = {
"netsh_row_wifi": "Enabled Connected Dedicated Wi-Fi",
"netsh_row_ethernet2": "Enabled Connected Dedicated Ethernet 2",
"sc_state": " STATE : 4 RUNNING",
}
for name, text in samples.items():
print(name, "netsh_match" if name.startswith("netsh") and netsh_regex.match(text) else "netsh_no_match",
"sc_match" if name.startswith("sc") and sc_regex.match(text) else "sc_no_match")
# Show why the lowercase connected check never matches capitalized output.
for actual in ["Connected", "Disconnected"]:
print(actual, actual == "connected")
PYRepository: chrisriv10/Soterios
Length of output: 314
Emergency-lockdown parsing misses real Windows output
getNetworkInterfaces()expects 5 single-token fields, butnetsh interface show interfacerows are 4 columns and useConnected/Disconnected, so connected adapters won’t be selected.getNonEssentialServices()also missessc queryoutput:STATE : 4 RUNNINGdoes not match^\s+STATE:\s+..., so running services stay undiscovered.
Lockdown can end up reporting success while disabling or stopping 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/security/EmergencyLockdown.js` around lines 24 - 48, Update
getNetworkInterfaces() to parse the four-column netsh interface rows, allowing
interface names and other fields containing spaces, and recognize
Connected/Disconnected status while retaining Ethernet, Wi-Fi, and Wireless
adapter filtering. Update getNonEssentialServices() to match the actual sc query
STATE format, including spacing before the colon and the numeric state code, so
RUNNING services are discovered and processed by lockdown.
| async getNonEssentialServices() { | ||
| const nonEssentialPatterns = [ | ||
| 'Adobe', 'Google', 'Mozilla', 'Spooler', 'Print', 'Fax', 'Xbox', | ||
| 'WSearch', 'SysMain', 'DiagTrack', 'WaaSMedicSvc', 'XblAuthManager', | ||
| 'XblGameSave', 'XboxNetApiSvc', 'BcastDVRUserService', 'OneSync' | ||
| ]; | ||
|
|
||
| try { | ||
| const { stdout } = await execAsync('sc query type= service state= all', { timeout: 10000 }); | ||
| const lines = stdout.split('\n'); | ||
| const services = []; | ||
|
|
||
| let currentService = null; | ||
| for (const line of lines) { | ||
| const serviceNameMatch = line.match(/^SERVICE_NAME:\s*(.+)$/); | ||
| if (serviceNameMatch) { | ||
| if (currentService && currentService.displayName) { | ||
| services.push(currentService); | ||
| } | ||
| currentService = { name: serviceNameMatch[1].trim(), displayName: '', state: '' }; | ||
| } else if (currentService) { | ||
| const displayNameMatch = line.match(/^DISPLAY_NAME:\s*(.+)$/); | ||
| const stateMatch = line.match(/^\s+STATE:\s+(\d+)\s+(\w+)$/); | ||
|
|
||
| if (displayNameMatch) { | ||
| currentService.displayName = displayNameMatch[1].trim(); | ||
| } else if (stateMatch) { | ||
| currentService.state = stateMatch[2].trim(); | ||
| } | ||
| } | ||
| } | ||
| if (currentService && currentService.displayName) { | ||
| services.push(currentService); | ||
| } | ||
|
|
||
| // Filter for non-essential services that are currently running | ||
| return services.filter(svc => { | ||
| const isNonEssential = nonEssentialPatterns.some(pattern => | ||
| svc.name.toLowerCase().includes(pattern.toLowerCase()) || | ||
| svc.displayName.toLowerCase().includes(pattern.toLowerCase()) | ||
| ); | ||
| const isRunning = svc.state === 'RUNNING'; | ||
| return isNonEssential && isRunning; | ||
| }); | ||
| } catch (err) { | ||
| throw new Error(`Failed to get services: ${err.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Windows "sc query" output pad the STATE label with spaces before the colon?
💡 Result:
Yes, the output of the Windows sc query command does include padding with spaces before the colon on the line containing the STATE label [1][2][3]. While the exact number of spaces can vary depending on the specific Windows version or the service being queried, the label is typically displayed as "STATE" followed by a variable amount of whitespace and then the colon [1][2][3]. Many users and technical references observe the label formatted as "STATE: [code] [status]" or sometimes with significant trailing spaces after the word "STATE" itself [1][3]. Because of this variable padding, scripts that attempt to parse the output often use filters like find or findstr to isolate the line first, followed by token-based parsing (such as with the for /f command) to reliably extract the status value regardless of the spacing [4][5][3].
Citations:
- 1: https://serverdevworker.com/52292d17b/
- 2: https://stackoverflow.com/questions/7248044/how-do-i-determine-the-status-of-a-specific-service
- 3: https://tutorialreference.com/batch-scripting/examples/faq/batch-script-how-to-query-the-status-of-a-service
- 4: https://stackoverflow.com/questions/19930486/parsing-output-from-sc-command-in-batch-file
- 5: https://stackoverflow.com/questions/12172997/how-to-collect-each-service-name-and-its-status-in-windows
Relax the STATE regex
sc query pads the STATE label before the colon, so /^\s+STATE:\s+(\d+)\s+(\w+)$/ never matches. state stays empty, isRunning is always false, and getNonEssentialServices() returns no running services.
Suggested fix
- const stateMatch = line.match(/^\s+STATE:\s+(\d+)\s+(\w+)$/);
+ const stateMatch = line.match(/^\s*STATE\s*:\s+(\d+)\s+(\w+)$/);📝 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.
| async getNonEssentialServices() { | |
| const nonEssentialPatterns = [ | |
| 'Adobe', 'Google', 'Mozilla', 'Spooler', 'Print', 'Fax', 'Xbox', | |
| 'WSearch', 'SysMain', 'DiagTrack', 'WaaSMedicSvc', 'XblAuthManager', | |
| 'XblGameSave', 'XboxNetApiSvc', 'BcastDVRUserService', 'OneSync' | |
| ]; | |
| try { | |
| const { stdout } = await execAsync('sc query type= service state= all', { timeout: 10000 }); | |
| const lines = stdout.split('\n'); | |
| const services = []; | |
| let currentService = null; | |
| for (const line of lines) { | |
| const serviceNameMatch = line.match(/^SERVICE_NAME:\s*(.+)$/); | |
| if (serviceNameMatch) { | |
| if (currentService && currentService.displayName) { | |
| services.push(currentService); | |
| } | |
| currentService = { name: serviceNameMatch[1].trim(), displayName: '', state: '' }; | |
| } else if (currentService) { | |
| const displayNameMatch = line.match(/^DISPLAY_NAME:\s*(.+)$/); | |
| const stateMatch = line.match(/^\s+STATE:\s+(\d+)\s+(\w+)$/); | |
| if (displayNameMatch) { | |
| currentService.displayName = displayNameMatch[1].trim(); | |
| } else if (stateMatch) { | |
| currentService.state = stateMatch[2].trim(); | |
| } | |
| } | |
| } | |
| if (currentService && currentService.displayName) { | |
| services.push(currentService); | |
| } | |
| // Filter for non-essential services that are currently running | |
| return services.filter(svc => { | |
| const isNonEssential = nonEssentialPatterns.some(pattern => | |
| svc.name.toLowerCase().includes(pattern.toLowerCase()) || | |
| svc.displayName.toLowerCase().includes(pattern.toLowerCase()) | |
| ); | |
| const isRunning = svc.state === 'RUNNING'; | |
| return isNonEssential && isRunning; | |
| }); | |
| } catch (err) { | |
| throw new Error(`Failed to get services: ${err.message}`); | |
| } | |
| } | |
| async getNonEssentialServices() { | |
| const nonEssentialPatterns = [ | |
| 'Adobe', 'Google', 'Mozilla', 'Spooler', 'Print', 'Fax', 'Xbox', | |
| 'WSearch', 'SysMain', 'DiagTrack', 'WaaSMedicSvc', 'XblAuthManager', | |
| 'XblGameSave', 'XboxNetApiSvc', 'BcastDVRUserService', 'OneSync' | |
| ]; | |
| try { | |
| const { stdout } = await execAsync('sc query type= service state= all', { timeout: 10000 }); | |
| const lines = stdout.split('\n'); | |
| const services = []; | |
| let currentService = null; | |
| for (const line of lines) { | |
| const serviceNameMatch = line.match(/^SERVICE_NAME:\s*(.+)$/); | |
| if (serviceNameMatch) { | |
| if (currentService && currentService.displayName) { | |
| services.push(currentService); | |
| } | |
| currentService = { name: serviceNameMatch[1].trim(), displayName: '', state: '' }; | |
| } else if (currentService) { | |
| const displayNameMatch = line.match(/^DISPLAY_NAME:\s*(.+)$/); | |
| const stateMatch = line.match(/^\s*STATE\s*:\s+(\d+)\s+(\w+)$/); | |
| if (displayNameMatch) { | |
| currentService.displayName = displayNameMatch[1].trim(); | |
| } else if (stateMatch) { | |
| currentService.state = stateMatch[2].trim(); | |
| } | |
| } | |
| } | |
| if (currentService && currentService.displayName) { | |
| services.push(currentService); | |
| } | |
| // Filter for non-essential services that are currently running | |
| return services.filter(svc => { | |
| const isNonEssential = nonEssentialPatterns.some(pattern => | |
| svc.name.toLowerCase().includes(pattern.toLowerCase()) || | |
| svc.displayName.toLowerCase().includes(pattern.toLowerCase()) | |
| ); | |
| const isRunning = svc.state === 'RUNNING'; | |
| return isNonEssential && isRunning; | |
| }); | |
| } catch (err) { | |
| throw new Error(`Failed to get services: ${err.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 `@src/security/EmergencyLockdown.js` around lines 77 - 124, Update the STATE
parsing in getNonEssentialServices so the regex permits whitespace before the
colon in the padded “STATE” label, while still capturing the numeric state and
status text. Preserve the existing isRunning check against “RUNNING” and the
surrounding service filtering behavior.
Refactored `browser-extension-host.js` stdin parsing to use a persistent stream buffer with a single `data` listener instead of attaching new `readable` listeners on every `readMessage` call, eliminating listener accumulation and memory leaks in the native host process. Added `CHECK_NATIVE_HOST` handler in `browser-extension/background.js` to let the extension verify if the native desktop host is installed and running, enabling clearer error messaging for disconnected hosts. Updated icon cleanup logic in `browser-extension/content.js` to properly remove attached `scroll` and `resize` event listeners, and fully clear password field tracking when icons are dismissed or icon display is disabled via settings, preventing memory leaks from orphaned listeners and stale field references.
| 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/ipcHandlers.js (1)
776-779: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
payload.countbefore formatting the alert. If the extension sends a payload without a numericcount, the message rendersPassword found in undefined breach...and the metadata storesundefined. Coerce/validate to a positive integer.🛠️ Suggested guard
- if (!payload?.password) return { ok: false, error: 'Missing password' }; + if (!payload?.password) return { ok: false, error: 'Missing password' }; + const count = Number.isFinite(payload.count) && payload.count > 0 ? Math.floor(payload.count) : 0; 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 } };🤖 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 776 - 779, Validate and normalize payload.count before constructing the alert fields in the surrounding IPC handler: coerce it to a positive integer and use the validated value for the pluralized message, breach detail, and metadata count. Reject or safely handle payloads whose count is missing, non-numeric, or not positive so no undefined or invalid count is rendered.src/security/EmergencyLockdown.js (1)
224-291: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRestore retries reprocess already-restored items, permanently blocking
status: 'success'.
savedNetworkState/savedServicesStateare only cleared whenstatus === 'success'(line 268-271); otherwise they're left completely intact for the nextrestore()call (line 281-289). But the restore loops (lines 228-253) always iterate the entire saved list again, with no tracking of which items already succeeded. For network interfaces, re-runningnetsh ... admin=enableon an already-enabled adapter is typically harmless, but for services, re-runningsc starton a service that a prior partial restore already started successfully returns "service already running" — which gets pushed intoresults.errorsagain on every subsequent retry.The practical effect: once any single service fails to restart on the first attempt (e.g., a transient error), every later restore attempt re-flags the already-recovered services as errors, so
allServicesRestorednever becomes true again for the remaining state,statusnever returns to'success', and the app is stuck reporting a partial/failed lockdown indefinitely even though the machine is actually functional. This also meansisLockedDownnever clears and the "still locked" notification/event fires repeatedly for items that are no longer actually a problem.Track and persist only the items that still need restoring (i.e., prune successes from
savedNetworkState/savedServicesStatebefore returning), so a subsequentrestore()call only retries the genuinely-failed subset.💡 Suggested approach
// Restore network interfaces + const remainingNetworkState = []; if (this.savedNetworkState) { for (const iface of this.savedNetworkState) { if (iface.state === 'connected') { try { await this.enableInterface(iface.name); results.enabledInterfaces.push(iface.name); } catch (err) { results.errors.push(`Network: ${err.message}`); + remainingNetworkState.push(iface); } + } else { + remainingNetworkState.push(iface); } } } // Restore services + const remainingServicesState = []; if (this.savedServicesState) { for (const svc of this.savedServicesState) { if (svc.state === 'RUNNING') { try { await this.startService(svc.name); results.startedServices.push(svc.name); } catch (err) { results.errors.push(`Service: ${err.message}`); + remainingServicesState.push(svc); } + } else { + remainingServicesState.push(svc); } } } ... if (status === 'success') { this.isLockedDown = false; this.savedNetworkState = null; this.savedServicesState = null; + } else { + this.savedNetworkState = remainingNetworkState; + this.savedServicesState = remainingServicesState; }Note: this is also entangled with the still-unresolved
state === 'connected'casing bug flagged above — since real interface state values won't equal lowercase'connected',totalInterfacesToRestorewill likely always compute to 0, makingallInterfacesRestoredvacuously true and masking real network-restore failures from this new status logic entirely.🤖 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/security/EmergencyLockdown.js` around lines 224 - 291, Update restore() so successful interfaces and services are removed from savedNetworkState and savedServicesState before returning, leaving only items that still need restoration for subsequent retries. Preserve the existing success/partial status behavior while ensuring already-started services are not retried and re-reported as errors. Also correct the network state comparison to match the actual connected-state casing so interface failures are counted accurately.
🧹 Nitpick comments (4)
browser-extension/content.js (1)
154-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated icon/listener teardown risks divergence. The removal logic here mirrors
cleanup()inaddIconToField(Lines 101-113). Since thescrolllistener is registered with capture (true) andresizewithout, both teardown paths must stay byte-for-byte in sync or listeners will leak. Extract a shareddetachIcon(input, icon)helper used by both the blur handler and this branch.🤖 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 154 - 166, Extract the duplicated icon and listener teardown from addIconToField’s blur handler and the shown passwordFields cleanup branch into a shared detachIcon(input, icon) helper. Preserve the existing capture flags for scroll and resize removal, icon removal, and soteriosId dataset cleanup, then call the helper from both paths.src/main/ipcHandlers.js (1)
963-968: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse
execFileSyncwith argv here.regaccepts discrete arguments directly, so the registry path and value do not need manual quoting/escaping; that also removes the shell-injection surface from the dynamic key 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 `@src/main/ipcHandlers.js` around lines 963 - 968, Replace the string-based execSync registry commands in the native messaging registration flow with execFileSync calls using discrete argv arrays for both Chrome and Edge. Update the relevant regPath/regCmd and regPathEdge/regCmdEdge handling so dynamic registry paths and manifest values are passed as arguments without manual quoting or backslash escaping, while preserving the existing execution behavior and ignored stdio.Source: Linters/SAST tools
browser-extension-host.js (1)
59-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider exiting the process on stdin
'end', not just rejecting the pending read.Chrome closes the native host's stdin to signal it should terminate. Currently the
'end'handler only rejects any in-flightmessageResolver; if noreadMessage()is pending at that moment (e.g., between processing a message and awaiting the next one), the event does nothing and the process relies entirely on the main loop noticing the next rejection. Explicitly callingprocess.exit(0)here (after any cleanup) more directly matches the documented Chrome native-messaging host lifecycle and avoids depending on loop-level string matching.🤖 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-host.js` around lines 59 - 64, Update the stdin `'end'` handler to explicitly terminate the native host with exit code 0 after rejecting and clearing any pending messageResolver, ensuring cleanup occurs before process.exit.src/security/EmergencyLockdown.js (1)
255-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant conditions in the
statusdetermination — minor readability note.Given every path that decrements
enabledInterfaces/startedServicesbelow their totals also pushes toresults.errors,hasErrorsalready implies!allInterfacesRestored/!allServicesRestoredwhenever they're false. The extra(!allInterfacesRestored && totalInterfacesToRestore > 0) || (!allServicesRestored && totalServicesToRestore > 0)terms in theelse ifare effectively unreachable givenhasErrorsis already checked first in that same condition. Not a correctness issue, just avoidable complexity.🤖 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/security/EmergencyLockdown.js` around lines 255 - 266, Remove the redundant allInterfacesRestored/allServicesRestored and totalInterfacesToRestore/totalServicesToRestore checks from the failed branch in the status determination, leaving the existing hasErrors-based partial and failed behavior unchanged.
🤖 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-host.js`:
- Around line 46-64: Update the read loop in main() to recognize the new 'Stream
ended' rejection from the process.stdin end handler and break cleanly, alongside
the existing 'Unexpected end of JSON' termination handling. Ensure stdin closure
does not trigger another readMessage() call or leave the host waiting
indefinitely.
In `@browser-extension/native-host.js`:
- Around line 76-102: Update both desktop launch branches in the Promise around
desktopProc.on('error') so a spawn error rejects the Promise and cancels the
pending 1500ms success timer instead of resolving later. Track each branch’s
timeout handle, reject with the launch error, and preserve normal resolution
only when the timer completes without an error.
In `@src/main/ipcHandlers.js`:
- Around line 946-961: Move the try/catch in the handler so it encloses the
entire manifest and batch-file generation flow, including JSON.parse,
allowed_origins access, all readFileSync calls, and both writeFileSync calls.
Preserve the existing { ok, error } response contract by routing any parsing,
malformed-data, or filesystem failure through the catch rather than allowing the
invoke promise to reject.
---
Outside diff comments:
In `@src/main/ipcHandlers.js`:
- Around line 776-779: Validate and normalize payload.count before constructing
the alert fields in the surrounding IPC handler: coerce it to a positive integer
and use the validated value for the pluralized message, breach detail, and
metadata count. Reject or safely handle payloads whose count is missing,
non-numeric, or not positive so no undefined or invalid count is rendered.
In `@src/security/EmergencyLockdown.js`:
- Around line 224-291: Update restore() so successful interfaces and services
are removed from savedNetworkState and savedServicesState before returning,
leaving only items that still need restoration for subsequent retries. Preserve
the existing success/partial status behavior while ensuring already-started
services are not retried and re-reported as errors. Also correct the network
state comparison to match the actual connected-state casing so interface
failures are counted accurately.
---
Nitpick comments:
In `@browser-extension-host.js`:
- Around line 59-64: Update the stdin `'end'` handler to explicitly terminate
the native host with exit code 0 after rejecting and clearing any pending
messageResolver, ensuring cleanup occurs before process.exit.
In `@browser-extension/content.js`:
- Around line 154-166: Extract the duplicated icon and listener teardown from
addIconToField’s blur handler and the shown passwordFields cleanup branch into a
shared detachIcon(input, icon) helper. Preserve the existing capture flags for
scroll and resize removal, icon removal, and soteriosId dataset cleanup, then
call the helper from both paths.
In `@src/main/ipcHandlers.js`:
- Around line 963-968: Replace the string-based execSync registry commands in
the native messaging registration flow with execFileSync calls using discrete
argv arrays for both Chrome and Edge. Update the relevant regPath/regCmd and
regPathEdge/regCmdEdge handling so dynamic registry paths and manifest values
are passed as arguments without manual quoting or backslash escaping, while
preserving the existing execution behavior and ignored stdio.
In `@src/security/EmergencyLockdown.js`:
- Around line 255-266: Remove the redundant
allInterfacesRestored/allServicesRestored and
totalInterfacesToRestore/totalServicesToRestore checks from the failed branch in
the status determination, leaving the existing hasErrors-based partial and
failed behavior unchanged.
🪄 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: fa7260bc-0884-4780-badf-e779d07630be
📒 Files selected for processing (9)
browser-extension-host.jsbrowser-extension/background.jsbrowser-extension/content.jsbrowser-extension/native-host.jsbrowser-extension/popup.jspackage.jsonsrc/main/ipcHandlers.jssrc/security/EmergencyLockdown.jstools/install-native-host.js
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- tools/install-native-host.js
- browser-extension/background.js
- browser-extension/popup.js
| // Set up persistent stdin listener once | ||
| process.stdin.on('data', (chunk) => { | ||
| messageBuffer = Buffer.concat([messageBuffer, chunk]); | ||
| tryParseBuffer(); | ||
| }); | ||
|
|
||
| process.stdin.on('error', (err) => { | ||
| if (messageResolver) { | ||
| messageResolver.reject(err); | ||
| messageResolver = null; | ||
| } | ||
| }); | ||
|
|
||
| process.stdin.on('end', () => { | ||
| if (messageResolver) { | ||
| messageResolver.reject(new Error('Stream ended')); | ||
| messageResolver = null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect main()'s loop/catch logic and its handling of readMessage() rejections
sed -n '66,119p' browser-extension-host.js | cat -n
rg -n "Unexpected end of JSON|Stream ended|Failed to parse message|messageResolver" browser-extension-host.jsRepository: chrisriv10/Soterios
Length of output: 2607
Handle the new stdin-close rejection in main()
main() still breaks only on 'Unexpected end of JSON', but readMessage() now rejects EOF with new Error('Stream ended'). That leaves the loop running after stdin closes, and the next readMessage() will wait forever. Break on the new stream-closed error (or use a dedicated sentinel) so the host exits cleanly.
🤖 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-host.js` around lines 46 - 64, Update the read loop in
main() to recognize the new 'Stream ended' rejection from the process.stdin end
handler and break cleanly, alongside the existing 'Unexpected end of JSON'
termination handling. Ensure stdin closure does not trigger another
readMessage() call or leave the host waiting indefinitely.
| 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 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.on('error', e => { | ||
| log('Desktop app launch error:', e.message); | ||
| desktopProc = null; | ||
| }); | ||
|
|
||
| setTimeout(resolve, 1500); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Launch reports success even when the spawn fails. In both branches the 'error' handler only logs and nulls desktopProc, while setTimeout(resolve, 1500) still resolves the Promise. handleMessage then replies APP_OPENED/LEAK_NOTIFIED as if the desktop app launched. Reject (and cancel the timer) on spawn error so callers see the failure.
🛠️ Suggested change (per branch)
- desktopProc = spawn(cmd, args, options);
- desktopProc.unref();
-
- desktopProc.on('error', e => {
- log('Desktop app launch error:', e.message);
- desktopProc = null;
- });
-
- setTimeout(resolve, 1500);
+ desktopProc = spawn(cmd, args, options);
+ desktopProc.unref();
+
+ const timer = setTimeout(resolve, 1500);
+ desktopProc.on('error', e => {
+ clearTimeout(timer);
+ log('Desktop app launch error:', e.message);
+ desktopProc = null;
+ reject(e);
+ });📝 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.
| 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 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.on('error', e => { | |
| log('Desktop app launch error:', e.message); | |
| desktopProc = null; | |
| }); | |
| setTimeout(resolve, 1500); | |
| desktopProc = spawn(cmd, args, options); | |
| desktopProc.unref(); | |
| const timer = setTimeout(resolve, 1500); | |
| desktopProc.on('error', e => { | |
| clearTimeout(timer); | |
| log('Desktop app launch error:', e.message); | |
| desktopProc = null; | |
| reject(e); | |
| }); |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 80-80: Avoid using the initial state variable in setState
Context: setTimeout(resolve, 1500)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 101-101: Avoid using the initial state variable in setState
Context: setTimeout(resolve, 1500)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🪛 GitHub Check: CodeQL
[warning] 94-94: 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 76 - 102, Update both desktop
launch branches in the Promise around desktopProc.on('error') so a spawn error
rejects the Promise and cancels the pending 1500ms success timer instead of
resolving later. Track each branch’s timeout handle, reject with the launch
error, and preserve normal resolution only when the timer completes without an
error.
| const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')); | ||
| manifest.allowed_origins = [manifest.allowed_origins[0].replace('<EXTENSION_ID>', extId)]; | ||
|
|
||
| // Write generated manifest to app data directory instead of mutating the shipped file | ||
| const userDataDir = app.getPath('userData'); | ||
| const generatedManifestPath = path.join(userDataDir, 'native-host-manifest.json'); | ||
| fs.writeFileSync(generatedManifestPath, JSON.stringify(manifest, null, 2)); | ||
|
|
||
| // Update bat file to reference the correct js path | ||
| const batContent = fs.readFileSync(batPath, 'utf8'); | ||
| const updatedBatContent = batContent.replace( | ||
| /node\s+"[^"]*native-host\.js"/, | ||
| `node "${jsPath}"` | ||
| ); | ||
| const generatedBatPath = path.join(userDataDir, 'native-host.bat'); | ||
| fs.writeFileSync(generatedBatPath, updatedBatContent); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
File operations here can throw and break the { ok, error } contract. JSON.parse, manifest.allowed_origins[0] (undefined if the array is empty/malformed), and the two fs.writeFileSync calls all run before the try at Line 965. A parse error or write failure (e.g., EPERM) rejects the invoke promise instead of returning { ok: false, error } like every other path in this handler. Wrap the whole body in the try/catch.
🛠️ Move the try to cover all I/O
- // Read template manifest and generate a new one in app data directory
- const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8'));
- manifest.allowed_origins = [manifest.allowed_origins[0].replace('<EXTENSION_ID>', extId)];
+ try {
+ // Read template manifest and generate a new one in app data directory
+ const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8'));
+ if (!Array.isArray(manifest.allowed_origins) || !manifest.allowed_origins[0]) {
+ return { ok: false, error: 'Malformed native host manifest.' };
+ }
+ manifest.allowed_origins = [manifest.allowed_origins[0].replace('<EXTENSION_ID>', extId)];
+ // ... remaining file/registry work, existing try body ...
+ } catch (e) {
+ return { ok: false, error: e.message || String(e) };
+ }🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 951-951: 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(generatedManifestPath, JSON.stringify(manifest, null, 2))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 954-954: 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.readFileSync(batPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 960-960: 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(generatedBatPath, updatedBatContent)
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 946 - 961, Move the try/catch in the
handler so it encloses the entire manifest and batch-file generation flow,
including JSON.parse, allowed_origins access, all readFileSync calls, and both
writeFileSync calls. Preserve the existing { ok, error } response contract by
routing any parsing, malformed-data, or filesystem failure through the catch
rather than allowing the invoke promise to reject.
…ild issues - Fix onInstalled listener to only enable external lookups on initial extension install, preventing setting resets on updates - Add user-configurable showIcon and autoCheck settings synced via chrome.storage.sync, with real-time updates when settings change - Implement automatic password checking on input when autoCheck is enabled, with proper event listener cleanup for removed icons - Rename native host environment variable to DESKTOP_APP for consistency - Update welcome image conversion script to use WIC for reliable SVG rasterization
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/content.js`:
- Around line 150-151: Update the CHECK_PASSWORD message handling around
showResult so responses containing result.error render an unavailable or unknown
state instead of being treated as safe; preserve the existing clean-result
behavior for successful responses with a valid pwned value.
- Around line 11-29: Update init() so it waits for loadSettings() to complete
before calling scanForPasswordFields(), ensuring persisted settings apply to the
first scan. Extend the chrome.storage.onChanged listener to reconcile existing
password fields after updating currentSettings, adding or removing icons as
needed to match the new showIcon setting.
In `@build/convert-welcome.ps1`:
- Around line 18-21: Update the image conversion flow around `$frame` and
`$graphics.DrawImage` so the WPF `BitmapSource` is converted to a compatible
`System.Drawing.Bitmap` or other `System.Drawing.Image` before drawing. Use the
converted image in `DrawImage`, preserving the existing 500x120 output
dimensions and rendering behavior.
- Around line 12-15: Update the BitmapDecoder.Create call in the convert-welcome
script to use the overload’s expected enum order: provide BitmapCacheOption in
the cache-option position and BitmapCreateOptions in the create-options
position. Preserve the existing SVG URI and read access arguments.
- Around line 11-16: Replace the WIC-based BitmapDecoder.Create call in the SVG
loading section with an SVG-capable rasterizer, or update the script to use a
checked-in pre-rendered BMP instead. Ensure the conversion continues to produce
the same welcome image without relying on third-party Windows SVG codecs.
In `@src/main/ipcHandlers.js`:
- Around line 954-976: Update the batch content construction to use the already
resolved nodeExePath variable instead of nodeRuntime. Remove the PATH-based
fallback so packaged installations always invoke the bundled Node executable
selected by the platform-specific logic above.
🪄 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: 2a90bbef-e5af-4a12-b303-ef441de862d4
📒 Files selected for processing (9)
browser-extension/background.jsbrowser-extension/content.jsbrowser-extension/native-host.jsbuild/convert-welcome.ps1build/installer.nsisrc/i18n/locales/ja.jsonsrc/main/ipcHandlers.jssrc/security/EmergencyLockdown.jssrc/ui/js/pages/lockdown.js
💤 Files with no reviewable changes (1)
- src/i18n/locales/ja.json
🚧 Files skipped from review as they are similar to previous changes (4)
- browser-extension/background.js
- src/ui/js/pages/lockdown.js
- build/installer.nsi
- src/security/EmergencyLockdown.js
| // Load settings from storage | ||
| function loadSettings() { | ||
| chrome.storage.sync.get(['showIcon', 'autoCheck'], (result) => { | ||
| currentSettings.showIcon = result.showIcon !== false; | ||
| currentSettings.autoCheck = result.autoCheck === true; | ||
| }); | ||
| } | ||
|
|
||
| // Listen for settings updates | ||
| chrome.storage.onChanged.addListener((changes, namespace) => { | ||
| if (namespace === 'sync') { | ||
| if (changes.showIcon !== undefined) { | ||
| currentSettings.showIcon = changes.showIcon.newValue !== false; | ||
| } | ||
| if (changes.autoCheck !== undefined) { | ||
| currentSettings.autoCheck = changes.autoCheck.newValue === true; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby symbols first.
git ls-files browser-extension/content.js
printf '\n--- outline ---\n'
ast-grep outline browser-extension/content.js --view expanded || true
printf '\n--- relevant lines ---\n'
wc -l browser-extension/content.js
sed -n '1,260p' browser-extension/content.jsRepository: chrisriv10/Soterios
Length of output: 7603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file around init/loadSettings and the settings change handling.
nl -ba browser-extension/content.js | sed -n '1,260p'Repository: chrisriv10/Soterios
Length of output: 197
Apply settings before the first scan.
init() calls scanForPasswordFields() before chrome.storage.sync.get() finishes, so a saved showIcon: false can still add icons on first load. The storage-change listener only updates currentSettings; it never removes or adds existing icons, so the DOM can stay out of sync until a later SETTINGS_UPDATED message. Make the initial settings read part of the startup flow, and reconcile already-added fields when settings change.
🤖 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 11 - 29, Update init() so it waits
for loadSettings() to complete before calling scanForPasswordFields(), ensuring
persisted settings apply to the first scan. Extend the chrome.storage.onChanged
listener to reconcile existing password fields after updating currentSettings,
adding or removing icons as needed to match the new showIcon setting.
| const result = await chrome.runtime.sendMessage({ type: 'CHECK_PASSWORD', password }); | ||
| showResult(input, result); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not render HIBP failures as a safe password.
background.js returns { error } on fetch failures, but this passes that object to showResult(), where a missing pwned value is treated as false. Render an unavailable/unknown state when result.error is present instead of a clean result.
🤖 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 150 - 151, Update the
CHECK_PASSWORD message handling around showResult so responses containing
result.error render an unavailable or unknown state instead of being treated as
safe; preserve the existing clean-result behavior for successful responses with
a valid pwned value.
| # Load SVG using WIC | ||
| $decoder = [System.Windows.Media.Imaging.BitmapDecoder]::Create( | ||
| [System.Uri]::new((Resolve-Path $svgPath)), | ||
| [System.IO.FileAccess]::Read, | ||
| [System.Windows.Media.Imaging.BitmapCreateOptions]::IgnoreColorProfile | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'build/convert-welcome.ps1' 'build/installer.nsh' 'build/*welcome*' 'build/*svg*' 'build/*bmp*' 'build/*png*' | sed 's#^`#FILE`: #'
printf '\n--- convert-welcome.ps1 ---\n'
cat -n build/convert-welcome.ps1
printf '\n--- installer.nsh ---\n'
cat -n build/installer.nshRepository: chrisriv10/Soterios
Length of output: 13626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show any references to the welcome banner asset and the convert script.
rg -n --hidden --glob '!**/.git/**' 'welcome-banner|convert-welcome|BitmapDecoder|WIC|svg' build .github . || trueRepository: chrisriv10/Soterios
Length of output: 17178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the commit-level diff for the touched files only.
git diff --stat -- build/convert-welcome.ps1 build/installer.nshRepository: chrisriv10/Soterios
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If a build manifest exists, inspect how the welcome banner is produced.
fd -a 'package*.json|*.yml|*.yaml|*.ps1|*.nsh|*.iss|*.wxs' build .github . | sed 's#^`#CANDIDATE`: #'Repository: chrisriv10/Soterios
Length of output: 501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- package.json scripts and build tooling ---\n'
cat -n package.json | sed -n '1,220p'
printf '\n--- references to convert-welcome.ps1 ---\n'
rg -n --hidden --glob '!**/.git/**' 'convert-welcome\.ps1|welcome-banner\.bmp|finish-banner\.bmp|svgexport|sharp|rasterize|bitmapdecoder' .
printf '\n--- tools/build-icons.js ---\n'
cat -n tools/build-icons.js
printf '\n--- browser-extension/package.json ---\n'
cat -n browser-extension/package.jsonRepository: chrisriv10/Soterios
Length of output: 7416
Replace WIC SVG decoding here build/convert-welcome.ps1:11-16 — BitmapDecoder.Create won’t read SVG on stock Windows, so this only works on machines with a third-party SVG codec installed. Use an SVG-capable rasterizer or keep a pre-rendered BMP in the repo.
🤖 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 `@build/convert-welcome.ps1` around lines 11 - 16, Replace the WIC-based
BitmapDecoder.Create call in the SVG loading section with an SVG-capable
rasterizer, or update the script to use a checked-in pre-rendered BMP instead.
Ensure the conversion continues to produce the same welcome image without
relying on third-party Windows SVG codecs.
| $decoder = [System.Windows.Media.Imaging.BitmapDecoder]::Create( | ||
| [System.Uri]::new((Resolve-Path $svgPath)), | ||
| [System.IO.FileAccess]::Read, | ||
| [System.Windows.Media.Imaging.BitmapCreateOptions]::IgnoreColorProfile |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- file: build/convert-welcome.ps1 (lines 1-80) ---'
nl -ba build/convert-welcome.ps1 | sed -n '1,80p'
printf '\n%s\n' '--- search for BitmapDecoder.Create usage ---'
rg -n "BitmapDecoder\.Create|Graphics\.DrawImage|BitmapFrame" build/convert-welcome.ps1Repository: chrisriv10/Soterios
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib
p = pathlib.Path('build/convert-welcome.ps1')
text = p.read_text(encoding='utf-8')
for i, line in enumerate(text.splitlines(), 1):
if 1 <= i <= 80:
print(f"{i:4d}: {line}")
PYRepository: chrisriv10/Soterios
Length of output: 1165
🌐 Web query:
System.Windows.Media.Imaging.BitmapDecoder.Create Uri BitmapCreateOptions BitmapCacheOption overload FileAccess.Read BitmapCreateOptions IgnoreColorProfile enum values
💡 Result:
The BitmapDecoder.Create method in WPF (System.Windows.Media.Imaging) is used to create a decoder from a Uri or a Stream with specific initialization and caching behaviors [1][2][3]. BitmapDecoder.Create Overloads The method includes several overloads, primarily accepting a Uri or Stream, along with BitmapCreateOptions and BitmapCacheOption [1][3]. A common overload for URIs is [1][2]: public static BitmapDecoder Create(Uri bitmapUri, BitmapCreateOptions createOptions, BitmapCacheOption cacheOption); An additional overload for URIs also accepts a RequestCachePolicy [2][3]: public static BitmapDecoder Create(Uri bitmapUri, BitmapCreateOptions createOptions, BitmapCacheOption cacheOption, RequestCachePolicy uriCachePolicy); BitmapCreateOptions Enum This enumeration specifies initialization options and supports bitwise combinations [4][5]. - None (0): Default behavior [4][5]. - PreservePixelFormat (1): Ensures the loaded PixelFormat matches the file's storage format [4][5]. - DelayCreation (2): Delays initialization until necessary [4][5]. - IgnoreColorProfile (4): Causes the BitmapSource to ignore an embedded color profile [4][5]. When set, methods such as CopyPixels do not return color-corrected bits [4][5]. - IgnoreImageCache (8): Loads images without using an existing cache [4][5]. BitmapCacheOption Enum This enumeration controls memory caching [6][7]. - Default (0): Caches the entire image in memory [6][7]. - OnDemand (0): Caches only requested data [6][7]. - OnLoad (1): Caches the entire image into memory at load time, allowing the source stream to be closed [6][7][8]. - None (2): No memory store is created; all requests hit the file directly [6][7]. Note on FileAccess.Read: While BitmapDecoder.Create is used to read data, FileAccess.Read is a standard.NET FileIO enumeration and is not a direct parameter in the BitmapDecoder.Create method signature. It would typically be used when creating the Stream passed to the method.
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.windows.media.imaging.bitmapdecoder.create?view=windowsdesktop-10.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.windows.media.imaging.bitmapdecoder?view=windowsdesktop-10.0
- 3: https://source.dot.net/PresentationCore/System/Windows/Media/Imaging/BitmapDecoder.cs.html
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.windows.media.imaging.bitmapcreateoptions?view=windowsdesktop-10.0
- 5: https://technet.microsoft.com/library/system.windows.media.imaging.bitmapcreateoptions
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.windows.media.imaging.bitmapcacheoption?view=windowsdesktop-10.0
- 7: https://source.dot.net/PresentationCore/System/Windows/Media/Imaging/BitmapCacheOption.cs.html
- 8: https://learn.microsoft.com/en-us/dotnet/api/system.windows.media.imaging.bitmapimage.cacheoption?view=windowsdesktop-10.0
Pass BitmapDecoder.Create the enum values in the right order. FileAccess.Read is not part of this overload; the current call feeds BitmapCreateOptions into the cache-option slot and can configure the decoder incorrectly.
🤖 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 `@build/convert-welcome.ps1` around lines 12 - 15, Update the
BitmapDecoder.Create call in the convert-welcome script to use the overload’s
expected enum order: provide BitmapCacheOption in the cache-option position and
BitmapCreateOptions in the create-options position. Preserve the existing SVG
URI and read access arguments.
| $frame = $decoder.Frames[0] | ||
| $bmp = New-Object System.Drawing.Bitmap(500, 120) | ||
| $graphics = [System.Drawing.Graphics]::FromImage($bmp) | ||
| $graphics.DrawImage($frame, 0, 0, 500, 120) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'build/convert-welcome.ps1'
echo '---'
cat -n build/convert-welcome.ps1 | sed -n '1,120p'Repository: chrisriv10/Soterios
Length of output: 1219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import sys, os, re, pathlib, textwrap, json
p = pathlib.Path('build/convert-welcome.ps1')
print(p.exists())
PYRepository: chrisriv10/Soterios
Length of output: 162
🌐 Web query:
System.Windows.Media.Imaging.BitmapFrame inherits BitmapSource; System.Drawing.Graphics.DrawImage accepts System.Drawing.Image overloads
💡 Result:
Yes, the statements in your query are accurate according to the.NET documentation: 1. System.Windows.Media.Imaging.BitmapFrame inherits from System.Windows.Media.Imaging.BitmapSource [1][2][3]. The BitmapFrame class is an abstract class that extends BitmapSource to provide additional functionality, such as support for multi-frame image formats like TIFF or GIF [4][5]. 2. System.Drawing.Graphics.DrawImage accepts System.Drawing.Image overloads [6][7][8]. The DrawImage method in the System.Drawing.Graphics class provides numerous overloads, all of which take an object of type System.Drawing.Image (the base class for Bitmaps and Metafiles) as the primary parameter to define the image to be drawn [6][9].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.windows.media.imaging.bitmapframe?view=netframework-4.8.1
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.windows.media.imaging.bitmapframe?view=windowsdesktop-10.0
- 3: https://source.dot.net/PresentationCore/System/Windows/Media/Imaging/BitmapFrame.cs.html
- 4: https://github.com/dotnet/dotnet-api-docs/blob/main/xml/System.Windows.Media.Imaging/BitmapFrame.xml
- 5: https://learn.microsoft.com/en-us/dotnet/desktop/wpf/graphics-multimedia/imaging-overview
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.drawing.graphics.drawimage?view=net-8.0
- 7: https://learn.microsoft.com/en-us/dotnet/api/system.drawing.graphics.drawimage?view=netframework-4.8.1
- 8: https://learn.microsoft.com/en-us/dotnet/api/system.drawing.graphics.drawimage?view=net-11.0-pp
- 9: https://learn.microsoft.com/en-us/dotnet/api/system.drawing.graphics?view=windowsdesktop-10.0
🌐 Web query:
site:learn.microsoft.com System.Drawing.Graphics.DrawImage BitmapSource BitmapFrame overloads
💡 Result:
The System.Drawing.Graphics.DrawImage method does not provide overloads that accept BitmapSource or BitmapFrame [1][2][3]. System.Drawing.Graphics.DrawImage operates on the System.Drawing.Image class (and its subclasses, such as System.Drawing.Bitmap and System.Drawing.Metafile) [4][5]. In contrast, BitmapSource and BitmapFrame are part of the Windows Presentation Foundation (WPF) imaging namespace (System.Windows.Media.Imaging) and represent a different architecture [6][7]. To use a BitmapSource or BitmapFrame with System.Drawing.Graphics.DrawImage, you must first convert the WPF-based image to a System.Drawing.Bitmap. Common approaches for this conversion include: 1. Using an encoder: Save the BitmapSource to a MemoryStream using a format like BMP or PNG, then initialize a new System.Drawing.Bitmap from that stream [8]. 2. Using CopyPixels: Copy the pixel data directly from the BitmapSource into a byte array, then create a System.Drawing.Bitmap using the constructor that accepts pixel data, stride, and format [7][8]. 3. Using helper libraries: Certain specialized libraries (such as the Microsoft Visual Studio SDK) provide helper methods like BitmapFromBitmapSource to facilitate this conversion [9][10]. There are no direct overloads in the System.Drawing namespace to interoperate with these WPF types because they belong to distinct.NET imaging frameworks [1][6][7].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.drawing.graphics.drawimage?view=netframework-4.8.1
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.drawing.graphics.drawimage?view=net-11.0-pp
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.drawing.graphics?view=windowsdesktop-10.0
- 4: https://learn.microsoft.com/en-us/dotnet/desktop/winforms/advanced/drawing-positioning-and-cloning-images-in-gdi
- 5: https://learn.microsoft.com/en-us/dotnet/api/system.drawing.bitmap?view=windowsdesktop-9.0
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.windows.media.imaging.bitmapframe?view=netframework-4.8.1
- 7: https://learn.microsoft.com/en-us/dotnet/api/system.windows.media.imaging.bitmapsource?view=windowsdesktop-9.0
- 8: https://learn.microsoft.com/en-us/archive/msdn-technet-forums/13147707-a9d3-40b9-82e4-290d1c64ccac
- 9: https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.imaging.imageconverter.bitmapfrombitmapsource?view=visualstudiosdk-2022
- 10: https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.imaging.imageconverter?view=visualstudiosdk-2022
Convert $frame to a GDI+ image before calling DrawImage.
$frame is a System.Windows.Media.Imaging.BitmapSource, and Graphics.DrawImage only accepts System.Drawing.Image overloads. Transcode the frame to System.Drawing.Bitmap first, or keep the pipeline entirely in WPF.
🤖 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 `@build/convert-welcome.ps1` around lines 18 - 21, Update the image conversion
flow around `$frame` and `$graphics.DrawImage` so the WPF `BitmapSource` is
converted to a compatible `System.Drawing.Bitmap` or other
`System.Drawing.Image` before drawing. Use the converted image in `DrawImage`,
preserving the existing 500x120 output dimensions and rendering behavior.
| // Update bat file to reference the correct js path and set DESKTOP_APP | ||
| const batContent = fs.readFileSync(batPath, 'utf8'); | ||
| const appExePath = process.execPath; | ||
| const appDir = path.dirname(appExePath); | ||
| // Use Electron's bundled Node runtime | ||
| const nodeExePath = process.platform === 'win32' | ||
| ? path.join(appDir, 'resources', 'app.asar.unpacked', 'node.exe') | ||
| : path.join(appDir, 'Contents', 'MacOS', 'Soterios'); // macOS | ||
|
|
||
| // For Windows packaged builds, Node is typically in the app directory | ||
| const nodeRuntime = process.platform === 'win32' | ||
| ? (fs.existsSync(path.join(appDir, 'node.exe')) ? path.join(appDir, 'node.exe') : 'node') | ||
| : 'node'; | ||
|
|
||
| const updatedBatContent = `@echo off | ||
| REM Soterios Native Messaging Host | ||
| REM This batch file launches the Node.js native host that communicates with the desktop app | ||
|
|
||
| set DESKTOP_APP=${appExePath} | ||
| set NODE_PATH=${path.join(path.dirname(appExePath), 'resources', 'node_modules')} | ||
| "${nodeRuntime}" "${jsPath}" %*`; | ||
| const generatedBatPath = path.join(userDataDir, 'native-host.bat'); | ||
| fs.writeFileSync(generatedBatPath, updatedBatContent); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 '"path"|"allowed_origins"' browser-extension/native-host-manifest.json
rg -n -C3 'asar|asarUnpack|extraResources|browser-extension|node\.exe' package.json
sed -n '1,220p' browser-extension/native-host.jsRepository: chrisriv10/Soterios
Length of output: 4530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant area first
ast-grep outline src/main/ipcHandlers.js --view expanded | sed -n '1,220p'
# Inspect the native-host installer section with line numbers
sed -n '900,990p' src/main/ipcHandlers.js
# Inspect any manifest-writing logic and references to generatedBatPath
rg -n -C3 'generatedBatPath|native-host-manifest|manifest\.path|native-host\.bat|nodeRuntime|nodeExePath|DESKTOP_APP' src/main/ipcHandlers.js browser-extension package.jsonRepository: chrisriv10/Soterios
Length of output: 9450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Re-open the relevant source section with line numbers
cat -n src/main/ipcHandlers.js | sed -n '930,990p'
# Check whether the generated manifest is written anywhere else
rg -n -C4 'native-host-manifest|generatedBatPath|writeFileSync\(.*manifest|manifest\.path|allowed_origins' src/main/ipcHandlers.js browser-extension
# Confirm packaging paths for browser-extension assets
sed -n '40,90p' package.jsonRepository: chrisriv10/Soterios
Length of output: 8343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact installer function around the batch and manifest handling
cat -n src/main/ipcHandlers.js | sed -n '930,1015p'
# Search for any code that edits the manifest path or installs the native host manifest
rg -n -C5 'native-host-manifest|manifest\.path|path"\s*:\s*"native-host\.bat"|register.*native|install.*native|update.*manifest' src/mainRepository: chrisriv10/Soterios
Length of output: 6601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the manifest template and any installation code across the repo
rg -n -C4 'native-host-manifest\.json|native-host\.bat|generatedBatPath|manifest\.path|allowed_origins|com\.soterios\.credential_safety' .
# Inspect the manifest template itself
cat -n browser-extension/native-host-manifest.json
# Inspect the package config around browser-extension packaging
cat -n package.json | sed -n '40,90p'Repository: chrisriv10/Soterios
Length of output: 8076
🌐 Web query:
Chrome native messaging host manifest Windows path relative to manifest directory path field documentation
💡 Result:
In the Chrome native messaging host manifest, the path field defines the location of the native messaging host binary [1][2]. For Windows, this field supports paths that are relative to the directory containing the manifest file itself [1][3]. When a relative path is used, the host process is started with its current directory set to the directory containing the host binary [1][4]. For instance, if the manifest file is located at C:\Apps\manifest.json and the path is set to host.exe, the system treats this as C:\Apps\host.exe and executes it with a working directory of C:\Apps [1][2]. Note that this flexibility is specific to Windows; on Linux and macOS, the path field must always be an absolute path [1][4].
Citations:
- 1: https://developer.chrome.com/docs/extensions/develop/concepts/native-messaging
- 2: https://developer.chrome.com/docs/apps/nativeMessaging
- 3: https://developer.chrome.google.cn/docs/extensions/develop/concepts/native-messaging
- 4: https://learn.microsoft.com/en-us/microsoft-edge/extensions/developer-guide/native-messaging
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the batch template and any install-time copy logic
fd -a 'native-host.bat|native-host-manifest.json' .
rg -n -C3 'native-host\.bat|native-host-manifest\.json|userDataDir|setAppUserModelId|protocol' src browser-extensionRepository: chrisriv10/Soterios
Length of output: 16823
Use nodeExePath here instead of falling back to node on PATH. The current branch still writes node into the batch file when appDir\node.exe is missing, so packaged installs without a system Node runtime won't be able to launch the native host.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 954-954: 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.readFileSync(batPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 975-975: 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(generatedBatPath, updatedBatContent)
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 954 - 976, Update the batch content
construction to use the already resolved nodeExePath variable instead of
nodeRuntime. Remove the PATH-based fallback so packaged installations always
invoke the bundled Node executable selected by the platform-specific logic
above.
… allowlist Prior to this change, all Emergency Lockdown UI strings were untranslated (displayed in English) in Arabic and German locale files, and the newly added allowlist management interface for the feature had no localized strings. This commit adds full translations for all existing Emergency Lockdown UI elements and all new allowlist-related i18n keys, completing localization for the feature in supported locales.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/security/EmergencyLockdown.js (1)
236-281: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftPersist the recovery snapshot before changing system state.
The locked flag and saved network/service snapshots are memory-only. If the app exits or crashes after disabling adapters, the next process reports unlocked and
restore()cannot recover the system. Persist the lockdown marker and snapshots before mutations, reload them in the constructor, and clear them only after a full restore.🤖 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/security/EmergencyLockdown.js` around lines 236 - 281, Update the EmergencyLockdown initialization and lockdown flow to persist the locked marker plus network and service recovery snapshots before calling disableInterface or stopService. Reload this persisted state in the constructor so a new process reports the lockdown and restore() can recover it, and only clear the persisted marker and snapshots after restore completes successfully.
🤖 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 `@src/i18n/locales/pl.json`:
- Around line 208-220: Update the Polish translations for lockdown.description
and lockdown.confirmActivate to replace “usługi niezbędne” with wording meaning
non-essential services, such as “usługi niekrytyczne,” while preserving the rest
of each message.
In `@src/security/EmergencyLockdown.js`:
- Around line 236-249: Update the interface-disabling flow around allowedIPs and
the connected-interface loop so IP allowlisting is enforced: resolve each
interface’s addresses, skip the interface when any address matches allowedIPs,
and record it in results.skippedInterfaces consistently with interface-name
allowlisting. Preserve disabling for interfaces with no matching allowlisted
IPs.
---
Outside diff comments:
In `@src/security/EmergencyLockdown.js`:
- Around line 236-281: Update the EmergencyLockdown initialization and lockdown
flow to persist the locked marker plus network and service recovery snapshots
before calling disableInterface or stopService. Reload this persisted state in
the constructor so a new process reports the lockdown and restore() can recover
it, and only clear the persisted marker and snapshots after restore completes
successfully.
🪄 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: bf4cb235-fc56-4fe6-88a8-231f41b06597
📒 Files selected for processing (19)
src/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/ipcHandlers.jssrc/preload/preload.jssrc/security/EmergencyLockdown.jssrc/ui/js/pages/lockdown.js
🚧 Files skipped from review as they are similar to previous changes (8)
- src/i18n/locales/hi.json
- src/i18n/locales/it.json
- src/i18n/locales/en.json
- src/i18n/locales/fr.json
- src/i18n/locales/ko.json
- src/i18n/locales/ar.json
- src/i18n/locales/nl.json
- src/i18n/locales/es.json
| "lockdown.description": "Natychmiast wyłącz wszystkie interfejsy sieciowe i zatrzymaj usługi niezbędne w sytuacjach awaryjnych.", | ||
| "lockdown.checking": "Sprawdzanie statusu…", | ||
| "lockdown.normal": "Normalna operacja", | ||
| "lockdown.normalDetail": "Wszystkie systemy działają normalnie", | ||
| "lockdown.active": "Lockdown aktywny", | ||
| "lockdown.activeDetail": "Sieć wyłączona i usługi zatrzymane", | ||
| "lockdown.activate": "Aktywuj lockdown", | ||
| "lockdown.restore": "Przywróć systemy", | ||
| "lockdown.activating": "Aktywacja lockdownu…", | ||
| "lockdown.restoring": "Przywracanie systemów…", | ||
| "lockdown.error": "Błąd", | ||
| "lockdown.confirmActivate": "Czy na pewno chcesz aktywować lockdown awaryjny? Spowoduje to wyłączenie wszystkich interfejsów sieciowych i zatrzymanie usług niezbędnych.", | ||
| "lockdown.confirmRestore": "Czy na pewno chcesz przywrócić systemy? Spowoduje to ponowne włączenie interfejsów sieciowych i restart usług.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the “non-essential services” translation.
usługi niezbędne means essential services, so the description and confirmation claim lockdown stops required services. Use wording such as usługi niekrytyczne in both strings.
Proposed fix
- "lockdown.description": "Natychmiast wyłącz wszystkie interfejsy sieciowe i zatrzymaj usługi niezbędne w sytuacjach awaryjnych.",
+ "lockdown.description": "Natychmiast wyłącz wszystkie interfejsy sieciowe i zatrzymaj usługi niekrytyczne w sytuacjach awaryjnych.",
- "lockdown.confirmActivate": "Czy na pewno chcesz aktywować lockdown awaryjny? Spowoduje to wyłączenie wszystkich interfejsów sieciowych i zatrzymanie usług niezbędnych.",
+ "lockdown.confirmActivate": "Czy na pewno chcesz aktywować lockdown awaryjny? Spowoduje to wyłączenie wszystkich interfejsów sieciowych i zatrzymanie usług niekrytycznych.",📝 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.
| "lockdown.description": "Natychmiast wyłącz wszystkie interfejsy sieciowe i zatrzymaj usługi niezbędne w sytuacjach awaryjnych.", | |
| "lockdown.checking": "Sprawdzanie statusu…", | |
| "lockdown.normal": "Normalna operacja", | |
| "lockdown.normalDetail": "Wszystkie systemy działają normalnie", | |
| "lockdown.active": "Lockdown aktywny", | |
| "lockdown.activeDetail": "Sieć wyłączona i usługi zatrzymane", | |
| "lockdown.activate": "Aktywuj lockdown", | |
| "lockdown.restore": "Przywróć systemy", | |
| "lockdown.activating": "Aktywacja lockdownu…", | |
| "lockdown.restoring": "Przywracanie systemów…", | |
| "lockdown.error": "Błąd", | |
| "lockdown.confirmActivate": "Czy na pewno chcesz aktywować lockdown awaryjny? Spowoduje to wyłączenie wszystkich interfejsów sieciowych i zatrzymanie usług niezbędnych.", | |
| "lockdown.confirmRestore": "Czy na pewno chcesz przywrócić systemy? Spowoduje to ponowne włączenie interfejsów sieciowych i restart usług.", | |
| "lockdown.description": "Natychmiast wyłącz wszystkie interfejsy sieciowe i zatrzymaj usługi niekrytyczne w sytuacjach awaryjnych.", | |
| "lockdown.checking": "Sprawdzanie statusu…", | |
| "lockdown.normal": "Normalna operacja", | |
| "lockdown.normalDetail": "Wszystkie systemy działają normalnie", | |
| "lockdown.active": "Lockdown aktywny", | |
| "lockdown.activeDetail": "Sieć wyłączona i usługi zatrzymane", | |
| "lockdown.activate": "Aktywuj lockdown", | |
| "lockdown.restore": "Przywróć systemy", | |
| "lockdown.activating": "Aktywacja lockdownu…", | |
| "lockdown.restoring": "Przywracanie systemów…", | |
| "lockdown.error": "Błąd", | |
| "lockdown.confirmActivate": "Czy na pewno chcesz aktywować lockdown awaryjny? Spowoduje to wyłączenie wszystkich interfejsów sieciowych i zatrzymanie usług niekrytycznych.", | |
| "lockdown.confirmRestore": "Czy na pewno chcesz przywrócić systemy? Spowoduje to ponowne włączenie interfejsów sieciowych i restart usług.", |
🤖 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/pl.json` around lines 208 - 220, Update the Polish
translations for lockdown.description and lockdown.confirmActivate to replace
“usługi niezbędne” with wording meaning non-essential services, such as “usługi
niekrytyczne,” while preserving the rest of each message.
| // Disable all connected network interfaces (respecting allowlist) | ||
| const allowedInterfaces = new Set(this.allowlist.interfaces?.map(i => i.toLowerCase()) || []); | ||
| const allowedIPs = new Set(this.allowlist.ips || []); | ||
|
|
||
| for (const iface of interfaces) { | ||
| if (iface.state === 'connected') { | ||
| // Check if interface is allowlisted | ||
| if (allowedInterfaces.has(iface.name.toLowerCase())) { | ||
| results.skippedInterfaces.push(`${iface.name} (allowlisted)`); | ||
| continue; | ||
| } | ||
|
|
||
| // Check if any IP on this interface is allowlisted | ||
| // For simplicity, we'll skip the interface if user explicitly allowlisted it |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the IP allowlist effective or remove it.
Line 238 creates allowedIPs, but it is never consulted; the UI promises allowlisted IPs remain active while their interface is still disabled. Resolve interface addresses and skip matching interfaces, or remove the IP allowlist option and its claims.
🤖 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/security/EmergencyLockdown.js` around lines 236 - 249, Update the
interface-disabling flow around allowedIPs and the connected-interface loop so
IP allowlisting is enforced: resolve each interface’s addresses, skip the
interface when any address matches allowedIPs, and record it in
results.skippedInterfaces consistently with interface-name allowlisting.
Preserve disabling for interfaces with no matching allowlisted IPs.
Replaces the broken readline-based stdin parser with a proper length-prefixed buffer parser to correctly handle Chrome native messaging format and eliminate listener accumulation. Adds logic to first connect to a running desktop Electron app via named pipe (Windows) or Unix socket (Linux/macOS) before falling back to launching a new instance. Restructures the main loop to sequentially process messages with robust error handling for stream end and parse failures, improving reliability and performance by reusing existing app instances.
2 participants
Summary by CodeRabbit