[Security][Frontend][Go-live] Remove EPUB sanitizer bypass & harden client-side XSS sinks #409

New Issue

2026-07-05T20:24:38Z

spikerj commented

2026-07-05 20:24:38 +00:00

Context: Frontend; the reader audience is largely minors, so any XSS carries the in-memory session token.

Problem: Uploaded EPUB/book HTML is rendered with Angular's sanitizer explicitly bypassed → stored XSS. Two more DOM-XSS sinks compound it, and browser-side Keycloak admin APIs mean any XSS escalates to identity-provider takeover.

Evidence:

projects/spikersoft/src/app/_components/epub-reader/epub-reader.component.ts:43 (bypassSecurityTrustHtml), rendered via epub-content-pane.component.html:16 ([innerHTML])
libraries/features/dev-tools-diagram/src/lib/nodes/node-classes.ts:418-428 (new Function)
_components/terminal/terminal.component.ts:179-180 (innerHTML of user input)
libraries/keycloak-admin/src/lib/services/keycloak-base.service.ts:18,24-33 (browser-side Keycloak admin REST)

Fix: Render EPUB HTML through the default sanitizer (or DOMPurify server-side to an allowlist); sandbox custom-code nodes in a worker with no DOM/token; escape terminal output; move Keycloak admin behind the backend (relates to #298).

Acceptance criteria: No bypassSecurityTrust* on uploaded content; no new Function/innerHTML on user input; the SPA token never carries realm-admin scope.

Effort: M · Related: #298 (keycloak-admin specialization).

**Context:** Frontend; the reader audience is largely minors, so any XSS carries the in-memory session token. **Problem:** Uploaded EPUB/book HTML is rendered with Angular's sanitizer explicitly bypassed → stored XSS. Two more DOM-XSS sinks compound it, and browser-side Keycloak *admin* APIs mean any XSS escalates to identity-provider takeover. **Evidence:** - `projects/spikersoft/src/app/_components/epub-reader/epub-reader.component.ts:43` (`bypassSecurityTrustHtml`), rendered via `epub-content-pane.component.html:16` (`[innerHTML]`) - `libraries/features/dev-tools-diagram/src/lib/nodes/node-classes.ts:418-428` (`new Function`) - `_components/terminal/terminal.component.ts:179-180` (`innerHTML` of user input) - `libraries/keycloak-admin/src/lib/services/keycloak-base.service.ts:18,24-33` (browser-side Keycloak admin REST) **Fix:** Render EPUB HTML through the default sanitizer (or DOMPurify server-side to an allowlist); sandbox custom-code nodes in a worker with no DOM/token; escape terminal output; move Keycloak admin behind the backend (relates to #298). **Acceptance criteria:** No `bypassSecurityTrust*` on uploaded content; no `new Function`/`innerHTML` on user input; the SPA token never carries realm-admin scope. **Effort:** M · Related: #298 (keycloak-admin specialization).

spikerj added the agentic label 2026-07-05 20:24:38 +00:00

spikerj commented

2026-07-06 02:54:06 +00:00

Triage — the EPUB sanitizer bypass

The live XSS sink is projects/spikersoft/src/app/_components/epub-reader/epub-reader.component.ts:43:

readonly safeHtml = computed<SafeHtml>(() => this.sanitizer.bypassSecurityTrustHtml(this.htmlContent()));

htmlContent is server-supplied EPUB page HTML (ReaderService.getEpubPage() → content.html) rendered from user-uploaded books, so any <script>/onerror=/javascript: in a malicious EPUB executes → stored XSS on a minors platform.

Why it can't be a one-line "just sanitize": the bypass exists to preserve EPUB typography. Angular's built-in DomSanitizer.sanitize(SecurityContext.HTML, …) (and plain [innerHTML]) strips <style> blocks and many attributes, so dropping the bypass would regress book formatting — a core feature on a reading platform. The correct fix is DOMPurify with an EPUB-safe profile (allow structural + style/class, USE_PROFILES:{html:true}, FORBID_TAGS:['script'], FORBID_ATTR:[/^on/]), then bind the purified string. That adds a dependency and, critically, needs visual render verification against real EPUBs — not just a build/lint pass.

Other bypassSecurityTrustHtml usages audited — safe, leave alone:

contact.component.ts:147 — hardcoded SVG icon literals (trusted, not user input).
dev-tools-x86-playground/.../disassembly-panel.component.ts — disassembler output in a dev tool (controlled generation).

Recommendation: treat as a proper (reviewed) task: add DOMPurify + EPUB-safe config, a unit test asserting <script>/on* are stripped while <style>/class survive, and a manual render check of a formatted EPUB. Also worth confirming whether the backend sanitizes EPUB HTML at ingest (defense-in-depth — belongs regardless). Flagging rather than shipping a blind sanitizer change that could break the reader.

### Triage — the EPUB sanitizer bypass The live XSS sink is `projects/spikersoft/src/app/_components/epub-reader/epub-reader.component.ts:43`: ```ts readonly safeHtml = computed<SafeHtml>(() => this.sanitizer.bypassSecurityTrustHtml(this.htmlContent())); ``` `htmlContent` is **server-supplied EPUB page HTML** (`ReaderService.getEpubPage()` → `content.html`) rendered from user-uploaded books, so any `<script>`/`onerror=`/`javascript:` in a malicious EPUB executes → **stored XSS on a minors platform**. **Why it can't be a one-line "just sanitize":** the bypass exists to preserve EPUB typography. Angular's built-in `DomSanitizer.sanitize(SecurityContext.HTML, …)` (and plain `[innerHTML]`) **strips `<style>` blocks and many attributes**, so dropping the bypass would regress book formatting — a core feature on a reading platform. The correct fix is **DOMPurify with an EPUB-safe profile** (allow structural + `style`/`class`, `USE_PROFILES:{html:true}`, `FORBID_TAGS:['script']`, `FORBID_ATTR:[/^on/]`), then bind the purified string. That adds a dependency and, critically, needs **visual render verification** against real EPUBs — not just a build/lint pass. **Other `bypassSecurityTrustHtml` usages audited — safe, leave alone:** - `contact.component.ts:147` — hardcoded SVG icon literals (trusted, not user input). - `dev-tools-x86-playground/.../disassembly-panel.component.ts` — disassembler output in a dev tool (controlled generation). **Recommendation:** treat as a proper (reviewed) task: add DOMPurify + EPUB-safe config, a unit test asserting `<script>`/`on*` are stripped while `<style>`/`class` survive, and a manual render check of a formatted EPUB. Also worth confirming whether the **backend** sanitizes EPUB HTML at ingest (defense-in-depth — belongs regardless). Flagging rather than shipping a blind sanitizer change that could break the reader.

spikerj commented

2026-07-06 21:55:16 +00:00

Audited the two remaining DOM sinks — severities differ from the ticket; scoping so the real one gets the effort

Terminal innerHTML (terminal.component.ts:179-180) — downgrade to near-nil; recommend closing this sub-item.
Two independent reasons it isn't a live vector:

The #t2 target is a <textarea> (template confirms). innerHTML on a textarea is parsed in RCDATA — <img onerror=…>/<script> become literal text, they do not execute. (It also means the terminal's own {red}→<span> coloring never renders — the component is half-broken.)
TerminalComponent (app-terminal) is Storybook-only — no route/template in the app uses it (only *.stories.ts + *.spec.ts). The x86-playground has its own separate terminal-panel.
So there's no reachable XSS here. I'd rather flag it honestly than ship an escaping "fix" that implies a vuln that isn't there. (Happy to add the escape as pure defense-in-depth if you want the AC literally satisfied, but it's cosmetic.)

