Skip to content

[codex] Harden preview script boundaries#384

Merged
schuyler merged 4 commits into
schuyler:mainfrom
yusufm:Codex/harden-preview-security-019da736-8573-7ac1-9883-259ce39e964a
Apr 19, 2026
Merged

[codex] Harden preview script boundaries#384
schuyler merged 4 commits into
schuyler:mainfrom
yusufm:Codex/harden-preview-security-019da736-8573-7ac1-9883-259ce39e964a

Conversation

@yusufm

@yusufm yusufm commented Apr 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What changed

This PR hardens the main app preview against document-supplied script execution.

  • adds a preview-only Content Security Policy to the rendered HTML template
  • mints a per-document token for the x-macdown-checkbox bridge and keeps it stable across DOM-only preview refreshes
  • requires that token before applying checkbox toggles back into the source document
  • keeps preview-only security metadata out of HTML exports
  • adds focused renderer and document-level tests for the CSP and checkbox bridge token behavior

Why it changed

The preview currently renders Markdown into a live WebView and injects body content directly into the DOM. That makes the preview an active execution surface for attacker-controlled HTML/JS. The checkbox bridge was also callable by any preview script because it trusted every x-macdown-checkbox://toggle/... navigation.

This patch contains the first hardening slice by constraining which scripts can execute in the preview and by making the checkbox bridge non-forgeable from static document content.

Related to #security-review

Impact

  • untrusted inline script and event handlers in preview content are blocked by CSP
  • bundled preview features continue to work through explicitly allowed script sources
  • checkbox toggles now require the active bridge token minted during preview rendering
  • DOM-only preview refreshes keep checkbox toggles working because the token remains stable for the document
  • HTML export output is unchanged by the preview-only protections

Validation

  • added renderer tests covering preview CSP emission, stable token behavior, and export behavior
  • added document-level tests covering accepted and rejected checkbox bridge tokens
  • ran xcodebuild test -workspace "MacDown 3000.xcworkspace" -scheme MacDown -destination 'platform=macOS'
  • result: 854 tests passed, 0 failures

@yusufm yusufm marked this pull request as ready for review April 19, 2026 19:55
@schuyler Claude

schuyler commented Apr 19, 2026

Copy link
Copy Markdown
Owner

Thanks for this — the layered CSP + bridge-token approach is the right shape, and catching the pre-existing <title> escape hole is a nice bonus.

One correctness issue that needs addressing before merge: the checkbox token lives in <head>, but the common render path in MPDocument.m (~line 1396) does body.innerHTML = bodyContent and never refreshes <head>. Meanwhile [MPRenderer render] rotates checkboxBridgeToken on every call. So after the first keystroke-triggered render, the DOM's meta tag holds the original token while the renderer holds a new one, and every subsequent checkbox click is silently rejected by handleCheckboxToggle:. The renderer-only tests don't catch this because they don't exercise the DOM-replacement lifecycle.

Simplest fix: mint the token once (e.g., lazily on first render) and stop rotating per-render — a 122-bit UUID per document is plenty given the CSP already blocks document scripts from reading the meta tag. Alternatives are updating the meta tag inside the DOM-replacement JS, or moving the tag into <body>.

Would also be great to add a document-level test that renders twice, then invokes handleCheckboxToggle: with the token actually present in the WebView's DOM, to pin the contract end-to-end.

A couple of smaller notes: script-src 'unsafe-eval' and style-src 'unsafe-inline' materially weaken the CSP — worth a one-line comment naming which bundled features force each (MathJax? Prism?). And connect-src http: https: is broader than it needs to be given script-src restrictions.

Thanks again!

Generated by Claude Code

@yusufm

yusufm commented Apr 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Addressed the token-desync review comment in 9b03a22. The checkbox bridge token is now minted once and kept stable across DOM-only preview refreshes, and I added document-level tests covering both accepted and rejected tokenized checkbox toggles. I also added a short code comment explaining the current / CSP allowances. Validation is current on this branch.

@yusufm

yusufm commented Apr 19, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the token-desync review comment in 9b03a22.

The checkbox bridge token is now minted once and kept stable across DOM-only preview refreshes, and I added document-level tests covering both accepted and rejected tokenized checkbox toggles. I also added a short code comment explaining the current unsafe-eval / unsafe-inline CSP allowances.

Validation is current on this branch: xcodebuild test -workspace "MacDown 3000.xcworkspace" -scheme MacDown -destination 'platform=macOS' passed with 854 tests and 0 failures.

@schuyler Claude

schuyler commented Apr 19, 2026

Copy link
Copy Markdown
Owner

Thanks for turning this around so quickly — the mint-once token, the CSP caveat comment, and the document-level tests for both the accept and reject paths are exactly what was needed. Merging now.

Generated by Claude Code

@schuyler schuyler merged commit 26233df into schuyler:main Apr 19, 2026
5 checks passed
@schuyler schuyler added the rc-pending Included in a release candidate awaiting validation label Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

rc-pending Included in a release candidate awaiting validation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yusufm @schuyler