From 46f9f32aefa632887468841e6d8af3f82e7c3552 Mon Sep 17 00:00:00 2001 From: th Date: Fri, 5 Jun 2026 20:07:19 +0200 Subject: [PATCH] Add security review and remediation plan --- dev_docs/security-remediation-plan.md | 318 ++++++++++++++++++++++++++ dev_docs/security-review.md | 174 ++++++++++++++ 2 files changed, 492 insertions(+) create mode 100644 dev_docs/security-remediation-plan.md create mode 100644 dev_docs/security-review.md diff --git a/dev_docs/security-remediation-plan.md b/dev_docs/security-remediation-plan.md new file mode 100644 index 0000000..f2c01af --- /dev/null +++ b/dev_docs/security-remediation-plan.md @@ -0,0 +1,318 @@ +# Security Remediation Plan + +Date: 2026-06-05 + +Source review: `dev_docs/security-review.md` + +## Goal + +Close the security issues from the review without changing RUWIO's core authoring workflow. The first priority is to make the MP4 render server safe to run on a developer machine by default; the second is to make project and bundle input handling bounded and reproducible. + +## Principles + +- Secure defaults first: local-only render server, explicit opt-in for network exposure, and bounded work. +- Validate at every trust boundary: imported files, `.ruw` bundles, render requests, Electron IPC, and generated/exported HTML. +- Keep browser and desktop builds working while hardening server-only paths. +- Add tests for security behavior so fixes do not drift quietly. + +## Phase 0: Baseline And Guardrails + +Files likely touched: + +- `dev_docs/security-review.md` +- `package.json` +- `server/package.json` +- `server/server.js` +- `src/devtools/tests.ts` + +Tasks: + +1. Capture a clean baseline with `npm run build`. +2. Add or document a lightweight verification command for render-server checks once server tests exist. +3. Decide the initial production limits so later validation has concrete numbers: + - maximum canvas width and height + - maximum render fps + - maximum render duration and total frames + - maximum layer, clip, keyframe, scene, asset, and point counts + - maximum JSON body, `.json` project, `.ruw` bundle, per-ZIP-entry, and total extracted asset size +4. Record the selected limits in this document or a small constants module. + +Acceptance: + +- Existing build still passes before remediation starts. +- Limits are explicit instead of scattered magic numbers. + +## Phase 1: Lock Down The Render Server + +Addresses: + +- High: unauthenticated cross-origin render server +- Part of Medium: unbounded render requests + +Files likely touched: + +- `server/server.js` +- `src/lib/serverExport.ts` +- `src/components/ExportDialog.vue` +- `docker-compose.yml` +- `README.md` + +Tasks: + +1. Bind the server to `127.0.0.1` by default: + - Add `HOST=127.0.0.1` default in `server/server.js`. + - Use `app.listen(PORT, HOST, ...)`. + - Change Docker Compose publishing from `6000:6000` to `127.0.0.1:6000:6000`. + - Require an explicit `HOST=0.0.0.0` or compose override for LAN exposure. +2. Replace default `cors()` with an allowlist: + - Add `RENDER_ALLOWED_ORIGINS`, defaulting to the Vite dev origin and localhost app origins that are actually needed. + - Reject browser requests with unexpected `Origin` values. + - Keep non-browser/local CLI requests possible when they have no `Origin` and pass auth. +3. Add authentication: + - Add `RENDER_TOKEN`. + - Require `Authorization: Bearer ` or `X-RUWIO-Render-Token` for `POST /render`. + - Because `EventSource` cannot send custom headers, have `POST /render` return a random per-job access token, then require that token for `/render/:id/events`, `/render/:id/file`, and `DELETE /render/:id`. + - Add render-server settings in the client if the token must be user-configurable. +4. Add basic abuse controls: + - Lower `express.json` from `200mb` to a documented default, with an environment override. + - Reject requests with a too-large `Content-Length` before JSON parsing. + - Add per-IP request rate limits for `/render`. + - Add `MAX_ACTIVE_JOBS` and `MAX_QUEUED_JOBS`; return `429` or `503` when full. + - Add a per-job timeout that aborts Puppeteer and `ffmpeg`. +5. Improve job cleanup: + - Delete failed and aborted job directories. + - Remove completed job metadata after download TTL expires. + - Ensure `ffmpeg` is killed when Chromium/page work fails. + +Acceptance: + +- A default server only listens on localhost. +- Cross-origin requests from an unlisted origin fail. +- Requests without the configured token fail. +- A completed job can still be started, watched, downloaded, and deleted by the RUWIO client. +- Concurrent jobs, over-large bodies, and over-long renders are rejected with clear errors. + +## Phase 2: Harden Chromium And The Container + +Addresses: + +- High: Chromium renders untrusted data with sandbox disabled + +Files likely touched: + +- `server/server.js` +- `server/Dockerfile` +- `docker-compose.yml` +- `README.md` + +Tasks: + +1. Run the container as a non-root user: + - Create an app user/group in `server/Dockerfile`. + - Ensure `/app` and temporary render directories are writable by that user. + - Set `USER` before `CMD`. +2. Remove unconditional sandbox-disabling Chromium flags: + - Try launching Chromium without `--no-sandbox` and `--disable-setuid-sandbox`. + - Keep an explicit `CHROME_NO_SANDBOX=1` escape hatch only if needed for a known deployment environment. + - Log a loud warning when the escape hatch is used. +3. Add process and container limits: + - Configure Docker Compose memory/CPU/pids limits where supported. + - Add Node-side child-process kill paths for abort, timeout, and client disconnect. + - Consider using a dedicated worker module/process so Express request handling and rendering are isolated. +4. Limit network reachability from render jobs: + - Prefer rendering local/data assets only. + - If external URLs must remain supported, document the SSRF/privacy tradeoff and add an allowlist or opt-in later. + +Acceptance: + +- The Docker image runs as a non-root user. +- The normal render path works without `--no-sandbox`, or the documented fallback is explicit and visible. +- Aborted/timed-out jobs leave no running Chromium or `ffmpeg` process. + +## Phase 3: Add Shared Project Validation And Resource Bounds + +Addresses: + +- Medium: project and bundle imports lack deep validation and bounds +- Part of High/Medium: unbounded render workload + +Files likely touched: + +- `src/types/scene.ts` +- new `src/lib/projectLimits.ts` +- new `src/lib/projectValidation.ts` +- `src/lib/projectFile.ts` +- `src/lib/ruwFile.ts` +- `src/lib/zipUtil.ts` +- `server/server.js` +- possibly `server/validation.js` or a shared package if server reuse becomes necessary + +Tasks: + +1. Add centralized limits: + - Keep numeric bounds in one module. + - Use browser-safe limits for imports and stricter server-safe limits for rendering if needed. +2. Add schema validation at import boundaries: + - Validate `Project`, `Scene`, `Asset`, `Layer`, `Keyframe`, clips, actions, background media, and canvas data. + - Reject non-finite numbers, negative dimensions, invalid colors where practical, oversized strings, and unknown critical shapes. + - Normalize optional arrays and legacy fields into safe defaults. + - Return friendly `ProjectFileError` messages rather than raw parser errors. +3. Add render-request validation: + - Validate `scene` and `options` before creating a job. + - Clamp or reject `fps`, `duration`, `crf`, `preset`, canvas size, and total frame count. + - Compute `totalFrames` only after validation. +4. Harden ZIP parsing: + - Add caps for entry count, archive byte length, central directory range, name length, per-entry size, and total extracted size. + - Check every local/central directory offset and data range before creating `DataView` or `subarray`. + - Reject path traversal and absolute paths even though extraction is in-memory. + - Preserve STORE-only behavior. +5. Add import tests: + - Valid legacy `.json` and `.ruw` still load. + - Malformed JSON reports a friendly error. + - Oversized canvas, extreme fps/duration/keyframes/assets, malformed ZIP offsets, too many entries, and missing assets are rejected. + +Acceptance: + +- Project import returns a safe internal `Project` shape. +- `.ruw` parsing fails early for oversized or malformed archives. +- Render server rejects pathological render requests before launching Chromium. +- Existing normal projects still open and export. + +## Phase 4: Make Server Dependencies Reproducible + +Addresses: + +- Medium: render server dependency installs are not reproducible + +Files likely touched: + +- `server/package-lock.json` +- `server/Dockerfile` +- root CI config when one exists +- `README.md` + +Tasks: + +1. Generate and commit `server/package-lock.json`. +2. Replace `npm install --omit=dev` with `npm ci --omit=dev` in `server/Dockerfile`. +3. Add server audit/documentation: + - `npm audit --prefix server --omit=dev` + - Add the command to local release checks or CI. +4. Rebuild the server image from a clean checkout to verify the lockfile path. + +Acceptance: + +- `npm audit --prefix server --omit=dev` can run. +- Docker builds use the committed lockfile. +- Repeated builds install identical dependency versions. + +## Phase 5: Remove Devtools From Production Bundles + +Addresses: + +- Medium: development test overlay ships in production + +Files likely touched: + +- `src/App.vue` +- `src/devtools/TestOverlay.vue` +- `src/devtools/tests.ts` + +Tasks: + +1. Replace the static `TestOverlay` import with a development-only dynamic import: + - Use `defineAsyncComponent` only when `import.meta.env.DEV` is true. + - Mount the overlay only in development. +2. Guard menu/command paths that open the test runner: + - Keep Electron menu gating already present. + - Ensure renderer-side command handling ignores `test-runner` in production. +3. Verify bundle output: + - Run `npm run build`. + - Confirm production chunks do not include `src/devtools/tests.ts` or the test overlay. + +Acceptance: + +- Production build output excludes devtools tests. +- Development build still opens the test overlay. + +## Phase 6: Tighten Electron Navigation And External Links + +Addresses: + +- Low: Electron navigation and external link policy could be stricter + +Files likely touched: + +- `electron/main.cts` + +Tasks: + +1. Replace string-prefix URL checks with parsed `URL` checks. +2. Define allowed renderer origins: + - Development: exactly the configured Vite origin. + - Production: the packaged `file://` app entry and same-directory app assets. +3. Restrict external opening: + - Prefer `https:` only. + - Add an explicit allowlist for help/about domains if external links are needed. + - Deny `file:`, custom protocols, `javascript:`, and unexpected `http:`. +4. Add small unit or smoke tests if Electron test infrastructure exists; otherwise document manual packaged-app checks. + +Acceptance: + +- App navigation works in dev and packaged builds. +- Unexpected app navigation is prevented. +- External links are opened only when they match the policy. + +## Phase 7: Add Content Security Policy + +Addresses: + +- Low: no Content Security Policy is defined + +Files likely touched: + +- `index.html` +- possibly Vite config if separate dev/prod CSP handling is needed +- generated export code only if app-shell CSP breaks previews + +Tasks: + +1. Add an app-shell CSP meta tag. +2. Start with a policy that supports current app behavior: + - scripts from self + - images/media/fonts from self plus `data:`/`blob:` where required for user assets + - connections to self plus configured render server during development/export + - no object/embed +3. Verify the Electron renderer and browser app do not log CSP violations for normal workflows. +4. Keep generated HTML exports separate if they need a looser asset policy; do not silently weaken the app shell for exports. + +Acceptance: + +- App loads in browser dev, production preview, and Electron. +- Common import/export/preview workflows work without unexpected CSP violations. +- CSP blocks inline or remote script execution in the app shell. + +## Suggested Pull Request Order + +1. Render server exposure and authentication. +2. Render server job limits, body limits, and cleanup. +3. Container non-root user and Chromium sandbox work. +4. Shared limits plus render-request validation. +5. Project and `.ruw` import validation. +6. Server lockfile and Docker `npm ci`. +7. Production devtools gating. +8. Electron URL policy. +9. CSP. + +## Final Verification Checklist + +- `npm run build` +- `npm audit --omit=dev` +- `npm audit --prefix server --omit=dev` +- Render server rejects unauthenticated `/render`. +- Render server rejects disallowed `Origin`. +- Render server rejects oversized request bodies and excessive frame counts. +- Normal MP4 render still completes through the app. +- Malformed `.json` and `.ruw` files fail with friendly errors. +- Production bundle does not contain `src/devtools/tests.ts`. +- Packaged Electron app opens only expected app URLs and approved external links. diff --git a/dev_docs/security-review.md b/dev_docs/security-review.md new file mode 100644 index 0000000..afee86a --- /dev/null +++ b/dev_docs/security-review.md @@ -0,0 +1,174 @@ +# Security Review + +Date: 2026-06-05 + +Scope reviewed: + +- Vue/Vite renderer in `src/` +- Electron main/preload in `electron/` +- MP4 render server in `server/` +- Project import/export paths for `.json`, `.ruw`, backups, assets, fonts, and generated HTML exports +- Build/dependency posture at the repository root + +## Summary + +The desktop shell starts from a good Electron baseline: `contextIsolation: true`, `nodeIntegration: false`, `sandbox: true`, and a narrow preload bridge. I did not find direct Node exposure to the Vue renderer. + +The highest-risk area is the standalone MP4 render server. It accepts unauthenticated cross-origin render jobs, large request bodies, and attacker-controlled scene data, then renders it in Chromium with `--no-sandbox` and feeds frames to `ffmpeg`. That combination should be treated as a network-exposed compute/sandbox boundary until it is tightened. + +The second broad theme is input hardening. Project files and `.ruw` bundles are structurally checked, but not deeply schema-validated or bounded before the app uses canvas sizes, frame counts, assets, and timelines. A crafted file can likely cause heavy CPU/memory use in both browser and server render paths. + +## Findings + +### High: Render Server Is Unauthenticated And Cross-Origin + +Evidence: + +- `server/server.js:19` enables default `cors()`, allowing browser requests from any origin. +- `server/server.js:20` accepts JSON bodies up to `200mb`. +- `server/server.js:27` starts render jobs without authentication, CSRF protection, origin checks, rate limits, or concurrency limits. +- `docker-compose.yml:4-5` publishes port `6000` on the host. + +Impact: + +Any site reachable by a user's browser, or any network client that can reach the service, can submit expensive render jobs. This can exhaust CPU, memory, disk, Chromium processes, or `ffmpeg` work queues. If the server is ever exposed beyond localhost, it becomes a public unauthenticated render farm. + +Recommendations: + +- Bind the service to localhost by default, or require an explicit opt-in for network exposure. +- Replace default `cors()` with an allowlist for the app origin. +- Add authentication or a per-session token for `/render`, `/events`, `/file`, and `DELETE`. +- Add request rate limits, maximum active jobs, queue limits, and per-job timeouts. +- Lower the default body limit and reject requests before parsing when possible. + +### High: Chromium Renders Untrusted Data With Sandbox Disabled + +Evidence: + +- `server/server.js:126-134` launches Chromium with `--no-sandbox` and `--disable-setuid-sandbox`. +- `server/server.js:166-167` loads `render.html` and injects caller-provided scene data into the page. +- `server/Dockerfile:1-20` uses the default root user; no non-root `USER` is configured. + +Impact: + +The server renders attacker-controlled image/audio/video/SVG-like asset data in Chromium. If a browser engine, image decoder, font decoder, media decoder, or canvas path is exploited, disabling the Chromium sandbox and running the container as root increases the blast radius. + +Recommendations: + +- Run the container as a non-root user. +- Prefer enabling the Chromium sandbox. If the container needs extra setup, document and configure it rather than relying on `--no-sandbox`. +- Treat each render job as untrusted: use process-level timeouts, memory/CPU limits, and container hardening. +- Consider a separate locked-down worker container for renders. + +### Medium: Project And Bundle Imports Lack Deep Validation And Bounds + +Evidence: + +- `src/lib/projectFile.ts:39-67` checks only top-level `magic`, `canvas`, `scenes`, and `assets` shape before returning the imported object as `Project`. +- `src/lib/zipUtil.ts:121-171` parses ZIP entries into memory without explicit caps on entry count, central directory size, total extracted size, or path/name length. +- `server/server.js:115-119` derives `fps`, `duration`, and `totalFrames` directly from request data. +- `server/server.js:135-138` uses request-provided canvas dimensions as the Chromium viewport. + +Impact: + +A crafted `.json`, `.ruw`, backup, or server request can specify extreme canvas sizes, frame counts, layer/keyframe arrays, base64 assets, font data, or ZIP metadata. The likely outcome is denial of service: browser tab hangs, Electron renderer memory pressure, server CPU exhaustion, or failed/very slow exports. + +Recommendations: + +- Add a real project schema validator at file and server boundaries. +- Clamp canvas dimensions, duration, fps, layer counts, keyframe counts, asset count, asset byte size, and total project size. +- Add ZIP reader bounds and clearer errors for malformed/truncated archives. +- Normalize imported projects into a safe internal shape instead of trusting arbitrary object fields. + +### Medium: Render Server Dependency Installs Are Not Reproducible + +Evidence: + +- `server/package.json` exists, but no `server/package-lock.json` is present. +- `server/Dockerfile:15-16` copies only `package.json` and runs `npm install --omit=dev`. +- `npm audit --prefix server --omit=dev` cannot run without a lockfile. + +Impact: + +Server builds may pull different transitive dependency versions over time. That makes vulnerability tracking, incident response, and reproducible releases harder. + +Recommendations: + +- Commit `server/package-lock.json`. +- Use `npm ci --omit=dev` in the server Dockerfile. +- Add server dependency auditing to CI. + +### Medium: Development Test Overlay Ships In The Production Bundle + +Evidence: + +- `src/App.vue:38` statically imports `./devtools/TestOverlay.vue`. +- `src/App.vue:806` always mounts the component. +- `npm run build` output shows `src/devtools/tests.ts` pulled into the production bundle through static/dynamic import relationships. + +Impact: + +This increases production attack surface and bundle size. Even if the test runner is intended to open only from a development-only Electron menu item, the code is still shipped to every web and desktop user. + +Recommendations: + +- Gate the test overlay behind `import.meta.env.DEV`. +- Load it with `defineAsyncComponent` or dynamic import only in development. +- Ensure production builds do not include `src/devtools/tests.ts`. + +### Low: Electron Navigation And External Link Policy Could Be Stricter + +Evidence: + +- `electron/main.cts:40-44` opens any `http://` or `https://` URL externally. +- `electron/main.cts:47-51` uses string `startsWith` checks for navigation allow/deny decisions. + +Impact: + +This is not a direct Node compromise because the renderer is isolated. Still, if untrusted content ever reaches a clickable/openable path, arbitrary external links can be launched. String-prefix navigation checks are also easier to get wrong than parsed URL origin/path checks. + +Recommendations: + +- Prefer parsed URL checks over `startsWith`. +- Allow only expected origins in development. +- Consider allowing only `https:` external links, or an explicit domain allowlist for help/about links. + +### Low: No Content Security Policy Is Defined + +Evidence: + +- `index.html` and built `dist/index.html` have no CSP meta tag. + +Impact: + +Vue's default escaping reduces XSS risk, and Electron disables Node integration. A CSP would still provide defense in depth for the web app, generated asset previews, and Electron renderer by limiting script, media, image, and connection destinations. + +Recommendations: + +- Add a CSP for the app shell. +- Account for required `data:`/`blob:` sources for user assets and exports. +- Add Electron-specific CSP validation during packaged smoke tests. + +## Positive Notes + +- Electron uses `contextIsolation`, disables `nodeIntegration`, enables renderer sandboxing, and exposes only a small preload API. +- The preload bridge does not expose arbitrary IPC invocation or Node primitives. +- The Electron app denies `window.open` in-app and prevents most unexpected navigation. +- `.ruw` uses STORE-only ZIP handling, avoiding decompression bombs from deflated entries. +- The root production dependency audit completed with `0 vulnerabilities`. +- A shallow secret scan found no obvious committed `.env`, key, secret, PEM, or certificate files outside dependencies. + +## Verification Performed + +- `npm run build` passed. +- `npm audit --omit=dev` passed with `0 vulnerabilities`. +- `npm audit --prefix server --omit=dev` could not run because `server/` has no lockfile. + +## Suggested Remediation Order + +1. Lock down the render server: localhost binding, CORS allowlist, auth/token, rate limits, job limits, and timeouts. +2. Run render workers as non-root and remove Chromium `--no-sandbox` if feasible. +3. Add schema validation and resource bounds for imported projects, `.ruw` bundles, backups, and render requests. +4. Add `server/package-lock.json`, switch Docker to `npm ci --omit=dev`, and audit server dependencies in CI. +5. Remove devtools/test runner code from production bundles. +6. Add CSP and stricter Electron URL parsing/allowlists.