Stop document content from drawing form controls

[inquinity]

Updated after review. Two findings from @mfauzaan, both correct: the
first version deleted the app's own Mermaid controls, and my explanation of
disabled described a mechanism that does not exist. Both are fixed in
044e3f6, and the framing below is corrected too — this is optional content
and UI hardening, not a demonstrated credential-theft fix.

A document can draw what looks like a credential prompt.

<form action="https://example.invalid/collect" method="post">
  <input name="password" type="password">
  <button type="submit">Sign in</button>
</form>

Open that and you get a password field in the middle of the page.

Why the existing guard misses it

SANITIZE_CONFIG already lists form in FORBID_TAGS, and that part works —
the <form> element is removed. But DOMPurify defaults to
KEEP_CONTENT: true, which unwraps a forbidden element and reparents its
children. The <input> survives on its own.

So the check that looks like it covers this covers the wrapper, not the
controls, and the controls are what the reader sees.

How bad

Modest, and I would rather say so than oversell it. There is no exfiltration
path: without the form there is nothing to submit to, and neither the sanitiser
nor the CSP leaves any script that could read the field. It is spoofing rather
than theft — a document that can render a convincing login box inside your
Markdown viewer. Treat this as optional hardening; nothing here is urgent.

The fix

Forbid the controls that invite typing, and drop any <input> that is not a
checkbox through a hook. input cannot be forbidden outright, because
EscapingHTMLFormatter emits one per task-list item.

button is deliberately left allowed. The first version of this patch
forbade it, which deleted the five Mermaid HUD controls — MarkdownHTML+Mermaid
emits them as article HTML, so they pass through the sanitiser like document
content. Special-casing them by class would not work either: the class comes
from the document. And the trade was never worth it. A document-authored
<button> with no <form> behind it and no script that can run is inert —
clicking it does nothing. input is the tag that carries the risk.

One thing in here is less obvious than it looks:

• The hook has to cover both sanitize paths. MdPreview.update calls
  DOMPurify.sanitize directly with SANITIZE_DOM_CONFIG rather than going
  through sanitize(), so hooking only the latter protects the cold render and
  leaves the hot path — every file change and editor exit — open.

The rule also does not key on disabled. An earlier version of this
description claimed DOMPurify strips that attribute. That was wrong. DOMPurify
preserves it; enableTaskCheckboxes() clears it afterwards, gated on the host
bridge — which is why task checkboxes stay inert in Quick Look. Matching on the
attribute would depend on which side of that call the sanitiser ran, which is
the actual reason to avoid it.

Tests

SanitizerFormControlTests drives the shipped sanitiser in a WKWebView,
following the pattern in MdPreviewUpdateTests, and asserts three things: no
typing surface survives the credential prompt, task-list checkboxes still render
and stay clickable, and the five Mermaid controls survive.

Mutation-checked, and the review exposed a gap in how I had done that. Removing
the FORBID_TAGS additions used to fail nothing, because the test markdown only
contained a form, an input and a button — eight of the nine forbidden tags had
no coverage, so the assertion passed on zero. The markdown now exercises all of
them. Three mutations are checked and each fails a test: re-adding button,
emptying the added tags, and neutering the input hook.

The reason the HUD removal went unnoticed is worth stating plainly: every
assertion I wrote checked what must be absent, so deleting the application's
own UI could not fail one.

Disclosure

Reported publicly rather than through the advisory process, deliberately. The
containment issue was different: unauthenticated arbitrary file read, unfixed,
so it went through private reporting as GHSA-vgmc-h5g6-xh2q. This one has no
exfiltration path, the patch is attached, and the analysis is already public in
my fork's history. Private disclosure would not have protected anyone. If you
would rather handle it the other way, say so and I will follow your preference
next time.

Same reporter as GHSA-vgmc-h5g6-xh2q, #337, #339, #343 and #368.

[mfauzaan]

The submitted example looks like optional UI hardening rather than a demonstrated credential-exfiltration issue. I tested the exact prompt against the original renderer in WKWebView: the form is removed, both the password field and button have form === null, and entering a dummy value and clicking Sign in produces no navigation/submission attempt on either article-rendering path. A document author already controls the document's text, images, and links; the appearance of a login prompt alone does not demonstrate a new application capability or permission-boundary crossing.

