refactor(main): split monolithic ipcHandlers.js into six domain modules under src/main/ipc/ - #93
Merged
Merged
Conversation
…es under src/main/ipc/
- Router ipcHandlers.js reduced from ~904 lines to ~63 lines
- New files: _shared.js, scan.js, quarantine.js, process.js, firewall.js, network.js, system.js
- Each domain exports a single register(ipcMain, deps) constructor
- All 60+ IPC channel strings preserved; zero renderer-visible contract changes
- Relies on shared relative requires (../../i18n, ../../security/reportExport, ...)
feat(core): introduce featureFlags single source of truth
- New src/core/featureFlags.js with DEFAULT_FLAGS, getFlag, setFlag, validation
- system.js db:getSetting/db:setSetting route feature.* keys through featureFlags
- main.js replaces direct db.getSetting('feature.*') reads with featureFlags.getFlag
- Unknown feature keys throw descriptive errors in dev builds
- Fixes: feature.systemMonitoring migration was accidentally placed in showNotification runtime path; restored to startup-time migration only
refactor(security): extract scanProgress clamp utility and replace inline clamps
- New src/core/scanProgress.js exports clampProgress(value)
- ScanEngine.js emitProgress uses clampProgress instead of Math.min(100, ...)
- scan.js scheduled scan path also cleaned up
- Phase3 compatibility getter retained because FolderWatcher/scanner.js depend on it
refactor(main): replace remaining raw console.error calls with structured logger
- Reuses existing src/utils/logger.js rather than introducing a duplicate
- Cleans up console.error in src/main/ipc/scan.js and src/main/ipc/network.js
- Zero console.* calls remain in src/main/ outside src/core/logger.js itself
test: update 13 scan-engine tests to match actual state-shape behavior
- scanEngine.test.js: direct top-level engine.isScanning assignments changed
to engine.userScan.isScanning; same for isFolderWatchScanning,
currentScan, abortController; constructor currentScan assertion uses null
- scanCancellation.test.js: scanEngine.abortController scan changed to
userScan.abortController; expected error string 'No scan in progress'
changed to 'No user scan in progress'
- Bounded the folderwatch guard test with Promise.race so it no longer
times out on this runner
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe change introduces shared feature-flag and progress utilities, splits Electron IPC handlers into focused modules, adds scan scheduling and network/firewall operations, expands system/report endpoints, and updates scan-state tests for separate user and folder-watch scans. ChangesIPC and feature integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
src/main/ipc/firewall.js (2)
58-60: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the
countderivation.Line 60 assumes
data.rulesis an array; ifexportRules()ever returns a bare array or omitsrules, the handler throws after the file has already been written, surfacing a false failure to the user.♻️ Suggested tweak
- return { success: true, path: result.filePath, count: data.rules.length }; + const rules = Array.isArray(data) ? data : (data && data.rules) || []; + return { success: true, path: result.filePath, count: rules.length };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/firewall.js` around lines 58 - 60, Update the export handler around exportRules and the success return to derive count safely when data is a bare array or when data.rules is missing, while preserving the existing file-writing behavior and returning the correct number of exported rules without throwing after writeFile succeeds.
15-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
isValidIpaccepts out-of-range octets and duplicatesisValidIPv4innetwork.js.
/^(\d{1,3}\.){3}\d{1,3}$/matches999.1.1.1, so invalid addresses can be persisted intofirewall.trustedIps(Line 99) and sent to the WHOIS endpoint.src/main/ipc/network.js(Lines 7-12) already implements a correct octet-range check — consider promoting a single validator into_shared.jsand reusing it in both modules.♻️ Proposed shared validator usage
-function isValidIp(ip) { - const v4 = /^(\d{1,3}\.){3}\d{1,3}$/; - const v6 = /^[0-9a-fA-F:]+$/; - return v4.test(ip) || (v6.test(ip) && ip.includes(':')); -} +const { isValidIPv4 } = require('./_shared'); + +function isValidIp(ip) { + if (isValidIPv4(ip)) return true; + return typeof ip === 'string' && ip.includes(':') && /^[0-9a-fA-F:]+$/.test(ip); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/firewall.js` around lines 15 - 19, Update isValidIp to reject IPv4 octets outside 0–255 and avoid duplicating the existing validation in isValidIPv4; promote the shared IPv4 validator into _shared.js, reuse it from both firewall.js and network.js, and preserve the current IPv6 validation behavior.src/main/ipc/network.js (2)
194-198: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSerialize/deduplicate bandwidth measurements, and fix the dangling comment reference.
Each call spawns a PowerShell process that compiles C# and sleeps 2s; nothing prevents the renderer from issuing many concurrent
network:measureBandwidthcalls, so a spammed UI button can pile up processes. The comment also points at "measureConnectionBandwidth's comment", but that function carries no explanatory comment — inline the IPv4-only rationale instead.♻️ Suggested in-flight guard
+ const inFlight = new Map(); ipcMain.handle('network:measureBandwidth', async (_event, spec) => { - return measureConnectionBandwidth(spec || {}); + const s = spec || {}; + const key = `${s.localAddress}:${s.localPort}|${s.remoteAddress}:${s.remotePort}`; + if (inFlight.has(key)) return inFlight.get(key); + const p = measureConnectionBandwidth(s).finally(() => inFlight.delete(key)); + inFlight.set(key, p); + return p; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/network.js` around lines 194 - 198, Update the network:measureBandwidth handler and measureConnectionBandwidth flow to serialize measurements and deduplicate concurrent requests so repeated calls share one in-flight operation rather than spawning multiple PowerShell processes. Replace the dangling comment reference with an inline explanation of the IPv4 TCP-only constraint, preserving the existing spec || {} behavior.
33-111: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAdd-Type compiles the P/Invoke shim on every invocation.
Each measurement pays a fresh Roslyn/csc compile plus a 2s sleep. Since the type definition is constant, consider guarding with
if (-not ([System.Management.Automation.PSTypeName]'SoteriosTcpEstats').Type) { Add-Type ... }so repeat calls in one session are cheaper, and confirm the redefinition doesn't error when the type is already loaded in a reused runspace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/network.js` around lines 33 - 111, Guard the constant SoteriosTcpEstats Add-Type definition with a PSTypeName check so compilation occurs only when the type is not already loaded in the reused PowerShell runspace. Keep the existing type definition and measurement flow unchanged, and ensure repeated invocations do not attempt to redefine the loaded type.src/main/ipc/_shared.js (1)
3-21: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the response body and settle on response-stream errors.
bodyaccumulates without limit from a remote host, and there is nores.on('error')/'aborted'handler — if the response stream fails after headers are received, the promise can remain unsettled and the awaiting IPC handler hangs. Also notesetTimeoutis an inactivity timeout, so a slow trickle can extend the call well past 15s.♻️ Suggested hardening
function requestText(url, options = {}) { return new Promise((resolve, reject) => { + const maxBytes = options.maxBytes || 512 * 1024; const req = https.request(url, { method: 'GET', headers: { 'User-Agent': 'Soterios', ...options.headers, }, }, (res) => { let body = ''; + let size = 0; res.setEncoding('utf8'); - res.on('data', chunk => { body += chunk; }); + res.on('data', chunk => { + size += Buffer.byteLength(chunk, 'utf8'); + if (size > maxBytes) { + req.destroy(new Error('Response too large')); + return; + } + body += chunk; + }); + res.on('error', reject); + res.on('aborted', () => reject(new Error('Response aborted'))); res.on('end', () => resolve({ statusCode: res.statusCode, body })); }); req.on('error', reject); req.setTimeout(15000, () => req.destroy(new Error('Request timed out'))); req.end(); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/_shared.js` around lines 3 - 21, Harden requestText by enforcing a maximum response-body size while accumulating chunks, and reject when the response stream emits error or aborted events so the promise always settles after headers are received. Also enforce a true overall 15-second deadline rather than relying solely on req.setTimeout’s inactivity behavior, preserving successful responses within the limit.
🤖 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/core/featureFlags.js`:
- Line 1: Update DEFAULT_FLAGS in featureFlags.js to define autoUpdates with a
default value of true, matching the key consumed by featureFlags.getFlag in
main.js. Also verify whether systemMonitoring is referenced as a flag and add it
to DEFAULT_FLAGS if required.
- Around line 6-19: Add the missing autoUpdates key to the DEFAULT_FLAGS object
in featureFlags.js, using the existing default expected by main.js’s getFlag(db,
'autoUpdates', true) call so the key is recognized without changing other
feature flag defaults.
In `@src/core/scanProgress.js`:
- Around line 6-9: Update clampProgress so conversion of value to a number is
guarded against coercion errors, including Symbol and non-coercible object
inputs. When conversion throws, return 0; preserve the existing finite check and
0–100 rounded clamping for successfully converted values.
In `@src/main/ipc/firewall.js`:
- Around line 96-102: Update the trustConnection handler to normalize the value
returned by db.getSetting(TRUSTED_IPS_KEY, []) to an array before calling
includes or push, falling back to an empty array for corrupted persisted values.
Deduplicate entries using normalized IP comparison, including case-insensitive
handling for IPv6, while preserving the returned and stored trusted-address list
behavior.
- Around line 111-116: Update the network:whois handler to safely parse the
response body from ipwho.is: wrap JSON.parse in a try/catch and return the
existing clean failure result when parsing fails, rather than allowing a
SyntaxError to cross the IPC boundary. Preserve the current handling for valid
JSON and non-200 responses.
- Around line 34-44: Update the firewall:setRuleEnabled IPC handler to use a
safe fallback before destructuring its payload, so null or undefined input does
not cause a TypeError. Preserve passing the resulting name and enabled values to
FirewallManager.setRuleEnabled, allowing it to perform rule-name validation.
In `@src/main/ipc/network.js`:
- Around line 178-188: Import the featureFlags module from
../../core/featureFlags before the network:geo handler uses
featureFlags.getFlag, ensuring the handler no longer raises a ReferenceError.
- Around line 171-176: Guard the progress callback inside the
network:connections handler before calling event.sender.send, checking that the
sender’s webContents is still alive and skipping the update when it has been
destroyed. Keep networkEnricher.enrich’s existing progress behavior unchanged
while preventing sends after the window closes.
In `@src/main/ipc/system.js`:
- Around line 342-351: Update the health:score handler to include lastScanDate
from latest.timestamp in the health-score tool input, matching
getTrayHealthSummary. Prefer reusing getTrayHealthSummary or a shared input
builder if appropriate, while preserving the existing passwordScore and
lastScanMatches values.
- Around line 68-90: The db:getSetting and db:setSetting handlers must remove
the leading “feature.” prefix before calling featureFlags.getFlag and
featureFlags.setFlag. Preserve the existing full key for raw db.getSetting
fallback and unknown-flag errors, while continuing to use normal database access
for non-feature keys.
In `@src/main/ipcHandlers.js`:
- Around line 55-60: Add IPC registrations for folderwatch:status,
folderwatch:toggle, rtp:status, and rtp:toggle in the main handler setup
alongside registerScan and the other register* calls, using the existing
folder-watch and RTP handler implementations/services. Ensure each UI-invoked
channel is registered via ipcMain.handle so calls no longer reject at runtime.
In `@tests/scanCancellation.test.js`:
- Around line 119-124: Update the folderwatch cancellation test around
pending/runScan and abortScan to assert the folderwatch state immediately after
starting pending, relying on runScan()’s synchronous state update. Remove the
timeout-based wait so a fast scan cannot finish before abortScan(), while
preserving the expected “No user scan in progress” result.
---
Nitpick comments:
In `@src/main/ipc/_shared.js`:
- Around line 3-21: Harden requestText by enforcing a maximum response-body size
while accumulating chunks, and reject when the response stream emits error or
aborted events so the promise always settles after headers are received. Also
enforce a true overall 15-second deadline rather than relying solely on
req.setTimeout’s inactivity behavior, preserving successful responses within the
limit.
In `@src/main/ipc/firewall.js`:
- Around line 58-60: Update the export handler around exportRules and the
success return to derive count safely when data is a bare array or when
data.rules is missing, while preserving the existing file-writing behavior and
returning the correct number of exported rules without throwing after writeFile
succeeds.
- Around line 15-19: Update isValidIp to reject IPv4 octets outside 0–255 and
avoid duplicating the existing validation in isValidIPv4; promote the shared
IPv4 validator into _shared.js, reuse it from both firewall.js and network.js,
and preserve the current IPv6 validation behavior.
In `@src/main/ipc/network.js`:
- Around line 194-198: Update the network:measureBandwidth handler and
measureConnectionBandwidth flow to serialize measurements and deduplicate
concurrent requests so repeated calls share one in-flight operation rather than
spawning multiple PowerShell processes. Replace the dangling comment reference
with an inline explanation of the IPv4 TCP-only constraint, preserving the
existing spec || {} behavior.
- Around line 33-111: Guard the constant SoteriosTcpEstats Add-Type definition
with a PSTypeName check so compilation occurs only when the type is not already
loaded in the reused PowerShell runspace. Keep the existing type definition and
measurement flow unchanged, and ensure repeated invocations do not attempt to
redefine the loaded type.
🪄 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: 69a300bf-9aaf-4de8-a074-edc013990af2
📒 Files selected for processing (14)
src/core/featureFlags.jssrc/core/scanProgress.jssrc/main/ipc/_shared.jssrc/main/ipc/firewall.jssrc/main/ipc/network.jssrc/main/ipc/process.jssrc/main/ipc/quarantine.jssrc/main/ipc/scan.jssrc/main/ipc/system.jssrc/main/ipcHandlers.jssrc/main/main.jssrc/security/ScanEngine.jstests/scanCancellation.test.jstests/scanEngine.test.js
| @@ -0,0 +1,58 @@ | |||
| // src/core/featureFlags.js | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Missing autoUpdates flag definition breaks the auto-update check. DEFAULT_FLAGS in src/core/featureFlags.js enumerates 12 flags but omits autoUpdates, which src/main/main.js relies on; since getFlag throws for any key absent from DEFAULT_FLAGS, the consumer call always fails.
src/core/featureFlags.js#L6-19: addautoUpdates: true(and confirm whethersystemMonitoring, referenced in the PR description, also needs an entry) toDEFAULT_FLAGS.src/main/main.js#L727-727: no change needed here once the flag is defined — this call site will start working correctly againstfeatureFlags.getFlag(db, 'autoUpdates', 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/core/featureFlags.js` at line 1, Update DEFAULT_FLAGS in featureFlags.js
to define autoUpdates with a default value of true, matching the key consumed by
featureFlags.getFlag in main.js. Also verify whether systemMonitoring is
referenced as a flag and add it to DEFAULT_FLAGS if required.
Comment on lines
+6
to
+19
| const DEFAULT_FLAGS = Object.freeze({ | ||
| realtimeProtection: true, | ||
| autoReports: true, | ||
| scanHistory: true, | ||
| externalLookups: true, | ||
| geoLookup: true, | ||
| networkPerimeterMap: true, | ||
| notificationsEnabled: true, | ||
| scanNotifications: true, | ||
| launchAtStartup: false, | ||
| folderWatch: true, | ||
| networkAlerts: true, | ||
| networkTrafficHistory: true, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm all featureFlags.getFlag/setFlag call sites use keys present in DEFAULT_FLAGS
rg -nP "featureFlags\.(getFlag|setFlag)\(" -A1
rg -nP "feature\.systemMonitoring"Repository: chrisriv10/Soterios
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== featureFlags file ==\n'
if [ -f src/core/featureFlags.js ]; then
wc -l src/core/featureFlags.js
cat -n src/core/featureFlags.js | sed -n '1,220p'
else
echo "src/core/featureFlags.js not found"
fi
printf '\n== search autoUpdates/systemMonitoring/featureFlags call sites ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
-e 'autoUpdates' -e 'systemMonitoring' -e 'featureFlags\.(getFlag|setFlag)\(' -e 'getFlag\(db, '\''autoUpdates'\''' -e 'feature\.autoUpdates' -e 'feature\.systemMonitoring' .Repository: chrisriv10/Soterios
Length of output: 7563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== relevant files ==\n'
git ls-files 'src/**' | rg 'featureFlags|main\.js|systemMonitoring|autoUpdates'
printf '\n== feature flag references ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
-e 'autoUpdates' -e 'systemMonitoring' -e 'featureFlags' srcRepository: chrisriv10/Soterios
Length of output: 5958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '700,740p' src/main/main.js | nl -baRepository: chrisriv10/Soterios
Length of output: 197
Add autoUpdates to DEFAULT_FLAGS — src/main/main.js:727 calls featureFlags.getFlag(db, 'autoUpdates', true), and getFlag() throws on unknown keys.
Suggested change
const DEFAULT_FLAGS = Object.freeze({
realtimeProtection: true,
autoReports: true,
scanHistory: true,
externalLookups: true,
geoLookup: true,
networkPerimeterMap: true,
notificationsEnabled: true,
scanNotifications: true,
launchAtStartup: false,
folderWatch: true,
networkAlerts: true,
networkTrafficHistory: true,
+ autoUpdates: true,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const DEFAULT_FLAGS = Object.freeze({ | |
| realtimeProtection: true, | |
| autoReports: true, | |
| scanHistory: true, | |
| externalLookups: true, | |
| geoLookup: true, | |
| networkPerimeterMap: true, | |
| notificationsEnabled: true, | |
| scanNotifications: true, | |
| launchAtStartup: false, | |
| folderWatch: true, | |
| networkAlerts: true, | |
| networkTrafficHistory: true, | |
| }); | |
| const DEFAULT_FLAGS = Object.freeze({ | |
| realtimeProtection: true, | |
| autoReports: true, | |
| scanHistory: true, | |
| externalLookups: true, | |
| geoLookup: true, | |
| networkPerimeterMap: true, | |
| notificationsEnabled: true, | |
| scanNotifications: true, | |
| launchAtStartup: false, | |
| folderWatch: true, | |
| networkAlerts: true, | |
| networkTrafficHistory: true, | |
| autoUpdates: 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/core/featureFlags.js` around lines 6 - 19, Add the missing autoUpdates
key to the DEFAULT_FLAGS object in featureFlags.js, using the existing default
expected by main.js’s getFlag(db, 'autoUpdates', true) call so the key is
recognized without changing other feature flag defaults.
Comment on lines
+6
to
+9
| function clampProgress(value) { | ||
| const n = Number(value); | ||
| if (!Number.isFinite(n)) return 0; | ||
| return Math.max(0, Math.min(100, Math.round(n))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep progress normalization non-throwing.
Number(value) can throw for Symbol values and non-coercible objects, so malformed progress input can escape instead of normalizing to 0.
Proposed fix
function clampProgress(value) {
- const n = Number(value);
+ let n;
+ try {
+ n = Number(value);
+ } catch (_) {
+ return 0;
+ }
if (!Number.isFinite(n)) return 0;
return Math.max(0, Math.min(100, Math.round(n)));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function clampProgress(value) { | |
| const n = Number(value); | |
| if (!Number.isFinite(n)) return 0; | |
| return Math.max(0, Math.min(100, Math.round(n))); | |
| function clampProgress(value) { | |
| let n; | |
| try { | |
| n = Number(value); | |
| } catch (_) { | |
| return 0; | |
| } | |
| if (!Number.isFinite(n)) return 0; | |
| return Math.max(0, Math.min(100, Math.round(n))); | |
| } |
🤖 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/core/scanProgress.js` around lines 6 - 9, Update clampProgress so
conversion of value to a number is guarded against coercion errors, including
Symbol and non-coercible object inputs. When conversion throws, return 0;
preserve the existing finite check and 0–100 rounded clamping for successfully
converted values.
Comment on lines
+34
to
+44
| ipcMain.handle('firewall:createRule', async (_event, spec) => { | ||
| return firewallManager.createRule(spec); | ||
| }); | ||
|
|
||
| ipcMain.handle('firewall:deleteRule', async (_event, name) => { | ||
| return firewallManager.deleteRule(name); | ||
| }); | ||
|
|
||
| ipcMain.handle('firewall:setRuleEnabled', async (_event, { name, enabled }) => { | ||
| return firewallManager.setRuleEnabled(name, enabled); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'FirewallManager' --exec ast-grep outline {} --items all
rg -nP '\b(createRule|deleteRule|setRuleEnabled)\s*\(' -C6 --iglob '*firewall*'Repository: chrisriv10/Soterios
Length of output: 715
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/main/ipc/firewall.js ---'
cat -n src/main/ipc/firewall.js | sed -n '1,120p'
echo
echo '--- src/security/FirewallManager.js (outline) ---'
ast-grep outline src/security/FirewallManager.js --items all
echo
echo '--- src/security/FirewallManager.js (relevant methods) ---'
rg -n "class FirewallManager|createRule|deleteRule|setRuleEnabled|setProfileEnabled|friendlyFirewallError|_validateImportRule|isValidIp|psEscape" src/security/FirewallManager.js -n -C 6Repository: chrisriv10/Soterios
Length of output: 14930
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' src/security/FirewallManager.js | cat -nRepository: chrisriv10/Soterios
Length of output: 13046
Guard firewall:setRuleEnabled before destructuring. null/undefined from the renderer will throw a raw TypeError here; use a payload fallback and let FirewallManager handle the rule-name check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc/firewall.js` around lines 34 - 44, Update the
firewall:setRuleEnabled IPC handler to use a safe fallback before destructuring
its payload, so null or undefined input does not cause a TypeError. Preserve
passing the resulting name and enabled values to FirewallManager.setRuleEnabled,
allowing it to perform rule-name validation.
Source: Linters/SAST tools
Comment on lines
+96
to
+102
| ipcMain.handle('firewall:trustConnection', (_event, ip) => { | ||
| if (!ip || !isValidIp(ip)) throw new Error('Invalid address.'); | ||
| const current = db.getSetting(TRUSTED_IPS_KEY, []); | ||
| if (!current.includes(ip)) current.push(ip); | ||
| db.setSetting(TRUSTED_IPS_KEY, current); | ||
| return current; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Normalize the stored value to an array.
untrustConnection (Line 105) defends with || [], but trustConnection calls .includes/.push directly on whatever getSetting returns; a corrupted/non-array persisted value throws here. Also worth deduplicating and normalizing case for IPv6.
🛡️ Proposed fix
- const current = db.getSetting(TRUSTED_IPS_KEY, []);
+ const stored = db.getSetting(TRUSTED_IPS_KEY, []);
+ const current = Array.isArray(stored) ? [...stored] : [];
if (!current.includes(ip)) current.push(ip);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ipcMain.handle('firewall:trustConnection', (_event, ip) => { | |
| if (!ip || !isValidIp(ip)) throw new Error('Invalid address.'); | |
| const current = db.getSetting(TRUSTED_IPS_KEY, []); | |
| if (!current.includes(ip)) current.push(ip); | |
| db.setSetting(TRUSTED_IPS_KEY, current); | |
| return current; | |
| }); | |
| ipcMain.handle('firewall:trustConnection', (_event, ip) => { | |
| if (!ip || !isValidIp(ip)) throw new Error('Invalid address.'); | |
| const stored = db.getSetting(TRUSTED_IPS_KEY, []); | |
| const current = Array.isArray(stored) ? [...stored] : []; | |
| if (!current.includes(ip)) current.push(ip); | |
| db.setSetting(TRUSTED_IPS_KEY, current); | |
| return current; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc/firewall.js` around lines 96 - 102, Update the trustConnection
handler to normalize the value returned by db.getSetting(TRUSTED_IPS_KEY, []) to
an array before calling includes or push, falling back to an empty array for
corrupted persisted values. Deduplicate entries using normalized IP comparison,
including case-insensitive handling for IPv6, while preserving the returned and
stored trusted-address list behavior.
Comment on lines
+178
to
+188
| ipcMain.handle('network:geo', async (_event, ips) => { | ||
| if (!featureFlags.getFlag(db, 'geoLookup', true)) return {}; | ||
| const results = {}; | ||
| for (const ip of ips) { | ||
| const geo = await geoLocationService.lookup(ip); | ||
| if (geo) { | ||
| results[ip] = geo; | ||
| } | ||
| } | ||
| return results; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP "require\(.*featureFlags.*\)|\bfeatureFlags\b" src --type=js -C2
fd -t f 'featureFlags.js' src --exec ast-grep outline {} --items allRepository: chrisriv10/Soterios
Length of output: 6012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/main/ipc/network.js (top + geo handler) =="
sed -n '1,260p' src/main/ipc/network.js | cat -n | sed -n '1,120p'
echo
echo "== Search for network:geo callers =="
rg -n "network:geo" src -C 2
echo
echo "== Search for ips argument validation around geo IPC =="
rg -n "geo.*ips|ips.*geo|Array\.isArray\(ips\)|for \(const ip of ips\)" src/main src/renderer -C 2Repository: chrisriv10/Soterios
Length of output: 6765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== featureFlags references in src/main/ipc/network.js =="
rg -n "featureFlags" src/main/ipc/network.js -C 2
echo
echo "== network geo caller shape =="
sed -n '292,330p' src/ui/js/pages/network.js | cat -nRepository: chrisriv10/Soterios
Length of output: 2455
Import featureFlags before using it here.
network:geo will throw a ReferenceError on every call until require('../../core/featureFlags') is added.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc/network.js` around lines 178 - 188, Import the featureFlags
module from ../../core/featureFlags before the network:geo handler uses
featureFlags.getFlag, ensuring the handler no longer raises a ReferenceError.
Source: Linters/SAST tools
Comment on lines
+68
to
+90
| ipcMain.handle('db:getSetting', (_event, key, def) => { | ||
| if (typeof key === 'string' && key.startsWith('feature.')) { | ||
| try { | ||
| return featureFlags.getFlag(db, key, def); | ||
| } catch (_) { | ||
| // Unknown feature flag; fall through to raw DB read so we don't | ||
| // break unknown keys used during feature-flag migration. | ||
| return db.getSetting(key, def); | ||
| } | ||
| } | ||
| return db.getSetting(key, def); | ||
| }); | ||
|
|
||
| ipcMain.handle('db:setSetting', (_event, key, value) => { | ||
| if (typeof key === 'string' && key.startsWith('feature.')) { | ||
| try { | ||
| return featureFlags.setFlag(db, key, value); | ||
| } catch (_) { | ||
| throw new Error(`Unknown feature flag: ${key}`); | ||
| } | ||
| } | ||
| return db.setSetting(key, value); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check renderer/preload callers of db:setSetting for the 'feature.' key convention
rg -nP "db:setSetting|db:getSetting" -C2Repository: chrisriv10/Soterios
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- outline: src/main/ipc/system.js ---'
ast-grep outline src/main/ipc/system.js --view expanded || true
echo
echo '--- outline: src/core/featureFlags.js ---'
ast-grep outline src/core/featureFlags.js --view expanded || true
echo
echo '--- search for feature.* and db:setSetting/db:getSetting callers ---'
rg -n "feature\.[A-Za-z0-9_]+|db:setSetting|db:getSetting|setFlag\(|getFlag\(" src -SRepository: chrisriv10/Soterios
Length of output: 9446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/core/featureFlags.js ---'
cat -n src/core/featureFlags.js
echo
echo '--- src/main/ipc/system.js (lines 60-95) ---'
sed -n '60,95p' src/main/ipc/system.js
echo
echo '--- src/ui/js/api.js (feature settings IPC usage) ---'
sed -n '70,180p' src/ui/js/api.jsRepository: chrisriv10/Soterios
Length of output: 10371
Strip feature. before calling featureFlags.*
featureFlags.getFlag/setFlag expect bare keys, so passing feature.realtimeProtection makes every feature.* read fall back and every write throw. That breaks persistence for the feature toggles in Settings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc/system.js` around lines 68 - 90, The db:getSetting and
db:setSetting handlers must remove the leading “feature.” prefix before calling
featureFlags.getFlag and featureFlags.setFlag. Preserve the existing full key
for raw db.getSetting fallback and unknown-flag errors, while continuing to use
normal database access for non-feature keys.
Comment on lines
+342
to
+351
| ipcMain.handle('health:score', async () => { | ||
| const latest = db.getLatestScanReport(); | ||
| const passwordScore = db.getSetting('feature.lastPasswordScore', null); | ||
| const result = await toolRegistry.run('health-score', { | ||
| lastScanMatches: latest ? latest.threats_found : null, | ||
| passwordScore: passwordScore === null ? null : Number(passwordScore), | ||
| }, { db }); | ||
| if (!result.ok) throw new Error(result.error || 'Unable to calculate health score'); | ||
| return result.data; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
health:score omits lastScanDate, diverging from getTrayHealthSummary.
src/main/healthSummary.js (Lines 8-26) builds the health-score tool input with lastScanDate: latest ? latest.timestamp : null, but this handler omits that field entirely when computing the same score for the dashboard. This can produce a different score than the tray widget for the same underlying data. Consider reusing getTrayHealthSummary (already imported) or extracting a shared input-builder to keep both call sites in sync.
♻️ Proposed fix
ipcMain.handle('health:score', async () => {
const latest = db.getLatestScanReport();
const passwordScore = db.getSetting('feature.lastPasswordScore', null);
const result = await toolRegistry.run('health-score', {
lastScanMatches: latest ? latest.threats_found : null,
+ lastScanDate: latest ? latest.timestamp : null,
passwordScore: passwordScore === null ? null : Number(passwordScore),
}, { db });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ipcMain.handle('health:score', async () => { | |
| const latest = db.getLatestScanReport(); | |
| const passwordScore = db.getSetting('feature.lastPasswordScore', null); | |
| const result = await toolRegistry.run('health-score', { | |
| lastScanMatches: latest ? latest.threats_found : null, | |
| passwordScore: passwordScore === null ? null : Number(passwordScore), | |
| }, { db }); | |
| if (!result.ok) throw new Error(result.error || 'Unable to calculate health score'); | |
| return result.data; | |
| }); | |
| ipcMain.handle('health:score', async () => { | |
| const latest = db.getLatestScanReport(); | |
| const passwordScore = db.getSetting('feature.lastPasswordScore', null); | |
| const result = await toolRegistry.run('health-score', { | |
| lastScanMatches: latest ? latest.threats_found : null, | |
| lastScanDate: latest ? latest.timestamp : null, | |
| passwordScore: passwordScore === null ? null : Number(passwordScore), | |
| }, { db }); | |
| if (!result.ok) throw new Error(result.error || 'Unable to calculate health score'); | |
| return result.data; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc/system.js` around lines 342 - 351, Update the health:score
handler to include lastScanDate from latest.timestamp in the health-score tool
input, matching getTrayHealthSummary. Prefer reusing getTrayHealthSummary or a
shared input builder if appropriate, while preserving the existing passwordScore
and lastScanMatches values.
Comment on lines
+55
to
+60
| registerScan(mainWindow, servicesForScan); | ||
| registerQuarantine(mainWindow, servicesForQuarantine); | ||
| registerProcess(mainWindow, servicesForProcess); | ||
| registerFirewall(mainWindow, servicesForFirewall); | ||
| registerNetwork(mainWindow, servicesForNetwork); | ||
| registerSystem(mainWindow, servicesForSystem); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Registered handlers in main
rg -nP --no-heading -o "ipcMain\.handle\(\s*'([^']+)'" -r '$1' src/main | sort -u -t: -k3 > /tmp/registered.txt
rg -nP --no-heading -o "ipcMain\.handle\(\s*'([^']+)'" -r '$1' src/main | awk -F: '{print $3}' | sort -u > /tmp/handlers.txt
# Channels invoked from renderer/preload
rg -nP --no-heading -o "invoke\(\s*'([^']+)'" -r '$1' src/ui src/preload* 2>/dev/null | awk -F: '{print $NF}' | sort -u > /tmp/invoked.txt
echo "== invoked but not handled =="
comm -23 /tmp/invoked.txt /tmp/handlers.txt
echo "== handled but never invoked =="
comm -13 /tmp/invoked.txt /tmp/handlers.txtRepository: chrisriv10/Soterios
Length of output: 1098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the IPC handler modules and the renderer/preload invoke sites.
echo "== files =="
git ls-files 'src/main/**/*.js' 'src/preload*' 'src/ui/**/*.js' | sed -n '1,200p'
echo
echo "== ipcMain.handle registrations =="
rg -n --no-heading "ipcMain\.handle\(" src/main
echo
echo "== ipcRenderer.invoke / invoke wrappers =="
rg -n --no-heading "(ipcRenderer\.invoke|\.invoke\()" src/preload src/ui src/main 2>/dev/null | sed -n '1,240p'Repository: chrisriv10/Soterios
Length of output: 28392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the split file and the modules it delegates to.
for f in src/main/ipcHandlers.js src/main/ipcHandlers/*.js; do
if [ -f "$f" ]; then
echo "== $f =="
wc -l "$f"
fi
done
echo
echo "== outline src/main/ipcHandlers.js =="
ast-grep outline src/main/ipcHandlers.js --view expanded
echo
echo "== first 120 lines of src/main/ipcHandlers.js =="
cat -n src/main/ipcHandlers.js | sed -n '1,140p'Repository: chrisriv10/Soterios
Length of output: 3057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact split entrypoint and nearby logic.
rg -n --no-heading "registerScan|registerQuarantine|registerProcess|registerFirewall|registerNetwork|registerSystem|ipcMain\.handle\(" src/main/ipcHandlers.js src/main -g '*.js' | sed -n '1,260p'Repository: chrisriv10/Soterios
Length of output: 10336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
import subprocess
from pathlib import Path
def extract_handlers(text):
return sorted(set(re.findall(r"ipcMain\.handle\(\s*'([^']+)'", text)))
# Current split files
paths = [
"src/main/ipc/scan.js",
"src/main/ipc/quarantine.js",
"src/main/ipc/process.js",
"src/main/ipc/firewall.js",
"src/main/ipc/network.js",
"src/main/ipc/system.js",
]
current = []
for p in paths:
current.extend(extract_handlers(Path(p).read_text()))
current = sorted(set(current))
# Previous monolithic file (parent commit)
prev = subprocess.check_output(
["git", "show", "HEAD^:src/main/ipcHandlers.js"],
text=True,
stderr=subprocess.DEVNULL,
)
previous = extract_handlers(prev)
print("previous_count", len(previous))
print("current_count", len(current))
print("missing_from_current")
for x in sorted(set(previous) - set(current)):
print(x)
print("new_in_current")
for x in sorted(set(current) - set(previous)):
print(x)
PYRepository: chrisriv10/Soterios
Length of output: 288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
# Extract string-literal invokes from preload + ui
invoke_pattern = re.compile(r"""(?:ipcRenderer\.invoke|window\.api\.invoke|\.invoke)\(\s*'([^']+)'""")
paths = [p for p in Path("src").rglob("*.js") if "node_modules" not in p.parts]
invoked = set()
for p in paths:
text = p.read_text(errors="ignore")
invoked.update(invoke_pattern.findall(text))
# Extract registered handlers from main/ipc modules and main/main.js
handle_pattern = re.compile(r"""ipcMain\.handle\(\s*'([^']+)'""")
handled = set()
for p in Path("src/main").rglob("*.js"):
text = p.read_text(errors="ignore")
handled.update(handle_pattern.findall(text))
print("invoked_count", len(invoked))
print("handled_count", len(handled))
print("invoked_without_handler")
for x in sorted(invoked - handled):
print(x)
print("handled_without_invocation")
for x in sorted(handled - invoked):
print(x)
PYRepository: chrisriv10/Soterios
Length of output: 477
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== references to folderwatch / rtp =="
rg -n --no-heading "(folderwatch|rtp):" src
echo
echo "== ipcMain.handle registrations for those channels =="
rg -n --no-heading "ipcMain\.handle\('(folderwatch|rtp):" src/main
echo
echo "== relevant IPC setup in src/main/main.js =="
rg -n --no-heading "(folderwatch|rtp):|registerIpcHandlers|registerScan|registerSystem" src/main/main.js src/main/ipc/*.jsRepository: chrisriv10/Soterios
Length of output: 915
Restore the missing IPC handlers. folderwatch:status, folderwatch:toggle, rtp:status, and rtp:toggle no longer have any ipcMain.handle(...) registration, so the UI calls now reject at runtime.
🤖 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 55 - 60, Add IPC registrations for
folderwatch:status, folderwatch:toggle, rtp:status, and rtp:toggle in the main
handler setup alongside registerScan and the other register* calls, using the
existing folder-watch and RTP handler implementations/services. Ensure each
UI-invoked channel is registered via ipcMain.handle so calls no longer reject at
runtime.
Comment on lines
+119
to
124
| // Give the manager one tick to register the folderwatch state; do not | ||
| // block on isScanning because waitFor's poll interval interacts badly | ||
| // with the test harness timeout on this machine. | ||
| await new Promise((resolve) => setTimeout(resolve, 5)); | ||
| const blocked = scanEngine.abortScan(); | ||
| assert.deepEqual(blocked, { success: false, canceled: false, error: 'No user scan in progress' }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert folderwatch state instead of waiting by time.
Line 122 can let a fast scan finish before abortScan(), so this no longer proves the folderwatch guard. Assert the state immediately after starting pending; runScan() sets it synchronously.
Proposed fix
- // Give the manager one tick to register the folderwatch state; do not
- // block on isScanning because waitFor's poll interval interacts badly
- // with the test harness timeout on this machine.
- await new Promise((resolve) => setTimeout(resolve, 5));
+ assert.equal(scanEngine.isFolderWatchScanning, true);
const blocked = scanEngine.abortScan();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Give the manager one tick to register the folderwatch state; do not | |
| // block on isScanning because waitFor's poll interval interacts badly | |
| // with the test harness timeout on this machine. | |
| await new Promise((resolve) => setTimeout(resolve, 5)); | |
| const blocked = scanEngine.abortScan(); | |
| assert.deepEqual(blocked, { success: false, canceled: false, error: 'No user scan in progress' }); | |
| assert.equal(scanEngine.isFolderWatchScanning, true); | |
| const blocked = scanEngine.abortScan(); | |
| assert.deepEqual(blocked, { success: false, canceled: false, error: 'No user scan in progress' }); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 121-121: Avoid using the initial state variable in setState
Context: setTimeout(resolve, 5)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 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 `@tests/scanCancellation.test.js` around lines 119 - 124, Update the
folderwatch cancellation test around pending/runScan and abortScan to assert the
folderwatch state immediately after starting pending, relying on runScan()’s
synchronous state update. Remove the timeout-based wait so a fast scan cannot
finish before abortScan(), while preserving the expected “No user scan in
progress” result.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(core): introduce featureFlags single source of truth
refactor(security): extract scanProgress clamp utility and replace inline clamps
refactor(main): replace remaining raw console.error calls with structured logger
test: update 13 scan-engine tests to match actual state-shape behavior
to engine.userScan.isScanning; same for isFolderWatchScanning,
currentScan, abortController; constructor currentScan assertion uses null
userScan.abortController; expected error string 'No scan in progress'
changed to 'No user scan in progress'
times out on this runner
Summary by CodeRabbit
New Features
Bug Fixes