Repository navigation
fix(sanitizer): merge sanitize rules for the same tag instead of overwriting them - #3041
Open
playlikeboy wants to merge 2 commits into
Open
playlikeboy wants to merge 2 commits into
playlikeboy wants to merge 2 commits into
Conversation
When several Inline Tools (or Block Tunes) declare sanitize rules for the same tag, the configs were combined with Object.assign, so the last tool's rule replaced the previous ones. Two tools that both wrap text in <span> with different allowed attributes lost one set of attributes on save. Add mergeSanitizerConfigs() which combines rules per tag: boolean, attribute object and function rules are merged so that an attribute allowed by any contributor stays allowed. Use it in BlockToolAdapter.baseSanitizeConfig and Tools.getAllInlineToolsSanitizeConfig. Closes codex-team#2501 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
playlikeboy
requested review from
TatianaFomina,
gohabereg,
ilyamore88 and
neSpecc
as code owners
November 2, 2026 07:43
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The strict comparison in sanitizer.ts rejects truthy non-boolean predicate results, potentially stripping previously allowed attributes when rules are merged; boolean coercion and a regression test are needed.
1 open issue
What changed in this PR
This PR addresses #2501 by merging sanitizer rules from tools that share an HTML tag, preventing one tool’s allowed attributes from overwriting another’s.
Changes:
- Adds per-tag and per-attribute merging and applies it to both sanitizer configuration collectors.
- Adds unit and save-path regression tests for shared-tag rules.
- Updates the release version and changelog.
| File | Description |
|---|---|
test/cypress/tests/utils/sanitizer.cy.ts |
Adds 11 cases covering merge semantics. |
test/cypress/tests/sanitisation.cy.ts |
Tests saving attributes allowed by multiple inline tools. |
src/components/utils/sanitizer.ts |
Adds the shared sanitizer configuration merge helper. |
src/components/tools/block.ts |
Merges inline-tool and tune sanitizer rules. |
src/components/modules/tools.ts |
Merges sanitizer rules across all inline tools. |
package.json |
Bumps the version to 2.32.0-rc.4. |
docs/CHANGELOG.md |
Documents the shared-tag sanitizer fix. |
Review effort: Balanced
| */ | ||
| function checkAttributeRule(rule: AttributeRule, value: string, node: Element): boolean { | ||
| if (_.isFunction(rule)) { | ||
| return rule(value, node) === true; |
This branch has not been deployed
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.
Resolves #2501
Problem
When several Inline Tools declare a sanitize rule for the same tag, the rules were combined with
Object.assign, so the last tool to register silently overwrote the others. Example from the issue: one tool allows<span class>and another allows<span style>; only one of the two attributes survives, and content from the other tool is stripped on paste / save.This happened in two places:
src/components/tools/block.ts(baseSanitizeConfig)src/components/modules/tools.ts(getAllInlineToolsSanitizeConfig)Change
mergeSanitizerConfigs(...configs)insrc/components/utils/sanitizer.tsthat merges rules per tag and per attribute instead of replacing them. Semantics (documented in the JSDoc):trueon either side keeps the tag with all attributes;false/undefinedyields to the other side;trues staytrue, a function and anything else become one combined function, and two different attribute-string constraints become an OR check;block.tsstill takes precedence over the merged inline config (unchanged), as discussed on the issue.package.jsonbumped to2.32.0-rc.4and aFixline added todocs/CHANGELOG.md, following the pattern from fix(inline-toolbar): do not show "Convert to" when there is nothing to convert to #3040.Tests
test/cypress/tests/sanitisation.cy.ts: two inline tools declaring differentspanattributes; both attributes survive.test/cypress/tests/utils/sanitizer.cy.ts(new): 11 unit cases for the merge semantics above.Locally:
yarn lint(0 errors, same warning count asnext),yarn buildandyarn build:testpass. Cypress could not run on my machine (no Xvfb), so the Cypress specs were validated through a headless-Chrome harness against the built UMD bundle (reproduces the bug onnext, passes with this change); CI will be the first full Cypress run and I will fix anything it reports.Notes
types/configs/sanitizer-config.d.tsdeclares attribute values asboolean | string, while html-janitor also accepts functions; the helper handles that internally and I left the public type unchanged to keep the diff small. Happy to widen the type in this PR if you prefer.🤖 Generated with Claude Code