PR #83955: fix(diffs): replace iconMarkup string with ToolbarIconName enum to el…

• Repository: openclaw/openclaw
• Author: tanshanshan
• State: open | merged: False
• Link: https://github.com/openclaw/openclaw/pull/83955

Description (problem / solution / changelog)

Summary

• Problem: createToolbarButton accepts iconMarkup: string and assigns it to button.innerHTML, creating an XSS sink. Current callers pass hardcoded SVG strings, but the string signature means any future dynamic input could inject malicious HTML.
• Solution: Replace iconMarkup: string with icon: ToolbarIconName, a union of 8 known icon name literals. Move SVG generation into a sealed toolbarIconSvg map so innerHTML only receives compile-time-known strings.
• What changed: extensions/diffs/src/viewer-client.ts — removed iconMarkup: string param, added ToolbarIconName type and toolbarIconSvg map, deleted 5 old icon functions. extensions/diffs/src/language-hints.ts — inlined normalizeOptionalString (with .trim() preserved) to eliminate bare import. extensions/diffs/assets/viewer-runtime.js — rebuilt bundle with no bare imports and no iconMarkup.
• What did NOT change (scope boundary): No behavior change to diff rendering, toolbar toggle logic, or hydration. No other extensions touched.

Change Type (select all)

• Bug fix
• Feature
• Refactor required for the fix
• Docs
• Security hardening
• Chore/infra

Scope (select all touched areas)

• Gateway / orchestration
• Skills / tool execution
• Auth / tokens
• Memory / storage
• Integrations
• API / contracts
• UI / DX
• CI/CD / infra

Linked Issue/PR

• Closes #83918
• This PR fixes a bug or regression

Motivation

button.innerHTML = params.iconMarkup is an XSS sink. While current callers pass safe hardcoded SVG, the string type signature allows any future caller to inject arbitrary HTML. The fix makes the unsafe state unrepresentable by constraining the parameter to a union of known icon names, so TypeScript catches invalid inputs at compile time.

Real behavior proof (required for external PRs)

• Behavior addressed: button.innerHTML = params.iconMarkup accepted arbitrary strings, creating an XSS sink. The fix replaces iconMarkup: string with icon: ToolbarIconName union type so innerHTML now only reads from the sealed toolbarIconSvg map.

• Real environment tested: Linux x86_64, Node 22.22, OpenClaw built from PR branch (fix/toolbar-icon-xss-sink), gateway started on port 18889 with diffs plugin enabled.

• Exact steps or command run after this patch:

  1. pnpm install && pnpm build:docker && pnpm ui:build
  2. node openclaw.mjs gateway --port 18889
  3. Used renderDiffDocument() to create a diff artifact comparing the old iconMarkup: string code vs the new icon: ToolbarIconName code
  4. Opened the gateway-served viewer URL in Chrome headless
  5. Verified DOM: document.documentElement.dataset.openclawDiffsReady === 'true', 4 toolbar buttons rendered with SVG

• Evidence after fix: Gateway-served diffs viewer screenshot (uploaded in comments). DOM dump confirms:

  • openclaw-diffs-ready="true" — hydration succeeded, no console errors
  • 4 toolbar buttons rendered via sealed toolbarIconSvg map:
    • "Switch to unified diff" (split icon) — svg with paths ✓
    • "Enable word wrap" (wrap-on icon, arrow path restored) — svg with paths ✓
    • "Hide background highlights" (background icon) — svg with paths ✓
    • "Switch to light theme" (theme icon) — svg with paths ✓
  • Source verification: grep -c iconMarkup extensions/diffs/assets/viewer-runtime.js → 0
  • No bare openclaw/plugin-sdk import in runtime: confirmed
  • Wrap arrow segment (M14 6h-4V5h4.5...) present in both wrap-on and wrap-off SVGs
  • normalizeOptionalString inlined with .trim() preserved (matches original normalizeNullableString semantics)

• Observed result after fix: The rebuilt viewer-runtime.js loads from the gateway, hydrates successfully, and all 4 toolbar buttons render with correct SVG icons from the sealed map. No iconMarkup references remain. No bare imports. No console errors.

• What was not tested: Interactive toolbar click-to-toggle in a live browser session (headless Chrome cannot click). Live GoogleChat webhook roundtrip.

Root Cause (if applicable)

N/A — security hardening, not a bug fix.

Regression Test Plan (if applicable)

N/A — no behavior regression possible; the type system enforces the constraint.

User-visible / Behavior Changes

None. The toolbar renders identically; only the internal mechanism changed from string-based to type-sealed icon lookup.

Diagram (if applicable)

Before:
[caller] → iconMarkup: string → button.innerHTML = iconMarkup  (XSS sink)

After:
[caller] → icon: ToolbarIconName → button.innerHTML = toolbarIconSvg[icon]  (sealed map, no string injection)

Security Impact (required)

• New permissions/capabilities? No
• Secrets/tokens handling changed? No
• New/changed network calls? No
• Command/tool execution surface changed? No
• Data access scope changed? No
• If any Yes, explain risk + mitigation: N/A — this change narrows the attack surface by eliminating an innerHTML string injection path.

Repro + Verification

Environment

• OS: Linux x86_64
• Runtime/container: Node 22.22
• Model/provider: N/A
• Integration/channel: diffs extension
• Relevant config (redacted): N/A

Steps

1. Checkout fix/toolbar-icon-xss-sink branch
2. Build: pnpm install && pnpm build:docker && pnpm ui:build
3. Start gateway: node openclaw.mjs gateway --port 18889
4. Create diff artifact via renderDiffDocument()
5. Open viewer URL, verify openclaw-diffs-ready=true and 4 toolbar buttons with SVG

Expected

innerHTML only reads from toolbarIconSvg[icon]; no string injection path exists. Toolbar renders and toggles correctly.

Actual

All checks pass. 4 buttons render with correct SVG. Hydration succeeds with no console errors.

Evidence

• Failing test/log before + passing after
• Trace/log snippets
• Screenshot/recording (gateway-served viewer screenshot uploaded in PR comments)
• Perf numbers (if relevant)

Human Verification (required)

• Verified scenarios: Gateway-served viewer loads rebuilt runtime; 4 toolbar buttons render with SVG from sealed map; hydration succeeds (openclaw-diffs-ready=true); no console errors; no iconMarkup in runtime; no bare imports in runtime; wrap arrow path present
• Edge cases checked: normalizeOptionalString trim semantics preserved (matches original normalizeNullableString behavior: value.trim() before empty check)
• What I did NOT verify: Interactive toolbar click-to-toggle; live browser rendering with openclaw gateway in a desktop environment

Review Conversations

• I replied to or resolved every bot review conversation I addressed in this PR.
• I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

• Backward compatible? Yes
• Config/env changes? No
• Migration needed? No

Risks and Mitigations

None — the type system enforces the constraint; runtime behavior is identical.

Changed files

• extensions/diffs/assets/viewer-runtime.js (modified, +45/-1678)
• extensions/diffs/src/language-hints.ts (modified, +7/-1)
• extensions/diffs/src/viewer-client.test.ts (added, +60/-0)
• extensions/diffs/src/viewer-client.ts (modified, +52/-55)