Give rich text one policy instead of two mirrored copies - #20
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
This was referenced Aug 5, 2026
The Worker and the browser each carried their own rich-text module. Both files
opened with a comment telling the reader to keep them in sync by hand, and the
flattening function had already drifted in name — richTextToPlain on the Worker,
richTextToPlainText in the browser — while staying byte-identical in body.
The two sanitizers stay separate, because they have to: HTMLRewriter is the
Workers runtime's native streaming parser, DOMPurify needs a real DOM, and
neither runs where the other does. Sharing them was never the option.
What moves to core is everything they agree on and must not drift:
RICH_TEXT_TAGS the 15-tag allow-list, previously duplicated exactly
MEDIA_TAGS the "no embedded media" product rule
SAFE_LINK_PROTOCOLS http/https/mailto
isSafeLink incl. stripping the control characters that make
`java\tscript:` navigate while passing a prefix check
LINK_TARGET/LINK_REL what a surviving link is rewritten to carry
richTextToPlain and richTextLength/isRichTextEmpty/richTextToLine/
isRichTextHtml on top of it
richTextToPlain is the surviving name; richTextToPlainText had no callers
outside its own module, so nothing had to change to adopt it.
One deliberate behaviour change: the browser's FORBID_TAGS is now composed from
the shared media list, which makes it a strict superset of what it was — nothing
is lost, and it gains `track` and `math`, aligning it with the Worker.
Adds 18 core tests covering link safety and the browser-side helpers, which had
no tests at all while they lived in apps/web. The Worker's 20 sanitizer tests
are unchanged and still pass, which is what actually guards this.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
playforge-coding
force-pushed
the
core-reconcile-richtext
branch
from
August 5, 2026 23:30
c2402b6 to
4c55a35
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fourth extraction layer. Stacked on #19. This one touches a security boundary — worth reading the diff rather than the summary.
What was actually wrong
Both files opened with a comment telling the reader to keep them in sync by hand, and the flattening function had already drifted in name —
richTextToPlainon the Worker,richTextToPlainTextin the browser — while staying byte-identical in body. The drift had a visible failure mode: the editor's character counter and the Worker's length limit must measure the same thing, or the editor shows "498/500" for a comment the server then rejects.The sanitizers stay separate
This does not merge them, and couldn't. HTMLRewriter is the Workers runtime's native streaming parser; DOMPurify needs a real DOM. Neither runs where the other does, and the two-pass design (Worker on write, browser on render) is deliberate — the render path uses
dangerouslySetInnerHTML, so a reader's safety shouldn't depend on every stored row having been written by the current server code.What moved
RICH_TEXT_TAGSMEDIA_TAGSSAFE_LINK_PROTOCOLSisSafeLinkjava\tscript:can't pass a prefix checkLINK_TARGET/LINK_RELrichTextToPlain+ helpersrichTextToPlainis the surviving name —richTextToPlainTexthad no callers outside its own module, so adopting it cost nothing.One deliberate behaviour change
The browser's
FORBID_TAGSis now composed from the shared media list. Verified to be a strict superset of what it was: nothing lost, gainstrackandmath, aligning it with the Worker. (It was already documented as "redundant given the allow-list" — the allow-list is the real enforcement either way.)Verification
The Worker's 20 sanitizer tests — media smuggling,
<scr<script>ipt>,onerror=,javascript:,java\tscript:, protocol-relative hrefs — are unchanged and still pass. That suite is what actually guards this change; I did not touch it.Adds 18 core tests covering link safety and the browser-side helpers (
richTextToLine,isRichTextHtml,richTextLength), which had no tests at all while they lived inapps/web.pnpm run lint,pnpm run build,pnpm run build:docspass; core 33, api 27, mcp 14.Stack created with GitHub Stacks CLI • Give Feedback 💬