From 50e445e7c3eef302fae48c18cb1d1451ae0dc255 Mon Sep 17 00:00:00 2001 From: Twest2 Date: Fri, 3 Jul 2026 11:25:36 -0700 Subject: [PATCH] Close the renderer privilege boundary: navigation, IPC, permissions, shell Phase 1 of the improvement plan (PR 2 of the sequence). A remote page could previously inherit the privileged preload bridge: the main window had no navigation guard or popup handler, IPC handlers accepted any sender, every Electron permission was granted to everyone, and shell:openPath accepted an arbitrary renderer-supplied target. - New app/security.js (plain-Node testable) centralizes the policy: app-page identity, navigation/popup denial, deny-by-default permissions, external-URL validation, IPC sender guard, payload budgets, field validators, and a produced-files registry. - Every window (main, region overlay, capture worker) is sandboxed, denies all navigation away from its own page, denies every popup, and refuses webview attachment. - Every IPC channel now rejects events that are not from the main window's top frame on index.html, rejects non-plain/oversized argument bags, and channels with risky inputs validate ids, enums, names, and sizes (path-traversal and prototype-pollution guards included). - Permissions are deny-by-default; the only grant in the app is display capture (+media) for the dedicated capture-worker page. The display-media handler also verifies the requesting frame. - shell:openPath/showItemInFolder are gone. Replacements are intent-specific: openProduced (only files the main process produced this session), revealLinkedArchive (path read from the store, not the renderer), and openExternal (parsed, scheme-checked http(s)/mailto only). - archive:peek removed: unused, and it let the renderer read arbitrary archives by path. - export:run only accepts output directories that came from a dialog pick or remembered settings; anything else re-prompts. - Renderer: description links never navigate; http(s)/mailto open externally via the validated handler. - 12 new security regression tests: hostile navigation targets, permission matrix, sender spoofing (wrong window/subframe/navigated/disposed frames), oversized payloads, traversal/pollution attempts, produced-file registry, and source-level guards (no blanket grants, no generic shell channels, sandbox on every window). Verified: 215 unit tests pass; startup smoke, unit-workflows, sample artifacts, and build-release E2E pass; click self-test still reaches source: stream with markers 3/3 and burst 8/8 under the deny-by-default policy (arm/debounce remain the known pre-existing failures, untouched here); UI screenshot confirms the sandboxed renderer boots. Co-Authored-By: Claude Fable 5 --- app/capture.js | 4 + app/main.js | 252 +++++++++++++++++++++++------- app/preload.js | 7 +- app/renderer/app.js | 18 +++ app/renderer/editor.js | 4 +- app/security.js | 302 ++++++++++++++++++++++++++++++++++++ app/stream-backend.js | 3 + tests/unit/security.test.js | 218 ++++++++++++++++++++++++++ 8 files changed, 747 insertions(+), 61 deletions(-) create mode 100644 app/security.js create mode 100644 tests/unit/security.test.js diff --git a/app/capture.js b/app/capture.js index 5fd2701..ecabf50 100644 --- a/app/capture.js +++ b/app/capture.js @@ -2019,8 +2019,12 @@ public static class SFHook { webPreferences: { preload: path.join(__dirname, 'region-preload.js'), contextIsolation: true, + nodeIntegration: false, + sandbox: true, }, }); + // The overlay may only display region.html; deny navigation/popups. + require('./security').installWindowSecurity(overlay, 'region'); let settled = false; const finish = (rect) => { if (settled) return; diff --git a/app/main.js b/app/main.js index e73a9f2..5510c12 100644 --- a/app/main.js +++ b/app/main.js @@ -15,12 +15,13 @@ const { TemplateManager, FORMATS, FORMAT_LABELS } = require('../core/templates') const { buildRenderAst } = require('../core/renderast'); const { runExport, EXPORTERS } = require('../exporters'); const { runExportInWorker } = require('./export-runner'); -const { exportGuideArchive, importGuideArchive, saveLinkedGuide, readArchive } = require('../core/archive'); +const { exportGuideArchive, importGuideArchive, saveLinkedGuide } = require('../core/archive'); const { createSnapshot, listSnapshots, restoreSnapshot } = require('../core/snapshots'); const { readLock } = require('../core/locks'); const CaptureService = require('./capture'); const { TextIntelService } = require('./text-intel'); const { keepProcessesResponsive } = require('./win-power'); +const security = require('./security'); const PACKAGE_JSON = require(path.join(__dirname, '..', 'package.json')); const APP_ID = 'com.stepforge.app'; @@ -95,6 +96,7 @@ function createWindow() { preload: path.join(__dirname, 'preload.js'), contextIsolation: true, nodeIntegration: false, + sandbox: true, spellcheck: Boolean(settings.get('spellcheck')), // During a recording the window is minimized (Linux) or hidden (Windows). // A throttled renderer stops processing capture:added events, so the step @@ -103,6 +105,10 @@ function createWindow() { backgroundThrottling: false, }, }); + // The main window may only ever display our index.html: all navigation + // away from it and every popup is denied, so no other document can run + // with this window's preload bridge. + security.installWindowSecurity(mainWindow, 'main'); mainWindow.loadFile(path.join(__dirname, 'renderer', 'index.html')); mainWindow.once('ready-to-show', () => { mainWindow.show(); @@ -375,7 +381,37 @@ function sendToRenderer(channel, payload) { // ---- IPC ------------------------------------------------------------------ function setupIpc() { - const h = (channel, fn) => ipcMain.handle(channel, async (event, args = {}) => fn(args)); + // Every invoke channel is guarded: the event must come from the current + // main window's top frame showing our index.html, the argument bag must be + // a plain object within a per-channel payload budget, and channels with + // risky inputs additionally validate fields before the handler runs. + const trustedSender = security.makeIpcSenderGuard({ + getMainWebContents: () => (mainWindow && !mainWindow.isDestroyed() ? mainWindow.webContents : null), + }); + const c = security.check; + const IMAGE_BUDGET = 256 * 1024 * 1024; // channels that carry base64 PNGs + const h = (channel, fn, opts = {}) => { + const { maxChars = 2 * 1024 * 1024, validate = null } = opts; + ipcMain.handle(channel, async (event, args = {}) => { + if (!trustedSender(event)) { + throw new Error(`${channel}: rejected — untrusted IPC sender`); + } + const a = args === undefined || args === null ? {} : args; + if (!security.isPlainArgs(a) || !security.payloadWithinBudget(a, maxChars)) { + throw new Error(`${channel}: rejected — invalid or oversized arguments`); + } + if (validate && !validate(a)) { + throw new Error(`${channel}: rejected — arguments failed validation`); + } + return fn(a); + }); + }; + + // Files the main process itself produced (exports, previews); only these + // may be re-opened via the shell on renderer request. + const producedFiles = new security.ProducedFiles(); + // Output directories the user actually picked in a dialog this session. + const chosenOutputDirs = new Set(); // library h('library:list', () => ({ @@ -393,60 +429,71 @@ function setupIpc() { }); reindex(guide.guideId); return guide; - }); + }, { validate: (a) => c.optionalString(a.title, 500) }); h('library:duplicate', ({ guideId }) => { const copy = store.duplicateGuide(guideId); reindex(copy.guideId); return copy; - }); + }, { validate: (a) => c.id(a.guideId) }); h('library:delete', ({ guideId }) => { store.deleteGuide(guideId); searchIndex.removeGuide(guideId); return true; - }); - h('library:setFavorite', ({ guideId, favorite }) => store.setFavorite(guideId, favorite)); + }, { validate: (a) => c.id(a.guideId) }); + h('library:setFavorite', ({ guideId, favorite }) => store.setFavorite(guideId, favorite), + { validate: (a) => c.id(a.guideId) }); h('library:trash:list', () => store.listTrash()); h('library:trash:restore', ({ name }) => { const id = store.restoreFromTrash(name); reindex(id); return id; - }); + }, { validate: (a) => c.fileName(a.name) }); h('library:trash:purge', ({ names } = {}) => { if (names && names.length) store.purgeTrashItems(names); else store.purgeTrash(); return true; + }, { + validate: (a) => a.names === undefined || a.names === null + || (Array.isArray(a.names) && a.names.length <= 1000 && a.names.every((n) => c.fileName(n))), }); - h('folders:create', ({ name, parentId }) => store.createFolder(name, parentId || null)); - h('folders:rename', ({ folderId, name }) => store.renameFolder(folderId, name)); - h('folders:delete', ({ folderId }) => store.deleteFolder(folderId)); - h('folders:moveGuide', ({ guideId, folderId }) => store.moveGuideToFolder(guideId, folderId || null)); + h('folders:create', ({ name, parentId }) => store.createFolder(name, parentId || null), + { validate: (a) => c.string(a.name, 200) && c.optionalId(a.parentId) }); + h('folders:rename', ({ folderId, name }) => store.renameFolder(folderId, name), + { validate: (a) => c.id(a.folderId) && c.string(a.name, 200) }); + h('folders:delete', ({ folderId }) => store.deleteFolder(folderId), + { validate: (a) => c.id(a.folderId) }); + h('folders:moveGuide', ({ guideId, folderId }) => store.moveGuideToFolder(guideId, folderId || null), + { validate: (a) => c.id(a.guideId) && c.optionalId(a.folderId) }); // guide + steps h('guide:get', ({ guideId }) => ({ guide: store.getGuide(guideId), steps: orderedSteps(guideId), - })); + }), { validate: (a) => c.id(a.guideId) }); h('guide:save', ({ guide }) => { const saved = store.saveGuide(guide); reindex(guide.guideId); return saved; - }); + }, { validate: (a) => security.isPlainArgs(a.guide) && a.guide && c.id(a.guide.guideId) }); h('step:add', ({ guideId, fields, imageBase64, size, position }) => { const buf = imageBase64 ? Buffer.from(imageBase64, 'base64') : null; const step = store.addStep(guideId, fields || {}, buf, size || null, { position }); reindex(guideId); return step; + }, { + maxChars: IMAGE_BUDGET, + validate: (a) => c.id(a.guideId) && c.optionalBase64(a.imageBase64) && c.optionalNumber(a.position, 0, 100000), }); h('step:save', ({ guideId, step }) => { const saved = store.saveStep(guideId, step); reindex(guideId); return saved; - }); + }, { validate: (a) => c.id(a.guideId) && security.isPlainArgs(a.step) && a.step && c.id(a.step.stepId) }); h('step:delete', ({ guideId, stepId }) => { store.deleteStep(guideId, stepId); reindex(guideId); return true; - }); + }, { validate: (a) => c.id(a.guideId) && c.id(a.stepId) }); h('step:restore', ({ guideId, step, originalBase64, workingBase64, position }) => { const images = { original: originalBase64 ? Buffer.from(originalBase64, 'base64') : null, @@ -455,20 +502,34 @@ function setupIpc() { const restored = store.restoreStep(guideId, step, images, position); reindex(guideId); return restored; + }, { + maxChars: IMAGE_BUDGET, + validate: (a) => c.id(a.guideId) && security.isPlainArgs(a.step) && a.step + && c.optionalBase64(a.originalBase64) && c.optionalBase64(a.workingBase64) + && c.optionalNumber(a.position, 0, 100000), + }); + h('steps:reorder', ({ guideId, order }) => store.reorderSteps(guideId, order), { + validate: (a) => c.id(a.guideId) + && Array.isArray(a.order) && a.order.length <= 100000 && a.order.every((id) => c.id(id)), }); - h('steps:reorder', ({ guideId, order }) => store.reorderSteps(guideId, order)); h('step:imagePath', ({ guideId, stepId, which }) => { const p = store.stepImagePath(guideId, stepId, which || 'working'); return p && fs.existsSync(p) ? `file://${p}?v=${fs.statSync(p).mtimeMs}` : null; + }, { + validate: (a) => c.id(a.guideId) && c.id(a.stepId) + && (a.which === undefined || a.which === null || c.oneOf(a.which, ['original', 'working'])), }); h('step:setWorkingImage', ({ guideId, stepId, pngBase64, size, step }) => - store.setWorkingImage(guideId, stepId, Buffer.from(pngBase64, 'base64'), size, step || null)); + store.setWorkingImage(guideId, stepId, Buffer.from(pngBase64, 'base64'), size, step || null), { + maxChars: IMAGE_BUDGET, + validate: (a) => c.id(a.guideId) && c.id(a.stepId) && c.base64(a.pngBase64), + }); h('step:resetWorkingImage', ({ guideId, stepId }) => { const p = store.stepImagePath(guideId, stepId, 'original'); const img = nativeImage.createFromPath(p); const { width, height } = img.getSize(); return store.resetWorkingImage(guideId, stepId, { width, height }); - }); + }, { validate: (a) => c.id(a.guideId) && c.id(a.stepId) }); h('step:fromClipboard', ({ guideId, position }) => { const img = clipboard.readImage(); if (img.isEmpty()) return { ok: false, reason: 'clipboard has no image' }; @@ -479,7 +540,7 @@ function setupIpc() { }, img.toPNG(), { width, height }, { position }); reindex(guideId); return { ok: true, step }; - }); + }, { validate: (a) => c.id(a.guideId) && c.optionalNumber(a.position, 0, 100000) }); h('step:importImage', async ({ guideId }) => { const res = await dialog.showOpenDialog(mainWindow, { title: 'Import images as steps', @@ -497,11 +558,13 @@ function setupIpc() { } reindex(guideId); return { ok: true, steps }; - }); + }, { validate: (a) => c.id(a.guideId) }); // search - h('search:query', ({ q, guideId }) => searchIndex.search(q, { guideId: guideId || null })); - h('search:titles', ({ q }) => searchIndex.searchTitles(q)); + h('search:query', ({ q, guideId }) => searchIndex.search(q, { guideId: guideId || null }), + { validate: (a) => c.optionalString(a.q, 1000) && c.optionalId(a.guideId) }); + h('search:titles', ({ q }) => searchIndex.searchTitles(q), + { validate: (a) => c.optionalString(a.q, 1000) }); // settings + placeholders h('settings:all', () => settings.data); @@ -510,13 +573,13 @@ function setupIpc() { if (keyPath === 'appearance') applyTheme(); if (keyPath.startsWith('capture.hotkey')) registerHotkeys(); return settings.data; - }); + }, { validate: (a) => c.settingsKeyPath(a.keyPath) }); h('ai:test', async ({ enabled = null, ollama = null } = {}) => { return textIntel.testAiConnection({ enabled, ollama, }); - }); + }, { validate: (a) => (a.ollama === undefined || a.ollama === null || security.isPlainArgs(a.ollama)) }); h('ai:fillStep', async ({ guideId, stepId, target = 'all', blockId = null } = {}) => { const result = await textIntel.generateStepPatch({ guideId, @@ -526,9 +589,15 @@ function setupIpc() { }); if (result.ok) reindex(guideId); return result; + }, { + validate: (a) => c.id(a.guideId) && c.id(a.stepId) && c.optionalId(a.blockId) + && (a.target === undefined || c.oneOf(a.target, ['all', 'title', 'description', 'block'])), }); h('ai:rewriteText', async ({ text, guideTitle = '', stepTitle = '' } = {}) => { return textIntel.rewriteText({ text, guideTitle, stepTitle }); + }, { + validate: (a) => c.string(a.text, 200000) + && c.optionalString(a.guideTitle, 1000) && c.optionalString(a.stepTitle, 1000), }); h('placeholders:globals:get', () => settings.getGlobalPlaceholders()); h('placeholders:globals:set', (values) => settings.setGlobalPlaceholders(values)); @@ -552,6 +621,10 @@ function setupIpc() { } } return result; + }, { + validate: (a) => c.id(a.guideId) + && (a.mode === undefined || c.oneOf(a.mode, ['fullscreen', 'window', 'region'])) + && c.optionalNumber(a.delayMs, 0, 600000), }); h('capture:region', async ({ guideId }) => { const result = await capture.regionCapture(guideId); @@ -571,7 +644,7 @@ function setupIpc() { } } return result; - }); + }, { validate: (a) => c.id(a.guideId) }); let capturePowerBlocker = -1; const startCapturePower = () => { if (!powerSaveBlocker.isStarted(capturePowerBlocker)) { @@ -615,6 +688,10 @@ function setupIpc() { const state = capture.state(); sendToRenderer('capture:state', state); return state; + }, { + validate: (a) => c.oneOf(a.action, ['start', 'pause', 'resume', 'finish', 'interval']) + && (a.action !== 'start' || c.id(a.guideId)) + && c.optionalNumber(a.intervalSec, 0, 86400), }); h('capture:state', () => capture.state()); @@ -629,7 +706,7 @@ function setupIpc() { if (res.canceled) return { ok: false }; exportGuideArchive(store, guideId, res.filePath); return { ok: true, path: res.filePath }; - }); + }, { validate: (a) => c.id(a.guideId) }); h('archive:open', async ({ mode }) => { const res = await dialog.showOpenDialog(mainWindow, { title: 'Open guide archive', @@ -644,30 +721,38 @@ function setupIpc() { } catch (err) { return { ok: false, reason: err.message }; } - }); - h('archive:peek', ({ file }) => { - const { manifest } = readArchive(file); - return manifest; - }); - h('archive:saveLinked', ({ guideId, force }) => saveLinkedGuide(store, guideId, { force: Boolean(force) })); + }, { validate: (a) => a.mode === undefined || c.oneOf(a.mode, ['copy', 'linked']) }); + // archive:peek was removed: nothing in the renderer used it, and it let a + // compromised renderer read arbitrary local archives by path. + h('archive:saveLinked', ({ guideId, force }) => saveLinkedGuide(store, guideId, { force: Boolean(force) }), + { validate: (a) => c.id(a.guideId) }); // snapshots - h('snapshots:list', ({ guideId }) => listSnapshots(store, guideId)); + h('snapshots:list', ({ guideId }) => listSnapshots(store, guideId), + { validate: (a) => c.id(a.guideId) }); h('snapshots:create', ({ guideId, label }) => - createSnapshot(store, guideId, { label: label || 'manual', keepLast: settings.get('backups.keepLast') })); + createSnapshot(store, guideId, { label: label || 'manual', keepLast: settings.get('backups.keepLast') }), + { validate: (a) => c.id(a.guideId) && c.optionalString(a.label, 200) }); h('snapshots:restore', ({ guideId, name }) => { const guide = restoreSnapshot(store, guideId, name); reindex(guideId); return guide; - }); + }, { validate: (a) => c.id(a.guideId) && c.fileName(a.name) }); // templates - h('templates:list', ({ format }) => templates.list(format)); - h('templates:load', ({ format, name }) => templates.load(format, name)); - h('templates:save', ({ format, name, options }) => templates.save(format, name, options)); - h('templates:delete', ({ format, name }) => templates.remove(format, name)); - h('templates:rename', ({ format, name, newName }) => templates.rename(format, name, newName)); - h('templates:duplicate', ({ format, name }) => templates.duplicate(format, name)); + const validFormat = (v) => c.oneOf(v, FORMATS); + h('templates:list', ({ format }) => templates.list(format), + { validate: (a) => validFormat(a.format) }); + h('templates:load', ({ format, name }) => templates.load(format, name), + { validate: (a) => validFormat(a.format) && c.fileName(a.name) }); + h('templates:save', ({ format, name, options }) => templates.save(format, name, options), + { validate: (a) => validFormat(a.format) && c.fileName(a.name) && security.isPlainArgs(a.options) }); + h('templates:delete', ({ format, name }) => templates.remove(format, name), + { validate: (a) => validFormat(a.format) && c.fileName(a.name) }); + h('templates:rename', ({ format, name, newName }) => templates.rename(format, name, newName), + { validate: (a) => validFormat(a.format) && c.fileName(a.name) && c.fileName(a.newName) }); + h('templates:duplicate', ({ format, name }) => templates.duplicate(format, name), + { validate: (a) => validFormat(a.format) && c.fileName(a.name) }); h('templates:export', async ({ format, name }) => { const res = await dialog.showSaveDialog(mainWindow, { defaultPath: `${name}.sfglt`, @@ -676,7 +761,7 @@ function setupIpc() { if (res.canceled) return { ok: false }; templates.exportTemplate(format, name, res.filePath); return { ok: true }; - }); + }, { validate: (a) => validFormat(a.format) && c.fileName(a.name) }); h('templates:import', async () => { const res = await dialog.showOpenDialog(mainWindow, { filters: [{ name: 'StepForge template', extensions: ['sfglt'] }], @@ -709,15 +794,24 @@ function setupIpc() { }[format]; if (!mod) return {}; return { ...require(mod).DEFAULT_TEMPLATE }; - }); + }, { validate: (a) => c.string(a.format, 40) }); h('export:run', async ({ guideId, format, options, outDir }) => { - let dir = outDir || settings.get(`exports.lastOutputDirs.${format}`); + // The renderer may only nominate directories that came from this main + // process: a dialog pick from this session or a remembered last-output + // directory. Anything else is ignored and re-asked via the dialog. + const rememberedDirs = Object.values(settings.get('exports.lastOutputDirs') || {}); + let dir = null; + if (outDir && (chosenOutputDirs.has(outDir) || rememberedDirs.includes(outDir))) { + dir = outDir; + } + if (!dir) dir = settings.get(`exports.lastOutputDirs.${format}`); if (!dir) { const res = await dialog.showOpenDialog(mainWindow, { title: 'Choose output folder', properties: ['openDirectory', 'createDirectory'], }); if (res.canceled) return { ok: false }; dir = res.filePaths[0]; + chosenOutputDirs.add(dir); } settings.set(`exports.lastOutputDirs.${format}`, dir); const result = await runExportInWorker({ @@ -728,17 +822,23 @@ function setupIpc() { outDir: dir, globals: settings.getGlobalPlaceholders(), }); + producedFiles.add(result.file); if (settings.get('exports.openFolderAfterExport')) shell.showItemInFolder(result.file); return { ok: true, ...result }; + }, { + validate: (a) => c.id(a.guideId) && validFormat(a.format) + && (a.options === undefined || security.isPlainArgs(a.options)) + && c.optionalString(a.outDir, 1000), }); h('export:chooseDir', async ({ format }) => { const res = await dialog.showOpenDialog(mainWindow, { title: 'Choose output folder', properties: ['openDirectory', 'createDirectory'], }); if (res.canceled) return null; + chosenOutputDirs.add(res.filePaths[0]); settings.set(`exports.lastOutputDirs.${format}`, res.filePaths[0]); return res.filePaths[0]; - }); + }, { validate: (a) => validFormat(a.format) }); h('export:preview', ({ guideId, format, options }) => { const previewDir = path.join(store.tempDir, `preview-${guideId}-${format}`); fs.rmSync(previewDir, { recursive: true, force: true }); @@ -747,7 +847,11 @@ function setupIpc() { maxSteps: settings.get('exports.previewStepCount') || 3, }); const result = runExport(format, ast, previewDir, options || {}); + producedFiles.add(result.file); return { ok: true, file: result.file, fileUrl: `file://${result.file}` }; + }, { + validate: (a) => c.id(a.guideId) && validFormat(a.format) + && (a.options === undefined || security.isPlainArgs(a.options)), }); h('preview:cleanup', () => { for (const entry of fs.readdirSync(store.tempDir)) { @@ -758,9 +862,32 @@ function setupIpc() { return true; }); - // shell helpers - h('shell:openPath', ({ target }) => shell.openPath(target)); - h('shell:showItemInFolder', ({ target }) => shell.showItemInFolder(target)); + // shell helpers — intent-specific, no arbitrary paths from the renderer. + // Only files this main process produced (exports/previews) may be opened. + h('shell:openProduced', ({ target }) => { + if (!producedFiles.has(target)) { + return { ok: false, reason: 'not a StepForge-produced file' }; + } + shell.openPath(target); + return { ok: true }; + }, { validate: (a) => c.string(a.target, 2000) }); + // Reveal the linked archive of a guide; the path comes from the store, + // never from the renderer. + h('shell:revealLinkedArchive', ({ guideId }) => { + const guide = store.getGuide(guideId); + const target = guide && guide.linkedSource && guide.linkedSource.path; + if (!target || !fs.existsSync(target)) return { ok: false, reason: 'no linked archive' }; + shell.showItemInFolder(target); + return { ok: true }; + }, { validate: (a) => c.id(a.guideId) }); + // Open a user-clicked link in the system browser. Scheme-validated; + // everything that is not plain http(s)/mailto is refused. + h('shell:openExternal', ({ url }) => { + const safe = security.validateExternalUrl(url); + if (!safe) return { ok: false, reason: 'blocked URL' }; + shell.openExternal(safe); + return { ok: true }; + }, { validate: (a) => c.string(a.url, 2048) }); h('app:info', () => ({ version: app.getVersion(), buildVersion: PACKAGE_JSON.buildVersion || app.getVersion(), @@ -842,15 +969,19 @@ if (!gotLock) { textIntel, }); - // Allow the hidden capture-worker renderer to open a desktop media stream. - // Electron 29+ requires an explicit permission grant for display-capture in - // renderer windows; without it getUserMedia/getDisplayMedia fails, the - // stream backend never starts, and every capture falls back to - // desktopCapturer.getSources() — which triggers the portal dialog on Linux - // on every single capture. StepForge is fully local/offline so allowing - // all permissions for our own content is safe. - session.defaultSession.setPermissionCheckHandler(() => true); - session.defaultSession.setPermissionRequestHandler((_wc, _perm, cb) => cb(true)); + // Deny-by-default permission policy. The only grant in the entire app is + // display capture (and the media permission getDisplayMedia consults) for + // the dedicated hidden capture-worker page. Electron 29+ requires that + // explicit grant; everything else — including our own main window — is + // rejected. "Content is local" is not a security control. + session.defaultSession.setPermissionCheckHandler((wc, permission, requestingOrigin, details) => { + const url = (details && details.requestingUrl) || (wc && wc.getURL()) || requestingOrigin; + return security.permissionAllowed(permission, url); + }); + session.defaultSession.setPermissionRequestHandler((wc, permission, cb, details) => { + const url = (details && details.requestingUrl) || (wc && wc.getURL()); + cb(security.permissionAllowed(permission, url)); + }); // On GNOME Wayland the only working screen-capture path is the portal-backed // getDisplayMedia (desktopCapturer source ids fail with "device not found"). @@ -860,6 +991,13 @@ if (!gotLock) { // the chosen source then streams for the whole session. (useSystemPicker is // macOS-only today, harmless elsewhere.) session.defaultSession.setDisplayMediaRequestHandler((request, callback) => { + // Only the capture worker page may open a desktop stream. + const frameUrl = request && request.frame && request.frame.url; + if (!security.isAppPageUrl(frameUrl, 'captureWorker')) { + console.error('[stepforge] display-media request denied for', frameUrl || '(unknown frame)'); + callback({}); + return; + } desktopCapturer.getSources({ types: ['screen'] }) .then((sources) => { console.log(`[stepforge] display-media request resolved: ${sources.length} screen source(s)`); diff --git a/app/preload.js b/app/preload.js index 2f48a7e..72cf637 100644 --- a/app/preload.js +++ b/app/preload.js @@ -95,8 +95,11 @@ const api = { cleanupPreviews: invoke('preview:cleanup'), }, shell: { - openPath: invoke('shell:openPath'), - showItemInFolder: invoke('shell:showItemInFolder'), + // Intent-specific shell access only: files the main process produced, + // the guide's linked archive, and scheme-validated external links. + openProduced: invoke('shell:openProduced'), + revealLinkedArchive: invoke('shell:revealLinkedArchive'), + openExternal: invoke('shell:openExternal'), }, app: { info: invoke('app:info'), diff --git a/app/renderer/app.js b/app/renderer/app.js index 760ca6d..bd796d0 100644 --- a/app/renderer/app.js +++ b/app/renderer/app.js @@ -934,6 +934,24 @@ class StepForgeApp { window.StepForgeApp = StepForgeApp; +// Links never navigate this window. http(s)/mailto links from guide content +// open externally via the scheme-validated main-process handler; internal +// step:/# links are handled by their own click handlers; everything else is +// inert. The main process additionally denies all navigation, so this is the +// user-experience half of a two-layer guarantee. +document.addEventListener('click', (e) => { + const anchor = e.target && e.target.closest ? e.target.closest('a[href]') : null; + if (!anchor) return; + const href = anchor.getAttribute('href') || ''; + if (/^(https?|mailto):/i.test(href)) { + e.preventDefault(); + api.shell.openExternal({ url: href }); + return; + } + // Ensure a stray href can never navigate the privileged window. + if (!href.startsWith('#')) e.preventDefault(); +}, true); + function boot() { const app = new StepForgeApp(); app.init(); diff --git a/app/renderer/editor.js b/app/renderer/editor.js index 420425b..98637fe 100644 --- a/app/renderer/editor.js +++ b/app/renderer/editor.js @@ -1899,7 +1899,7 @@ class GuideEditor { onPreview: async ({ format, options }) => { const preview = await api.export.preview({ guideId: this.guideId, format, options }); if (preview && preview.file) { - await api.shell.openPath({ target: preview.file }); // open in default viewer + await api.shell.openProduced({ target: preview.file }); // open in default viewer this.onToast('Preview opened (first steps only).'); } return true; @@ -1935,7 +1935,7 @@ class GuideEditor { else this.onToast('Could not save linked archive.', { error: true }); }, onOpenArchive: async () => { - await api.shell.showItemInFolder({ target: this.guide.linkedSource.path }); + await api.shell.revealLinkedArchive({ guideId: this.guideId }); }, }); } diff --git a/app/security.js b/app/security.js new file mode 100644 index 0000000..7c3a028 --- /dev/null +++ b/app/security.js @@ -0,0 +1,302 @@ +'use strict'; + +/** + * Privilege-boundary policy for every renderer surface. + * + * This module is deliberately loadable under plain Node (no electron + * require at module scope) so the decision logic is unit-testable: + * - which URLs count as our own app pages, + * - whether a navigation/popup may proceed (it may not), + * - which permission a renderer may be granted (display capture for the + * dedicated capture worker only), + * - whether an IPC event comes from the trusted main-window frame, + * - which external URLs may be handed to shell.openExternal, + * - which filesystem paths the renderer may ask the shell to open + * (only files the main process itself produced). + */ + +const path = require('node:path'); +const { pathToFileURL } = require('node:url'); + +const RENDERER_DIR = path.join(__dirname, 'renderer'); + +const APP_PAGES = { + main: pathToFileURL(path.join(RENDERER_DIR, 'index.html')).href, + region: pathToFileURL(path.join(RENDERER_DIR, 'region.html')).href, + captureWorker: pathToFileURL(path.join(RENDERER_DIR, 'capture-worker.html')).href, +}; + +/** Normalize a URL for identity comparison: drop query/hash, decode path. */ +function normalizeAppUrl(rawUrl) { + let url; + try { + url = new URL(String(rawUrl)); + } catch { + return null; + } + if (url.protocol !== 'file:') return null; + let pathname; + try { + pathname = decodeURIComponent(url.pathname); + } catch { + pathname = url.pathname; + } + // Windows drive letters may differ in case between loadFile and senderFrame. + if (process.platform === 'win32') pathname = pathname.toLowerCase(); + return pathname; +} + +function isAppPageUrl(rawUrl, page) { + const expected = APP_PAGES[page]; + if (!expected) return false; + const a = normalizeAppUrl(rawUrl); + const b = normalizeAppUrl(expected); + return a !== null && a === b; +} + +/** + * Navigation policy: a privileged window may only ever stay on the exact + * page it was created with. Everything else — remote URLs, other local + * files, javascript:, data: — is denied. + */ +function navigationAllowed(targetUrl, page) { + return isAppPageUrl(targetUrl, page); +} + +/** + * Permission policy for Electron sessions. Display capture (and the media + * permission that getDisplayMedia consults) is granted only to the dedicated + * hidden capture-worker page. Everything else is denied for everyone, + * including our own main window. + */ +function permissionAllowed(permission, requestingUrl) { + if (permission === 'media' || permission === 'display-capture') { + return isAppPageUrl(requestingUrl, 'captureWorker'); + } + return false; +} + +/** + * External-link policy: only well-formed http(s) and mailto URLs may reach + * shell.openExternal. Returns the normalized URL string or null. + */ +function validateExternalUrl(rawUrl) { + if (typeof rawUrl !== 'string' || rawUrl.length > 2048) return null; + let url; + try { + url = new URL(rawUrl); + } catch { + return null; + } + if (url.protocol === 'https:' || url.protocol === 'http:') { + if (!url.hostname) return null; + return url.href; + } + if (url.protocol === 'mailto:') return url.href; + return null; +} + +/** + * IPC sender guard: an invoke event is trusted only when it originates from + * the current main window's top frame, and that frame is our index.html. + * Destroyed frames, subframes, other windows, and navigated-away frames are + * all rejected. + */ +function makeIpcSenderGuard({ getMainWebContents }) { + return function trustedSender(event) { + if (!event || typeof event !== 'object') return false; + const expected = getMainWebContents(); + if (!expected || event.sender !== expected) return false; + const frame = event.senderFrame; + if (!frame) return false; + try { + if (frame.parent) return false; // top frame only + return isAppPageUrl(frame.url, 'main'); + } catch { + // Accessing a disposed WebFrameMain throws — reject. + return false; + } + }; +} + +/** + * Cheap recursive payload budget: sums string lengths (and key counts) with + * early exit. Protects handlers from absurd payloads without JSON.stringify + * on hundred-megabyte image saves. + */ +function payloadWithinBudget(value, maxChars, depth = 0) { + let budget = maxChars; + + const walk = (v, d) => { + if (budget < 0 || d > 16) return false; + if (v == null) return true; + const t = typeof v; + if (t === 'string') { + budget -= v.length; + return budget >= 0; + } + if (t === 'number' || t === 'boolean') { + budget -= 8; + return budget >= 0; + } + if (t === 'function' || t === 'symbol' || t === 'bigint') return false; + if (Array.isArray(v)) { + if (v.length > 100000) return false; + for (const item of v) if (!walk(item, d + 1)) return false; + return true; + } + if (t === 'object') { + const keys = Object.keys(v); + if (keys.length > 4096) return false; + for (const key of keys) { + budget -= key.length; + if (budget < 0) return false; + if (!walk(v[key], d + 1)) return false; + } + return true; + } + return false; + }; + + return walk(value, depth); +} + +/** True for plain-object argument bags (what every IPC channel expects). */ +function isPlainArgs(args) { + if (args === undefined || args === null) return true; + if (typeof args !== 'object' || Array.isArray(args)) return false; + const proto = Object.getPrototypeOf(args); + return proto === Object.prototype || proto === null; +} + +// ---- field validators (used by main.js per-channel checks) ---------------- + +const ID_RE = /^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$/; + +const check = { + /** Guide/step/folder/template identifiers and snapshot/trash entry names: + * single path segment, no separators, no dot-dot, sane length. */ + id(v) { + return typeof v === 'string' && ID_RE.test(v) && !v.includes('..'); + }, + optionalId(v) { + return v === null || v === undefined || check.id(v); + }, + string(v, max = 4096) { + return typeof v === 'string' && v.length <= max; + }, + optionalString(v, max = 4096) { + return v === null || v === undefined || check.string(v, max); + }, + bool(v) { + return typeof v === 'boolean'; + }, + number(v, min = -1e15, max = 1e15) { + return typeof v === 'number' && Number.isFinite(v) && v >= min && v <= max; + }, + optionalNumber(v, min, max) { + return v === null || v === undefined || check.number(v, min, max); + }, + oneOf(v, values) { + return values.includes(v); + }, + base64(v, maxChars = 192 * 1024 * 1024) { + return typeof v === 'string' && v.length <= maxChars && /^[A-Za-z0-9+/=\r\n]*$/.test(v.slice(0, 4096)); + }, + optionalBase64(v, maxChars) { + return v === null || v === undefined || check.base64(v, maxChars); + }, + /** Single filesystem name (template/snapshot/trash entries): no path + * separators, no traversal, no NUL, printable, bounded length. */ + fileName(v, max = 160) { + return ( + typeof v === 'string' && + v.trim().length > 0 && + v.length <= max && + !/[/\\\0]/.test(v) && + !v.includes('..') && + v !== '.' && + // eslint-disable-next-line no-control-regex + !/[\x00-\x1f]/.test(v) + ); + }, + optionalFileName(v, max) { + return v === null || v === undefined || check.fileName(v, max); + }, + /** settings keyPath: dotted segments, no prototype-pollution segments. */ + settingsKeyPath(v) { + if (typeof v !== 'string' || v.length === 0 || v.length > 200) return false; + const segments = v.split('.'); + return segments.every( + (segment) => + /^[A-Za-z0-9_-]+$/.test(segment) && + !['__proto__', 'constructor', 'prototype'].includes(segment) + ); + }, +}; + +/** + * Registry of files the main process itself produced (export outputs, + * previews) and may therefore re-open on renderer request. Bounded LRU. + */ +class ProducedFiles { + constructor(limit = 256) { + this.limit = limit; + this.paths = new Set(); + } + + key(p) { + const resolved = path.resolve(String(p)); + return process.platform === 'win32' ? resolved.toLowerCase() : resolved; + } + + add(p) { + if (!p || typeof p !== 'string') return; + const key = this.key(p); + this.paths.delete(key); + this.paths.add(key); + while (this.paths.size > this.limit) { + this.paths.delete(this.paths.values().next().value); + } + } + + has(p) { + if (!p || typeof p !== 'string') return false; + return this.paths.has(this.key(p)); + } +} + +/** + * Attach the navigation/popup policy to a BrowserWindow. `page` names the + * app page this window is allowed to display (key of APP_PAGES). + */ +function installWindowSecurity(win, page) { + const contents = win.webContents; + contents.setWindowOpenHandler(() => ({ action: 'deny' })); + contents.on('will-navigate', (event, url) => { + if (!navigationAllowed(url, page)) event.preventDefault(); + }); + contents.on('will-frame-navigate', (details) => { + if (!navigationAllowed(details.url, page) && typeof details.preventDefault === 'function') { + details.preventDefault(); + } + }); + // Defense in depth: if a disallowed document somehow starts loading, + // never let it attach webviews either. + contents.on('will-attach-webview', (event) => event.preventDefault()); +} + +module.exports = { + APP_PAGES, + ProducedFiles, + check, + installWindowSecurity, + isAppPageUrl, + isPlainArgs, + makeIpcSenderGuard, + navigationAllowed, + normalizeAppUrl, + payloadWithinBudget, + permissionAllowed, + validateExternalUrl, +}; diff --git a/app/stream-backend.js b/app/stream-backend.js index 302da4f..4e43e82 100644 --- a/app/stream-backend.js +++ b/app/stream-backend.js @@ -343,11 +343,14 @@ async function createElectronHost(onEvent) { preload: path.join(__dirname, 'capture-worker-preload.js'), contextIsolation: true, nodeIntegration: false, + sandbox: true, // The worker must keep sampling while hidden — throttling a hidden // window is exactly the wrong default for a frame recorder. backgroundThrottling: false, }, }); + // The worker may only display capture-worker.html; deny navigation/popups. + require('./security').installWindowSecurity(win, 'captureWorker'); const listener = (event, msg) => { if (event.sender === win.webContents) onEvent(msg); }; diff --git a/tests/unit/security.test.js b/tests/unit/security.test.js new file mode 100644 index 0000000..b10020a --- /dev/null +++ b/tests/unit/security.test.js @@ -0,0 +1,218 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const security = require('../../app/security'); + +const MAIN_URL = security.APP_PAGES.main; +const WORKER_URL = security.APP_PAGES.captureWorker; +const REGION_URL = security.APP_PAGES.region; + +const ROOT = path.resolve(__dirname, '..', '..'); +const read = (rel) => fs.readFileSync(path.join(ROOT, rel), 'utf8'); + +// ---- navigation policy ----------------------------------------------------- + +test('navigation: a window may only stay on its own app page', () => { + assert.equal(security.navigationAllowed(MAIN_URL, 'main'), true); + assert.equal(security.navigationAllowed(`${MAIN_URL}?q=1#frag`, 'main'), true); + assert.equal(security.navigationAllowed(REGION_URL, 'region'), true); + + // Hostile / cross-page navigations are all denied. + for (const target of [ + 'https://evil.example/phish.html', + 'http://127.0.0.1:8080/', + 'javascript:alert(1)', + 'data:text/html,', + 'about:blank', + REGION_URL, // a *different* app page is still a denial for 'main' + 'file:///etc/passwd', + 'not a url', + '', + ]) { + assert.equal(security.navigationAllowed(target, 'main'), false, `should deny ${target}`); + } + assert.equal(security.navigationAllowed(MAIN_URL, 'nonexistent-page'), false); +}); + +// ---- permission policy ----------------------------------------------------- + +test('permissions: display capture only for the capture worker, nothing else for anyone', () => { + assert.equal(security.permissionAllowed('display-capture', WORKER_URL), true); + assert.equal(security.permissionAllowed('media', WORKER_URL), true); + + // The main window gets nothing, including display capture. + assert.equal(security.permissionAllowed('display-capture', MAIN_URL), false); + assert.equal(security.permissionAllowed('media', MAIN_URL), false); + + // Everything else is denied even for the worker. + for (const permission of ['geolocation', 'notifications', 'clipboard-read', 'openExternal', 'fullscreen', 'pointerLock', 'hid', 'usb', 'serial']) { + assert.equal(security.permissionAllowed(permission, WORKER_URL), false, permission); + } + + // Remote origins never get anything. + assert.equal(security.permissionAllowed('display-capture', 'https://evil.example/'), false); + assert.equal(security.permissionAllowed('media', undefined), false); +}); + +// ---- external URL policy --------------------------------------------------- + +test('external links: only well-formed http(s)/mailto pass', () => { + assert.equal(security.validateExternalUrl('https://example.com/docs'), 'https://example.com/docs'); + assert.equal(security.validateExternalUrl('http://example.com'), 'http://example.com/'); + assert.match(security.validateExternalUrl('mailto:hi@example.com'), /^mailto:/); + + for (const url of [ + 'javascript:alert(1)', + 'file:///etc/passwd', + 'data:text/html,x', + 'ftp://example.com/x', + 'smb://server/share', + 'chrome://settings', + 'vbscript:x', + 'https://', + 'not a url', + 123, + null, + `https://example.com/${'a'.repeat(3000)}`, + ]) { + assert.equal(security.validateExternalUrl(url), null, `should reject ${String(url).slice(0, 60)}`); + } +}); + +// ---- IPC sender guard -------------------------------------------------------- + +function makeGuard(mainWc) { + return security.makeIpcSenderGuard({ getMainWebContents: () => mainWc }); +} + +test('ipc guard: accepts only the main window top frame on index.html', () => { + const wc = { id: 1 }; + const guard = makeGuard(wc); + + assert.equal(guard({ sender: wc, senderFrame: { parent: null, url: MAIN_URL } }), true); +}); + +test('ipc guard: rejects other windows, subframes, navigated and disposed frames', () => { + const wc = { id: 1 }; + const otherWc = { id: 2 }; + const guard = makeGuard(wc); + + // Different webContents (popup, worker, region overlay). + assert.equal(guard({ sender: otherWc, senderFrame: { parent: null, url: MAIN_URL } }), false); + // Subframe of our own window. + assert.equal(guard({ sender: wc, senderFrame: { parent: {}, url: MAIN_URL } }), false); + // Frame that navigated somewhere else but kept the preload bridge. + assert.equal(guard({ sender: wc, senderFrame: { parent: null, url: 'https://evil.example/' } }), false); + assert.equal(guard({ sender: wc, senderFrame: { parent: null, url: WORKER_URL } }), false); + // Disposed frame accessor throws. + const disposed = {}; + Object.defineProperty(disposed, 'parent', { get() { throw new Error('disposed'); } }); + assert.equal(guard({ sender: wc, senderFrame: disposed }), false); + // Missing frame or window entirely. + assert.equal(guard({ sender: wc, senderFrame: null }), false); + assert.equal(makeGuard(null)({ sender: wc, senderFrame: { parent: null, url: MAIN_URL } }), false); + assert.equal(guard(null), false); +}); + +// ---- argument hygiene -------------------------------------------------------- + +test('args must be plain object bags', () => { + assert.equal(security.isPlainArgs({ a: 1 }), true); + assert.equal(security.isPlainArgs(Object.create(null)), true); + assert.equal(security.isPlainArgs(undefined), true); + assert.equal(security.isPlainArgs(null), true); + assert.equal(security.isPlainArgs([1, 2]), false); + assert.equal(security.isPlainArgs('x'), false); + class Weird {} + assert.equal(security.isPlainArgs(new Weird()), false); +}); + +test('payload budget rejects oversized and non-data values', () => { + assert.equal(security.payloadWithinBudget({ a: 'small' }, 1000), true); + assert.equal(security.payloadWithinBudget({ a: 'x'.repeat(2000) }, 1000), false); + assert.equal(security.payloadWithinBudget({ fn: () => 1 }, 1000), false); + // Deep nesting is cut off. + let deep = 'leaf'; + for (let i = 0; i < 40; i += 1) deep = { deep }; + assert.equal(security.payloadWithinBudget(deep, 100000), false); + // Numbers/booleans/null are fine. + assert.equal(security.payloadWithinBudget({ n: 5, b: true, z: null }, 1000), true); +}); + +test('field validators refuse traversal, separators, and pollution', () => { + const c = security.check; + assert.equal(c.id('guide-123_A.b'), true); + assert.equal(c.id('../../etc/passwd'), false); + assert.equal(c.id('a/b'), false); + assert.equal(c.id('a\\b'), false); + assert.equal(c.id(''), false); + assert.equal(c.id(42), false); + + assert.equal(c.fileName('My snapshot 2026-07-03'), true); + assert.equal(c.fileName('..'), false); + assert.equal(c.fileName('a/../b'), false); + assert.equal(c.fileName('a/b'), false); + assert.equal(c.fileName('a\\b'), false); + assert.equal(c.fileName('a\0b'), false); + assert.equal(c.fileName(' '), false); + + assert.equal(c.settingsKeyPath('capture.hotkeyCapture'), true); + assert.equal(c.settingsKeyPath('__proto__.polluted'), false); + assert.equal(c.settingsKeyPath('a.constructor.b'), false); + assert.equal(c.settingsKeyPath('a..b'), false); + assert.equal(c.settingsKeyPath(''), false); + + assert.equal(c.base64('aGVsbG8=', 100), true); + assert.equal(c.base64('