There is precedent for allowing sanitized HTML while separately disabling form submission. VS Code's Markdown preview explicitly sets enableForms: false and disables document scripts by default. That supports keeping submission and execution restricted, but does not establish a need to remove every visible form control here.

Removing interactive HTML controls could still be a product policy we choose. As written, however, this patch breaks the app-generated Mermaid controls, and its explanation of checkbox enabling is incorrect; details inline. I would address those before considering this for merge, and describe the change as optional content/UI hardening rather than a demonstrated credential-theft fix.

[mfauzaan]

[P2] Preserve the app-generated Mermaid buttons

This also removes the <button> elements emitted by MarkdownHTML+Mermaid.swift for zoom out, reset, zoom in, fill width, and open in window. I rendered a simple graph TD; A-->B diagram through the shipped renderer and host bridge in WKWebView: document.querySelectorAll('.mermaid-hud button').length is 5 with the original sanitizer and 0 with this patch. The submitted tests pass while these controls disappear. Please preserve the app-generated controls and add coverage for initial rendering and subsequent updates. Creating trusted controls after sanitizing document content would avoid treating an author-supplied class name as a security exception.

[inquinity]

You are right, and thank you.

Confirmed here: .mermaid-hud button is 5 before the patch and 0 after. It also means this shipped broken in two releases of my fork before your review caught it.

Fixed in 044e3f6 by dropping button from FORBID_TAGS entirely. I did not take the class-based route, for the reason you gave: mermaid-hud comes from the document as far as the sanitizer is concerned, so it cannot carry a security decision.

Reconsidering the trade, forbidding the tag was not worth much anyway. A document-authored <button> with no <form> behind it and no script that can run is inert — clicking it does nothing. input is the tag that matters, because a field is what invites typing, and that restriction stays.

On coverage: testMermaidControlsSurviveSanitisation now asserts all five controls remain, and re-adding button to FORBID_TAGS fails it. Both paths are exercised, since render() goes through MdPreview.update(), which is the morphdom path.

The deeper reason nothing caught this is that every assertion I wrote checked what must be absent. Deleting the application's own UI could not fail a test like that. Worth stating plainly, because the same shape hid a second problem you did not see: the test markdown only ever contained a form, an input and a button, so eight of the nine forbidden tags had no coverage at all — removing them from the list passed on zero. That is fixed too.

[mfauzaan]

[P3] Correct the explanation of checkbox enabling

The bundled DOMPurify preserves disabled: sanitizing <input type=checkbox disabled> returns <input type="checkbox" disabled="">. It is the existing enableTaskCheckboxes() function, called by MdPreview.update(), that sets checkbox.disabled = false. The new test observes the DOM after that function has run, so it does not show that DOMPurify removed the attribute. Please correct this comment, the matching test explanation, and the PR description; the desired clickable behavior is valid, but the stated mechanism is not.

[inquinity]

Correct, and the mechanism I described does not exist. DOMPurify preserves disabled; enableTaskCheckboxes() clears it afterwards, gated on the host bridge — which is also why the checkboxes stay inert in Quick Look, where there is nothing to write a change back to.

Your diagnosis of how I got it wrong is exact: I read the final DOM and attributed what I saw to the sanitizer without asking what else had touched it in between.

Corrected in 044e3f6 — the code comment, the test explanation and the PR description. The comment now says the rule must not key on disabled because matching it would depend on which side of enableTaskCheckboxes() the sanitizer happened to run, which is the real reason to avoid it.

[mfauzaan]

Thanks for working through the review and fixing the Mermaid regression.

After considering this further, I’ve decided to keep the current sanitized HTML behavior. The example doesn’t demonstrate credential submission, and I don’t think we need the additional restrictions on visible controls for this app and make the app complex on a lower handing this :)

I’m going to close this PR. I appreciate the investigation and the time you spent addressing the feedback. The narrower asset-containment work in #337 is still something I’d like to move forward with.