Restrict Markdown HTML sanitizer to an explicit attribute allowlist - #485
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b93b2261f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Hardens rendered Markdown sanitization by explicitly allowlisting HTML attributes and blocking script-capable attributes.
Changes:
- Adds an explicit attribute allowlist.
- Adds sanitizer security and preservation tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
app/models/html_scrubber.rb |
Defines permitted Markdown HTML attributes. |
test/models/html_scrubber_test.rb |
Tests dangerous-attribute removal and valid markup preservation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4b93b22 to
ef9d038
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef9d0387de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ef9d038 to
350164a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 350164a582
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fd4f637 to
756a1f2
Compare
|
Thanks — the automated reviews (Codex connector + Copilot) caught a real regression and a residual, both now addressed. Summary of where this landed: Feature regressions fixed. The first cut rebuilt the allowlist from Rails' 13-attribute default, which silently dropped attributes the app and authors rely on — most importantly the
Value-sensitive iframe attributes dropped (Copilot). Net: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 756a1f2882
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
HtmlScrubber set an allowed-tag list but left the attribute list unset, so sanitized Markdown fell back to Loofah's default attribute handling. That default permits every data-* attribute, which lets stored page content carry data-controller + data-action — a self-firing Stimulus controller — and execute arbitrary same-origin JavaScript on page view with no user interaction. Set the attribute list explicitly to Loofah's vetted safe-attribute set plus the media/embed attributes the extra allowed tags need. This keeps every safe attribute that was effectively allowed before (target, rel, dir, tabindex, aria-*, table/list layout, ...) while dropping the data-* wildcard, so data-controller, data-turbo-*, any other data-*, inline event handlers, and srcdoc are removed. URL-valued attributes keep their javascript:-scheme scrubbing; style keeps its CSS scrubbing. The value-sensitive iframe attributes allow/referrerpolicy/sandbox are not allowed. The MarkdownRenderer wires generated image anchors to the lightbox via data-action; only that one benign action value survives, so authored HTML can't bind arbitrary or auto-firing Stimulus/Turbo actions to the page's controllers. The lightbox reads the anchor's scrubbed href. Add a regression test asserting the scrubber strips event handlers, data-controller/data-turbo-* and foreign data-action, iframe srcdoc, value-sensitive iframe attributes, and javascript: URLs, while preserving the renderer's lightbox/header markup, ARIA, safe authored attributes, and formatting/media/table markup.
756a1f2 to
88e95ff
Compare
|
Independent adversarial pass (Codex, xhigh, read the pinned Loofah 2.25.2 / rails-html-sanitizer 1.7.1 / Nokogiri source): verdict is closes the data-* auto-execution class, no blocking finding — parent scrub runs before One in-scope test gap it flagged, now fixed in f15c09f: the foreign- Two non-blocking notes left as-is (pre-existing/intentional, out of scope for this XSS-class fix): |
Problem
HtmlScrubber(the sanitizer applied to rendered Markdown page content) sets an allowed-tag list but never sets an allowed-attribute list. WhenRails::Html::PermitScrubberhas no attribute list, it falls back to Loofah's default attribute handling, which permits alldata-*attributes.That fallback lets editor-authored page content persist a self-firing Stimulus controller — e.g.
data-controller+data-actionwired to a lifecycle event — and execute arbitrary same-origin JavaScript on page view, with no user interaction. Because the rendered content runs in the app origin, this escalates to acting as the viewer (including an admin viewer).Inline event handlers (
onload,onerror) andiframe srcdochappen to be stripped by the current Loofah default, but only implicitly — nothing in this app pins that behavior, so a dependency default shift would silently reopen the hole.Fix
Set an explicit attribute allowlist on
HtmlScrubber, containing only the attributes the permitted tags legitimately need (media playback controls, table/iframe layout attributes, and the previously-relied-onid/style).With an explicit allowlist,
PermitScrubberis deny-by-default:on*) — droppeddata-controller/data-action/data-turbo-*and all otherdata-*— droppediframe srcdoc— droppedhref,src) keep theirjavascript:-scheme scrubbingstylevalues continue to be CSS-scrubbedThis turns the sanitizer's safety from an implicit dependency default into an explicit, tested contract, and closes the
data-*script-execution vector.Tests
test/models/html_scrubber_test.rbasserts the scrubber strips event handlers,data-*gadget attributes,iframe srcdoc, andjavascript:URLs, while preserving legitimate formatting, media, and table markup.