Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions browser-extension/background.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
chrome.runtime.onInstalled.addListener((details) => {
chrome.runtime.onInstalled.addListener(async (details) => {
if (details.reason === 'install') {
chrome.storage.sync.set({ externalLookupsEnabled: true });
const { externalLookupsEnabled } = await chrome.storage.sync.get('externalLookupsEnabled');
if (externalLookupsEnabled === undefined) {
await chrome.storage.sync.set({ externalLookupsEnabled: true });
}
}
});

Expand All @@ -10,14 +13,6 @@ chrome.runtime.onMessage.addListener((msg, sender, sendResponse) => {
checkPassword(msg.password).then(sendResponse);
return true; // async response
}
if (msg.type === 'CHECK_NATIVE_HOST') {
// Check if native host is connected
const connected = nativePort !== null;
sendResponse({
connected,
error: connected ? null : 'Native host not installed or desktop app not running'
});
}
});

// Native messaging port for desktop app communication
Expand Down
82 changes: 2 additions & 80 deletions browser-extension/content.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,6 @@
let soteriosIcon = null;
let passwordFields = new Map();
let observer = null;
let currentSettings = { showIcon: true, autoCheck: false };

// 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;
}
}
});

function createIcon() {
const icon = document.createElement('img');
Expand Down Expand Up @@ -103,9 +82,6 @@ function removeResult(input) {

function addIconToField(input) {
if (input.dataset.soteriosId) return;

// Check showIcon setting before adding icon
if (!currentSettings.showIcon) return;

const id = `soterios-${Date.now()}-${Math.random().toString(36).slice(2, 9)}`;
input.dataset.soteriosId = id;
Expand All @@ -118,45 +94,7 @@ function addIconToField(input) {
const updatePos = () => positionIcon(icon, input);
window.addEventListener('scroll', updatePos, true);
window.addEventListener('resize', updatePos);

// Store handler references for cleanup
icon._soteriosHandlers = { updatePos, scroll: true, resize: true };

const cleanup = () => {
if (icon._soteriosHandlers) {
if (icon._soteriosHandlers.scroll) {
window.removeEventListener('scroll', icon._soteriosHandlers.updatePos, true);
}
if (icon._soteriosHandlers.resize) {
window.removeEventListener('resize', icon._soteriosHandlers.updatePos);
}
if (icon._soteriosHandlers.autoCheckHandler) {
input.removeEventListener('input', icon._soteriosHandlers.autoCheckHandler);
}
}
icon.remove();
passwordFields.delete(input);
delete input.dataset.soteriosId;
};

input.addEventListener('blur', () => setTimeout(cleanup, 200), { once: true });

// Add autoCheck listener if enabled
if (currentSettings.autoCheck) {
const autoCheckHandler = async () => {
const password = input.value;
if (password && password.length >= 8) {
try {
const result = await chrome.runtime.sendMessage({ type: 'CHECK_PASSWORD', password });
showResult(input, result);
} catch (err) {
console.error('[Soterios] Auto-check failed:', err);
}
}
};
input.addEventListener('input', autoCheckHandler);
icon._soteriosHandlers.autoCheckHandler = autoCheckHandler;
}
input.addEventListener('blur', () => setTimeout(() => icon.remove(), 200), { once: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Icon teardown is incomplete at both cleanup sites, so fields permanently lose their icon.

addIconToField registers a updatePos listener on window for scroll and resize (lines 95-96), stores an entry in passwordFields (line 99), and sets input.dataset.soteriosId (line 87). Both cleanup paths now call only icon.remove(), so none of those three are reversed. One missing teardown routine causes all of the following:

  • The updatePos closure keeps icon and input reachable, so the detached icon never gets collected, and the window listener list grows with every password field.
  • The passwordFields entry retains both nodes for the page lifetime.
  • input.dataset.soteriosId stays set. addIconToField returns early at line 84 on that dataset value, and scanForPasswordFields excludes [data-soterios-id] at line 103. After the first blur, the field never gets an icon again, even when the user focuses it again.

Add one removeIcon(input, icon) helper that removes the listeners, deletes the map entry, deletes dataset.soteriosId, and removes the icon. Call it from both sites.

  • browser-extension/content.js#L97: call the helper from the blur handler instead of icon.remove().
  • browser-extension/content.js#L136: call the helper for each entry instead of icon.remove(), then clear the map.
🐛 Proposed shared teardown helper
 function addIconToField(input) {
@@
   const updatePos = () => positionIcon(icon, input);
   window.addEventListener('scroll', updatePos, true);
   window.addEventListener('resize', updatePos);
-  input.addEventListener('blur', () => setTimeout(() => icon.remove(), 200), { once: true });
+  icon._cleanup = () => {
+    window.removeEventListener('scroll', updatePos, true);
+    window.removeEventListener('resize', updatePos);
+    delete input.dataset.soteriosId;
+    passwordFields.delete(input);
+    icon.remove();
+  };
+  input.addEventListener('blur', () => setTimeout(() => icon._cleanup(), 200), { once: true });
 
   passwordFields.set(input, icon);
 }
-      passwordFields.forEach((icon, input) => icon.remove());
+      passwordFields.forEach((icon) => icon._cleanup());
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
input.addEventListener('blur', () => setTimeout(() => icon.remove(), 200), { once: true });
const updatePos = () => positionIcon(icon, input);
window.addEventListener('scroll', updatePos, true);
window.addEventListener('resize', updatePos);
icon._cleanup = () => {
window.removeEventListener('scroll', updatePos, true);
window.removeEventListener('resize', updatePos);
delete input.dataset.soteriosId;
passwordFields.delete(input);
icon.remove();
};
input.addEventListener('blur', () => setTimeout(() => icon._cleanup(), 200), { once: true });
passwordFields.set(input, icon);
Suggested change
input.addEventListener('blur', () => setTimeout(() => icon.remove(), 200), { once: true });
passwordFields.forEach((icon) => icon._cleanup());
📍 Affects 1 file
  • browser-extension/content.js#L97-L97 (this comment)
  • browser-extension/content.js#L136-L136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@browser-extension/content.js` at line 97, In browser-extension/content.js,
add a shared removeIcon(input, icon) helper that removes the scroll/resize
updatePos listeners, deletes the passwordFields entry and
input.dataset.soteriosId, then removes the icon. At
browser-extension/content.js:97, replace the direct icon removal in the blur
handler with this helper; at browser-extension/content.js:136, use it for each
stored entry before clearing passwordFields.


passwordFields.set(input, icon);
}
Expand All @@ -172,7 +110,6 @@ function init() {
return;
}

loadSettings();
scanForPasswordFields();

observer = new MutationObserver(mutations => {
Expand All @@ -196,22 +133,7 @@ if (typeof window !== 'undefined') {
chrome.runtime.onMessage.addListener((msg, sender, sendResponse) => {
if (msg.type === 'SETTINGS_UPDATED') {
if (!msg.settings.showIcon) {
// Properly clean up all icons and their listeners
passwordFields.forEach((icon, input) => {
if (icon._soteriosHandlers) {
if (icon._soteriosHandlers.scroll) {
window.removeEventListener('scroll', icon._soteriosHandlers.updatePos, true);
}
if (icon._soteriosHandlers.resize) {
window.removeEventListener('resize', icon._soteriosHandlers.updatePos);
}
if (icon._soteriosHandlers.autoCheckHandler) {
input.removeEventListener('input', icon._soteriosHandlers.autoCheckHandler);
}
}
icon.remove();
delete input.dataset.soteriosId;
});
passwordFields.forEach((icon, input) => icon.remove());
passwordFields.clear();
} else {
scanForPasswordFields();
Expand Down
197 changes: 51 additions & 146 deletions browser-extension/native-host.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@
/**
* Soterios Native Messaging Host
* Bridges browser extension <-> desktop Electron app via stdin/stdout JSON messages
* First attempts to connect via named pipe (if app is running), falls back to launching app
*/

const { spawn } = require('child_process');
const readline = require('readline');
const fs = require('fs');
const path = require('path');
const net = require('net');

const DESKTOP_APP = process.env.SOTERIOS_APP_PATH || 'soterios://';

function log(...args) {
console.error('[Soterios Native Host]', new Date().toISOString(), ...args);
Expand All @@ -23,126 +24,67 @@
process.stdout.write(buf);
}

// Persistent stream parser to avoid listener accumulation
let messageBuffer = Buffer.alloc(0);
let messageResolver = null;

function readMessage() {
return new Promise((resolve, reject) => {
messageResolver = { resolve, reject };
tryParseBuffer();
function readMessages() {
const rl = readline.createInterface({
input: process.stdin,
terminal: false
});
}

function tryParseBuffer() {
if (!messageResolver) return;

while (messageBuffer.length >= 4) {
const len = messageBuffer.readUInt32LE(0);
if (messageBuffer.length < 4 + len) break;

const msgBuf = messageBuffer.subarray(4, 4 + len);
messageBuffer = messageBuffer.subarray(4 + len);

try {
const msg = JSON.parse(msgBuf.toString('utf8'));
messageResolver.resolve(msg);
messageResolver = null;
return;
} catch (e) {
messageResolver.reject(new Error(`Failed to parse message: ${e.message}`));
messageResolver = null;
return;
}
}
}

// Set up persistent stdin listener once
process.stdin.on('data', (chunk) => {
messageBuffer = Buffer.concat([messageBuffer, chunk]);
tryParseBuffer();
});
let buffer = Buffer.alloc(0);

process.stdin.on('error', (err) => {
if (messageResolver) {
messageResolver.reject(err);
messageResolver = null;
}
});
process.stdin.on('data', chunk => {
buffer = Buffer.concat([buffer, chunk]);

process.stdin.on('end', () => {
if (messageResolver) {
messageResolver.reject(new Error('Stream ended'));
messageResolver = null;
}
});
while (buffer.length >= 4) {
const len = buffer.readUInt32LE(0);
if (buffer.length < 4 + len) break;

let desktopClient = null;
let desktopProc = null;
const json = buffer.subarray(4, 4 + len).toString();
buffer = buffer.subarray(4 + len);

async function connectToDesktopApp() {
const pipeName = process.platform === 'win32' ? '\\\\.\\pipe\\soterios-credential-safety' : '/tmp/soterios-credential-safety.sock';

return new Promise((resolve, reject) => {
const client = net.createConnection(pipeName, () => {
log('Connected to desktop app via named pipe');
resolve(client);
});

client.on('error', (err) => {
log('Named pipe connection failed:', err.message);
reject(err);
});
try {
const msg = JSON.parse(json);
handleMessage(msg);
} catch (e) {
log('Parse error:', e.message);
}
}
Comment on lines +45 to +51

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Handle rejections from handleMessage to keep the host alive.

handleMessage is async, and line 47 calls it without awaiting it or attaching a catch. The try block only catches JSON.parse errors, because the returned promise settles after the block exits.

handleMessage awaits launchDesktopApp at lines 96 and 105, and that promise rejects when the path is unset or missing. The rejection is then unhandled. Node terminates the process on an unhandled rejection by default. The native host exits, and background.js onDisconnect at line 19 clears nativePort. The extension loses the host on the first CREDENTIAL_LEAK, and the extension receives no error message.

Catch the rejection and report it over the protocol.

🛡️ Proposed fix to report failures instead of exiting
       try {
         const msg = JSON.parse(json);
-        handleMessage(msg);
+        Promise.resolve(handleMessage(msg)).catch(e => {
+          log('Handler error:', e.message);
+          send({ type: 'ERROR', error: e.message, original: msg });
+        });
       } catch (e) {
         log('Parse error:', e.message);
       }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
const msg = JSON.parse(json);
handleMessage(msg);
} catch (e) {
log('Parse error:', e.message);
}
}
try {
const msg = JSON.parse(json);
Promise.resolve(handleMessage(msg)).catch(e => {
log('Handler error:', e.message);
send({ type: 'ERROR', error: e.message, original: msg });
});
} catch (e) {
log('Parse error:', e.message);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@browser-extension/native-host.js` around lines 45 - 51, Update the
message-processing flow around handleMessage to await its promise and catch
asynchronous rejections, not just JSON.parse failures. Report rejected
handleMessage errors through the existing native-host protocol so the host
remains running and the extension receives an error message.

});
}

let desktopProc = null;
const pending = new Map();
let msgId = 0;

function launchDesktopApp() {
if (desktopProc) return Promise.resolve();

return new Promise((resolve, reject) => {
const appPath = process.env.DESKTOP_APP || 'soterios://';

// Check if it's a protocol URL or an executable path
const isProtocolUrl = appPath.startsWith('soterios://') || appPath.startsWith('http://') || appPath.startsWith('https://');

if (isProtocolUrl) {
// Launch using OS-appropriate protocol handler
const isWin = process.platform === 'win32';
const args = isWin ? ['/c', 'start', '', appPath] : ['open', appPath];
const cmd = isWin ? 'cmd' : (process.platform === 'darwin' ? 'open' : 'xdg-open');
const options = { shell: false, detached: true };

desktopProc = spawn(cmd, args, options);
desktopProc.unref();

desktopProc.on('error', e => {
log('Desktop app launch error:', e.message);
desktopProc = null;
});

setTimeout(resolve, 1500);
} else {
// Launch as executable path
const resolvedPath = path.resolve(appPath);
if (!fs.existsSync(resolvedPath)) {
return reject(new Error('Desktop app not found at: ' + resolvedPath));
}
const appPath = process.env.DESKTOP_APP;
if (!appPath) {
return reject(new Error('DESKTOP_APP environment variable not set'));
}
Comment on lines 59 to +66

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

launchDesktopApp reads the wrong environment variable, so it always fails.

Line 12 defines DESKTOP_APP from SOTERIOS_APP_PATH. Line 63 reads process.env.DESKTOP_APP instead, and the DESKTOP_APP constant is never read anywhere. process.env.DESKTOP_APP is a different variable that nothing sets.

The result is that launchDesktopApp rejects with "DESKTOP_APP environment variable not set" on every call. Both CREDENTIAL_LEAK (line 96) and OPEN_APP (line 105) therefore never launch the desktop app.

A native-messaging host inherits only the browser process environment, so SOTERIOS_APP_PATH is also unlikely to be present. The removed protocol-URL fallback means the 'soterios://' default at line 12 is now unreachable. Add a resolved default path, or restore the protocol fallback, so the host works without an environment variable.

🐛 Proposed fix to use the resolved constant
   return new Promise((resolve, reject) => {
-    const appPath = process.env.DESKTOP_APP;
+    const appPath = DESKTOP_APP;
     if (!appPath) {
-      return reject(new Error('DESKTOP_APP environment variable not set'));
+      return reject(new Error('SOTERIOS_APP_PATH environment variable not set'));
     }

DESKTOP_APP still defaults to 'soterios://', which is not a filesystem path and fails the fs.existsSync check at line 70. Either point the default at the installed executable, or handle the protocol form before the path validation.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@browser-extension/native-host.js` around lines 59 - 66, Update
launchDesktopApp to use the module-level DESKTOP_APP constant instead of
process.env.DESKTOP_APP, and ensure its default resolves to a valid installed
executable path or is handled as a protocol URL before fs.existsSync validation.
Preserve the existing rejection behavior only when no usable application target
is available.


// Resolve and validate path - prevent command injection
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 };
const isWin = process.platform === 'win32';
const args = isWin ? ['/c', 'start', '""', resolvedPath] : [resolvedPath];
const cmd = isWin ? 'cmd' : resolvedPath;
const options = { shell: false, detached: true };

desktopProc = spawn(cmd, args, options);
desktopProc.unref();
desktopProc = spawn(cmd, args, options);
desktopProc.unref();
Comment on lines +74 to +80

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Launch the executable directly instead of through cmd /c start on Windows.

The comment at line 68 states the goal is to prevent command injection. shell: false stops Node from invoking a shell, but cmd is then spawned explicitly and parses /c start "" <resolvedPath> with its own rules. That reintroduces a command-execution surface for a path taken from the environment. CodeQL flags line 79 for this reason.

spawn starts a Windows executable directly, so cmd is not needed. Note also that setTimeout(resolve, 1500) at line 87 resolves even after the error handler fires, so callers treat a failed launch as a success.

🔒 Proposed fix to drop the `cmd` indirection
-    const isWin = process.platform === 'win32';
-    const args = isWin ? ['/c', 'start', '""', resolvedPath] : [resolvedPath];
-    const cmd = isWin ? 'cmd' : resolvedPath;
-    const options = { shell: false, detached: true };
-
-    desktopProc = spawn(cmd, args, options);
+    desktopProc = spawn(resolvedPath, [], { shell: false, detached: true, stdio: 'ignore' });
     desktopProc.unref();
 
+    let settled = false;
     desktopProc.on('error', e => {
       log('Desktop app launch error:', e.message);
       desktopProc = null;
+      if (!settled) { settled = true; reject(e); }
     });
 
-    setTimeout(resolve, 1500);
+    setTimeout(() => { if (!settled) { settled = true; resolve(); } }, 1500);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const isWin = process.platform === 'win32';
const args = isWin ? ['/c', 'start', '""', resolvedPath] : [resolvedPath];
const cmd = isWin ? 'cmd' : resolvedPath;
const options = { shell: false, detached: true };
desktopProc = spawn(cmd, args, options);
desktopProc.unref();
desktopProc = spawn(cmd, args, options);
desktopProc.unref();
desktopProc = spawn(resolvedPath, [], { shell: false, detached: true, stdio: 'ignore' });
desktopProc.unref();
let settled = false;
desktopProc.on('error', e => {
log('Desktop app launch error:', e.message);
desktopProc = null;
if (!settled) { settled = true; reject(e); }
});
setTimeout(() => { if (!settled) { settled = true; resolve(); } }, 1500);
🧰 Tools
🪛 GitHub Check: CodeQL

[warning] 79-79: Shell command built from environment values
This shell command depends on an uncontrolled absolute path.
This shell command depends on an uncontrolled absolute path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@browser-extension/native-host.js` around lines 74 - 80, Update the launch
flow around desktopProc and spawn to invoke resolvedPath directly on Windows
instead of using cmd with /c start, while preserving detached execution and
unref behavior. Also update the surrounding completion/error handling so the
launch promise rejects when the spawn error handler fires rather than resolving
unconditionally after the timeout.

Source: Linters/SAST tools


desktopProc.on('error', e => {
log('Desktop app launch error:', e.message);
desktopProc = null;
});
desktopProc.on('error', e => {
log('Desktop app launch error:', e.message);
desktopProc = null;
});

setTimeout(resolve, 1500);
}
setTimeout(resolve, 1500);
});
}

Expand All @@ -151,20 +93,8 @@

switch (msg.type) {
case 'CREDENTIAL_LEAK': {
// Try to connect via named pipe first
try {
if (!desktopClient) {
desktopClient = await connectToDesktopApp();
}
if (desktopClient) {
desktopClient.write(JSON.stringify({ type: 'CREDENTIAL_LEAK', ...msg.payload }) + '\n');
}
send({ type: 'LEAK_NOTIFIED', ok: true, original: msg });
} catch (pipeErr) {
log('Pipe connection failed, launching desktop app:', pipeErr.message);
await launchDesktopApp();
send({ type: 'LEAK_NOTIFIED', ok: true, original: msg });
}
await launchDesktopApp();
send({ type: 'LEAK_NOTIFIED', ok: true, original: msg });
break;
}
case 'PING': {
Expand All @@ -182,29 +112,6 @@
}
}

async function main() {
log('Starting native messaging host');

// Try to connect to desktop app on startup
try {
desktopClient = await connectToDesktopApp();
} catch (e) {
log('Desktop app not running on startup, will launch when needed');
}

while (true) {
try {
const msg = await readMessage();
await handleMessage(msg);
} catch (e) {
if (e.message.includes('Stream ended') || e.message.includes('Unexpected end of JSON')) {
break;
}
log('Error processing message:', e.message);
}
}
}

process.on('uncaughtException', e => {
log('Uncaught:', e);
send({ type: 'ERROR', error: e.message });
Expand All @@ -214,7 +121,5 @@
log('Unhandled rejection:', e);
});

main().catch(e => {
log('Fatal:', e);
process.exit(1);
});
log('Starting native messaging host');
readMessages();
Loading
Loading