new Function(code) custom-code node (node-classes.ts:418-428) — this is the real one; keep it.

The dev-tools-diagram feature is routed/shipped (routes.ts:361, lazy DiagramComponent). The "Custom Code" node runs user text via new Function(code)() in the page context — full DOM + in-memory session token.
Execution is explicit (execution.service.play() → node.execute), not auto-on-load, and the file-import paths I found load JSON data payloads into nodes, not a serialized graph — so today's vector is closest to self-exec (you run code you typed), escalating to stored code-exec only if a full diagram graph (with a Custom Code node) can be shared+imported then run. Worth confirming whether graph import/share exists or is planned.
Correct fix per the ticket (sandbox in a Worker with no DOM/token) is an M-sized refactor (worker bundling under Nx/Angular, structured-clone of the result, timeout for infinite loops) that changes node behavior (code loses DOM access — appropriate for a "compute a result" node) and wants a browser/build smoke test, so I'm not blind-shipping it.

Recommendation: close the terminal sub-item as not-a-vuln; keep item 2 (custom-code) + item 1 (EPUB, DOMPurify — see prior comment) + item 4 (keycloak-admin behind backend, #298). I can implement the Worker sandbox for the custom-code node on a go-ahead if someone can run a browser check that the diagram still executes custom-code nodes after the change.

### Audited the two remaining DOM sinks — severities differ from the ticket; scoping so the real one gets the effort **Terminal `innerHTML` (`terminal.component.ts:179-180`) — downgrade to near-nil; recommend closing this sub-item.** Two independent reasons it isn't a live vector: 1. The `#t2` target is a **`<textarea>`** (template confirms). `innerHTML` on a textarea is parsed in **RCDATA** — `<img onerror=…>`/`<script>` become literal text, they do **not** execute. (It also means the terminal's own `{red}`→`<span>` coloring never renders — the component is half-broken.) 2. `TerminalComponent` (`app-terminal`) is **Storybook-only** — no route/template in the app uses it (only `*.stories.ts` + `*.spec.ts`). The x86-playground has its own separate `terminal-panel`. So there's no reachable XSS here. I'd rather flag it honestly than ship an escaping "fix" that implies a vuln that isn't there. (Happy to add the escape as pure defense-in-depth if you want the AC literally satisfied, but it's cosmetic.) **`new Function(code)` custom-code node (`node-classes.ts:418-428`) — this is the real one; keep it.** - The `dev-tools-diagram` feature **is routed/shipped** (`routes.ts:361`, lazy `DiagramComponent`). The "Custom Code" node runs user text via `new Function(code)()` in the page context — full DOM + in-memory session token. - Execution is **explicit** (`execution.service.play()` → `node.execute`), not auto-on-load, and the file-import paths I found load **JSON data payloads into nodes**, not a serialized graph — so today's vector is closest to **self-exec** (you run code you typed), escalating to **stored code-exec only if a full diagram graph (with a Custom Code node) can be shared+imported then run**. Worth confirming whether graph import/share exists or is planned. - Correct fix per the ticket (sandbox in a Worker with no DOM/token) is an **M-sized refactor** (worker bundling under Nx/Angular, structured-clone of the result, timeout for infinite loops) that changes node behavior (code loses DOM access — appropriate for a "compute a result" node) and **wants a browser/build smoke test**, so I'm not blind-shipping it. **Recommendation:** close the terminal sub-item as not-a-vuln; keep item 2 (custom-code) + item 1 (EPUB, DOMPurify — see prior comment) + item 4 (keycloak-admin behind backend, #298). I can implement the Worker sandbox for the custom-code node on a go-ahead if someone can run a browser check that the diagram still executes custom-code nodes after the change.

spikerj commented

2026-07-07 15:31:04 +00:00

Item 1 (EPUB sanitizer bypass) implemented — spikersoft-angular PR #148 (open, for review).

Per the prescribed fix: uploaded EPUB HTML now goes through DOMPurify with an EPUB-safe profile before bypassSecurityTrustHtml (which now wraps an already-purified string). New pure epub-html-sanitizer.ts + 6 unit tests pinning the contract — <script>/on*/javascript:/iframe·object·embed·form stripped, while <style> blocks + class/style + structural markup are preserved (FORCE_BODY: true stops a leading <style> being hoisted to <head> and dropped). Adds the dompurify dep. nx lint+build+test spikersoft (1869) green.

Needs your ~2-min visual check that a formatted EPUB still renders (typography preserved) before merge. CSS caveat: url("javascript:…") inside a <style> survives DOMPurify but is inert in modern browsers; full CSS isolation would need a sandboxed iframe (out of scope).

Remaining sub-items unchanged: item 2 (new Function custom-code node → Worker sandbox, M-sized, needs a browser smoke test — happy to take on a go-ahead); item 3 (terminal innerHTML) recommend closing as not-a-vuln (textarea RCDATA + Storybook-only); item 4 (Keycloak-admin behind backend, ties to #298).

**Item 1 (EPUB sanitizer bypass) implemented — spikersoft-angular PR #148 (open, for review).** Per the prescribed fix: uploaded EPUB HTML now goes through DOMPurify with an EPUB-safe profile before `bypassSecurityTrustHtml` (which now wraps an already-purified string). New pure `epub-html-sanitizer.ts` + 6 unit tests pinning the contract — `<script>`/`on*`/`javascript:`/`iframe`·`object`·`embed`·`form` stripped, while `<style>` blocks + `class`/`style` + structural markup are preserved (`FORCE_BODY: true` stops a leading `<style>` being hoisted to `<head>` and dropped). Adds the `dompurify` dep. `nx lint`+`build`+`test spikersoft` (1869) green. Needs your ~2-min visual check that a formatted EPUB still renders (typography preserved) before merge. **CSS caveat:** `url("javascript:…")` inside a `<style>` survives DOMPurify but is inert in modern browsers; full CSS isolation would need a sandboxed iframe (out of scope). **Remaining sub-items unchanged:** item 2 (`new Function` custom-code node → Worker sandbox, M-sized, needs a browser smoke test — happy to take on a go-ahead); item 3 (terminal `innerHTML`) recommend closing as **not-a-vuln** (textarea RCDATA + Storybook-only); item 4 (Keycloak-admin behind backend, ties to #298).

spikerj commented

2026-07-07 15:38:26 +00:00

Item 2 threat-model resolved (code analysis) — it's self-exec only, no stored/shared vector.

Audited the dev-tools-diagram feature for any path that could persist/import/share a full graph (which would turn the new Function Custom Code node into stored code-exec against another user):

No graph serialize/deserialize exists. diagram.component.ts's only localStorage use is loadNumber/persistNumber (panel-size prefs), not the rete graph.
The file-import (FileReader.readAsText) + IndexedDB (QuickTypeJSONStorage) paths load QuickType JSON data into nodes for type generation — not a serialized graph, and never the Custom Code node's code control.
The node's code comes solely from the live InputControl the user types into in their own session (node-classes.ts:418-428).

Conclusion: the Custom Code node is self-XSS only — you run JS you typed, in your own session, on the /tools dev-diagram route. There is no attacker→victim vector (nothing an attacker authors is stored or delivered to another user). Severity is low, not the stored-XSS class of item 1.

Recommendation (your call): given that, the Worker-sandbox refactor (item 2) is a behavior change (Custom Code loses DOM access) for low security benefit, and its worker-bundling core can't be unit-verified. Options: (a) accept as documented dev-tool behavior and close item 2; (b) lightweight mitigation — leave a comment/guard noting it's intentional self-exec; (c) still sandbox it for defense-in-depth on a go-ahead (I'd extract a unit-testable orchestrator + you run the browser smoke test). I'd lean (a)/(b) unless a graph share/import feature is planned — say which and I'll execute.

**Item 2 threat-model resolved (code analysis) — it's self-exec only, no stored/shared vector.** Audited the `dev-tools-diagram` feature for any path that could persist/import/share a full graph (which would turn the `new Function` Custom Code node into *stored* code-exec against another user): - **No graph serialize/deserialize exists.** `diagram.component.ts`'s only `localStorage` use is `loadNumber`/`persistNumber` (panel-size prefs), not the rete graph. - The file-import (`FileReader.readAsText`) + IndexedDB (`QuickTypeJSONStorage`) paths load **QuickType JSON *data* into nodes** for type generation — not a serialized graph, and never the Custom Code node's `code` control. - The node's `code` comes solely from the live `InputControl` the user types into in their own session (`node-classes.ts:418-428`). **Conclusion:** the Custom Code node is **self-XSS only** — you run JS you typed, in your own session, on the `/tools` dev-diagram route. There is no attacker→victim vector (nothing an attacker authors is stored or delivered to another user). Severity is **low**, not the stored-XSS class of item 1. **Recommendation (your call):** given that, the Worker-sandbox refactor (item 2) is a **behavior change** (Custom Code loses DOM access) for **low** security benefit, and its worker-bundling core can't be unit-verified. Options: (a) **accept as documented dev-tool behavior** and close item 2; (b) **lightweight mitigation** — leave a comment/guard noting it's intentional self-exec; (c) **still sandbox it** for defense-in-depth on a go-ahead (I'd extract a unit-testable orchestrator + you run the browser smoke test). I'd lean (a)/(b) unless a graph share/import feature is planned — say which and I'll execute.

spikerj commented

2026-07-07 16:28:40 +00:00

Item 1 (EPUB sanitizer bypass) merged to master — spikersoft-angular PR #148. Uploaded EPUB HTML now sanitized through DOMPurify (EPUB-safe profile) before bypassSecurityTrustHtml; 6 unit tests pin the contract. The headline stored-XSS sink is closed.

Keeping this ticket open for the remaining sub-items:

Item 2 (new Function custom-code node): threat model resolved to self-exec only (no graph share/import path) — recommend accept/close or lightweight guard rather than the Worker-sandbox refactor. Your call (see prior comment).
Item 3 (terminal innerHTML): not-a-vuln (textarea RCDATA + Storybook-only) — recommend close.
Item 4 (Keycloak-admin behind backend): separate, ties to #298.

If you're happy with the recommendations on 2/3, this reduces to item 4 and can likely be narrowed to that.

**Item 1 (EPUB sanitizer bypass) merged to `master`** — spikersoft-angular PR #148. Uploaded EPUB HTML now sanitized through DOMPurify (EPUB-safe profile) before `bypassSecurityTrustHtml`; 6 unit tests pin the contract. The headline stored-XSS sink is closed. Keeping this ticket **open** for the remaining sub-items: - **Item 2** (`new Function` custom-code node): threat model resolved to **self-exec only** (no graph share/import path) — recommend accept/close or lightweight guard rather than the Worker-sandbox refactor. Your call (see prior comment). - **Item 3** (terminal `innerHTML`): **not-a-vuln** (textarea RCDATA + Storybook-only) — recommend close. - **Item 4** (Keycloak-admin behind backend): separate, ties to #298. If you're happy with the recommendations on 2/3, this reduces to item 4 and can likely be narrowed to that.

spikerj referenced this issue

2026-07-17 04:50:03 +00:00

[Reader][Social] Frontend: profile Social tab, reader Community tab, upvotes, reading-list dialog #634

spikerj commented

2026-07-22 02:58:14 +00:00

Board-sweep status (2026-07-22): item 1 merged (EPUB DOMPurify, angular #148). REMAINING: the new-Function node + the other DOM-XSS / keycloak-admin sinks.

spikerj referenced this issue

2026-07-27 02:46:14 +00:00

Research: Replace / drop npm libs with built-in browser APIs (2026) to slim spikersoft-angular #866

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: spikerj/spikersoft-issues#409