commit 1e9a616c6b0c9c2902edf6b5c0ce755de68f639c
parent acb0b7a9f7f7ff7a82a71bbffac5a7086a2f0a4a
Author: Tom Schuster <tschuster@mozilla.com>
Date: Mon, 20 Oct 2025 10:19:28 +0000
Bug 1994959 - Sanitizer: Don't allow local attributes+removeAttributes together with global removeAttributes. r=smaug
https://github.com/WICG/sanitizer-api/pull/311
Differential Revision: https://phabricator.services.mozilla.com/D269013
Diffstat:
5 files changed, 191 insertions(+), 64 deletions(-)
diff --git a/dom/security/sanitizer/Sanitizer.cpp b/dom/security/sanitizer/Sanitizer.cpp
@@ -536,6 +536,10 @@ void Sanitizer::IsValid(ErrorResult& aRv) {
// Step 4. None of config[elements], config[removeElements],
// config[replaceWithChildrenElements], config[attributes], or
// config[removeAttributes], if they exist, has duplicates.
+ //
+ // NOTE: The Sanitizer::CanonicalizeConfiguration method would have already
+ // thrown an error for duplicate elements/attributes by this point. The map
+ // and sets can't have duplicates by definition.
// Step 5. If both config[elements] and config[replaceWithChildrenElements]
// exist, then the intersection of config[elements] and
@@ -577,6 +581,11 @@ void Sanitizer::IsValid(ErrorResult& aRv) {
// Step 7.1.1.1. Neither element[attributes] or
// element[removeAttributes], if they exist, has duplicates.
+ //
+ // NOTE: The Sanitizer::CanonicalizeConfiguration (specifically
+ // CanonicalizeElementAttributes) method would have already thrown an
+ // error for duplicate attributes by this point. The attribute sets
+ // can't have duplicates by definition.
// Step 7.1.1.2. The intersection of config[attributes] and
// element[attributes] with default « [] » is empty.
@@ -657,7 +666,24 @@ void Sanitizer::IsValid(ErrorResult& aRv) {
for (const auto& entry : *mElements) {
const CanonicalElementAttributes& elemAttributes = entry.GetData();
- // Step 8.1.1. The intersection of config[removeAttributes] and
+ // Step 8.1.1. Not both element[attributes] and
+ // element[removeAttributes] exist.
+ if (elemAttributes.mAttributes && elemAttributes.mRemoveAttributes) {
+ return aRv.ThrowTypeError(
+ nsFmtCString(FMT_STRING("Element {} can't have both 'attributes' "
+ "and 'removeAttributes'."),
+ entry.GetKey()));
+ }
+
+ // Step 8.1.2. Neither element[attributes] nor
+ // element[removeAttributes], if they exist, has duplicates.
+ //
+ // NOTE: The Sanitizer::CanonicalizeConfiguration (specifically
+ // CanonicalizeElementAttributes) method would have already thrown an
+ // error for duplicate attributes by this point. The attribute sets
+ // can't have duplicates by definition.
+
+ // Step 8.1.3. The intersection of config[removeAttributes] and
// element[attributes] with default « [] » is empty.
if (elemAttributes.mAttributes) {
for (const CanonicalName& name : *elemAttributes.mAttributes) {
@@ -672,7 +698,7 @@ void Sanitizer::IsValid(ErrorResult& aRv) {
}
}
- // Step 8.1.2. The intersection of config[removeAttributes] and
+ // Step 8.1.4. The intersection of config[removeAttributes] and
// element[removeAttributes] with default « [] » is empty.
if (elemAttributes.mRemoveAttributes) {
for (const CanonicalName& name : *elemAttributes.mRemoveAttributes) {
@@ -885,78 +911,111 @@ bool Sanitizer::AllowElement(
// Step 2.2. Comment: We need to make sure the per-element attributes do
// not overlap with global attributes.
- // Step 2.3. If element["attributes"] exists:
- if (elementAttributes.mAttributes) {
- CanonicalNameSet attributes;
- for (const CanonicalName& attr : *elementAttributes.mAttributes) {
- // Step 2.3.1. Set element["attributes"] to remove duplicates from
- // element["attributes"].
- MOZ_ASSERT(!attributes.Contains(attr));
-
- // Step 2.3.2. If configuration["attributes"] exists:
- if (mAttributes) {
- // Step 2.3.2.1. Set element["attributes"] to the difference of
+ // Step 2.3. If configuration["attributes"] exists:
+ if (mAttributes) {
+ // Step 2.3.1. If element["attributes"] exists:
+ if (elementAttributes.mAttributes) {
+ CanonicalNameSet attributes;
+ for (const CanonicalName& attr : *elementAttributes.mAttributes) {
+ // Step 2.3.1.1. Set element["attributes"] to remove duplicates from
+ // element["attributes"].
+ MOZ_ASSERT(!attributes.Contains(attr));
+
+ // Step 2.3.1.2. Set element["attributes"] to the difference of
// element["attributes"] and configuration["attributes"].
if (mAttributes->Contains(attr)) {
continue;
}
+ // Step 2.3.1.3. If configuration["dataAttributes"] is true:
MOZ_ASSERT(mDataAttributes.isSome(),
"mDataAttributes exists iff mAttributes");
-
- // Step 2.3.2.2. If configuration["dataAttributes"] is true:
if (*mDataAttributes) {
- // Step 2.3.2.2.1. Remove all items item from element["attributes"]
+ // Step 2.3.1.3.1 Remove all items item from element["attributes"]
// where item is a custom data attribute.
if (attr.IsDataAttribute()) {
continue;
}
}
+
+ attributes.Insert(attr.Clone());
}
+ elementAttributes.mAttributes = Some(std::move(attributes));
+ }
- // Step 2.3.3. If configuration["removeAttributes"] exists:
- if (mRemoveAttributes) {
- // Step 2.3.3.1. Set element["attributes"] to the difference of
- // element["attributes"] and configuration["removeAttributes"].
- if (mRemoveAttributes->Contains(attr)) {
+ // Step 2.3.2. If element["removeAttributes"] exists:
+ if (elementAttributes.mRemoveAttributes) {
+ CanonicalNameSet removeAttributes;
+ for (const CanonicalName& attr : *elementAttributes.mRemoveAttributes) {
+ // Step 2.3.2.1. Set element["removeAttributes"] to remove duplicates
+ // from element["removeAttributes"].
+ //
+ // NOTE: CanonicalizeElementAttributes removed all duplicates for us.
+ MOZ_ASSERT(!removeAttributes.Contains(attr));
+
+ // Step 2.3.2.2. Set element["removeAttributes"] to the intersection
+ // of element["removeAttributes"] and configuration["attributes"].
+ if (!mAttributes->Contains(attr)) {
continue;
}
- }
- attributes.Insert(attr.Clone());
+ removeAttributes.Insert(attr.Clone());
+ }
+ elementAttributes.mRemoveAttributes = Some(std::move(removeAttributes));
}
- elementAttributes.mAttributes = Some(std::move(attributes));
- }
+ } else {
+ // Step 2.4. Otherwise:
- // Step 2.4. If element["removeAttributes"] exists:
- if (elementAttributes.mRemoveAttributes) {
- CanonicalNameSet removeAttributes;
- for (const CanonicalName& attr : *elementAttributes.mRemoveAttributes) {
- // Step 2.4.1. Set element["removeAttributes"] to remove duplicates from
- // element["removeAttributes"].
- MOZ_ASSERT(!removeAttributes.Contains(attr));
-
- // Step 2.4.2. If configuration["attributes"] exists:
- if (mAttributes) {
- // Step 2.4.2.1. Set element["removeAttributes"] to the intersection
- // of element["removeAttributes"] and configuration["attributes"].
- if (!mAttributes->Contains(attr)) {
+ // Step 2.4.1. If element["attributes"] exists:
+ if (elementAttributes.mAttributes) {
+ CanonicalNameSet attributes;
+ for (const CanonicalName& attr : *elementAttributes.mAttributes) {
+ // Step 2.4.1.1. Set element["attributes"] to remove duplicates from
+ // element["attributes"].
+ //
+ // NOTE: CanonicalizeElementAttributes removed all duplicates for us.
+ MOZ_ASSERT(!attributes.Contains(attr));
+
+ // Step 2.4.1.2. Set element["attributes"] to the difference of
+ // element["attributes"] and element["removeAttributes"] with default
+ // « ».
+ if (elementAttributes.mRemoveAttributes &&
+ elementAttributes.mRemoveAttributes->Contains(attr)) {
continue;
}
+
+ // Step 2.4.1.4. Set element["attributes"] to the difference of
+ // element["attributes"] and configuration["removeAttributes"].
+ if (mRemoveAttributes->Contains(attr)) {
+ continue;
+ }
+
+ attributes.Insert(attr.Clone());
}
+ elementAttributes.mAttributes = Some(std::move(attributes));
+
+ // Step 2.4.1.3. Remove element["removeAttributes"].
+ elementAttributes.mRemoveAttributes = Nothing();
+ }
+
+ // Step 2.4.2. If element["removeAttributes"] exists:
+ if (elementAttributes.mRemoveAttributes) {
+ CanonicalNameSet removeAttributes;
+ for (const CanonicalName& attr : *elementAttributes.mRemoveAttributes) {
+ // Step 2.4.2.1. Set element["removeAttributes"] to remove duplicates
+ // from element["removeAttributes"].
+ MOZ_ASSERT(!removeAttributes.Contains(attr));
- // Step 2.4.3. If configuration["removeAttributes"] exists:
- if (mRemoveAttributes) {
- // Step 2.4.3.1. Set element["removeAttributes"] to the difference of
+ // Step 2.4.2.2. Set element["removeAttributes"] to the difference of
// element["removeAttributes"] and configuration["removeAttributes"].
if (mRemoveAttributes->Contains(attr)) {
continue;
}
- }
- removeAttributes.Insert(attr.Clone());
+ removeAttributes.Insert(attr.Clone());
+ }
+ elementAttributes.mRemoveAttributes = Some(std::move(removeAttributes));
}
- elementAttributes.mRemoveAttributes = Some(std::move(removeAttributes));
}
// Step 2.5. If configuration["elements"] does not contain element:
diff --git a/testing/web-platform/meta/sanitizer-api/sethtml-tree-construction.tentative.html.ini b/testing/web-platform/meta/sanitizer-api/sethtml-tree-construction.tentative.html.ini
@@ -1,6 +1,3 @@
[sethtml-tree-construction.tentative.html]
- [Testcase #28, "<div id='div' title='div'>DIV</div>", config: "{ "elements": [{ "name": "div", "attributes": ["id"\], "removeAttributes": ["id"\] }\]}".]
- expected: FAIL
-
[Testcase #71, "<table><div><td>", config: "{ "replaceWithChildrenElements": ["table"\] }".]
expected: FAIL
diff --git a/testing/web-platform/tests/sanitizer-api/sanitizer-config.tentative.html b/testing/web-platform/tests/sanitizer-api/sanitizer-config.tentative.html
@@ -348,6 +348,17 @@ test(() => {
}, "Global attribute allow-list with a data attribute must not co-exist with config[dataAttributes] set to true.");
// If config[removeAttributes] exists:
+
+// Not both element[attributes] and element[removeAttributes] exist.
+test(() => {
+ assert_throws_js(TypeError, () => {
+ new Sanitizer({
+ removeAttributes: [],
+ elements: [{ name: "div", attributes: [], removeAttributes: [] }],
+ });
+ });
+}, "element[attributes] and element[removeAttributes] should not both exist with config[removeAttributes].");
+
// The intersection of config[removeAttributes] and element[attributes] with default « » is empty.
test(() => {
assert_throws_js(TypeError, () => {
diff --git a/testing/web-platform/tests/sanitizer-api/sanitizer-modifiers.tentative.html b/testing/web-platform/tests/sanitizer-api/sanitizer-modifiers.tentative.html
@@ -70,7 +70,7 @@ test(() => {
}, "sanitizer.allowAttribute() with global attributes and elements");
test(() => {
- const ELEMENTS = [{ name: "div", attributes: ["href"], removeAttributes: ["title"] }];
+ const ELEMENTS = [{ name: "div", attributes: ["href"] }];
let s = new Sanitizer({
removeAttributes: ["id"],
@@ -100,7 +100,41 @@ test(() => {
removeAttributes: [],
elements: ELEMENTS,
});
-}, "sanitizer.allowAttribute() with global removeAttributes and elements");
+}, "sanitizer.allowAttribute() with global removeAttributes and element's attributes");
+
+test(() => {
+ const ELEMENTS = [{ name: "div", removeAttributes: ["href"] }];
+
+ let s = new Sanitizer({
+ removeAttributes: ["id"],
+ elements: ELEMENTS,
+ });
+
+ assert_false(s.allowAttribute("class"));
+ assert_config(s.get(), {
+ removeAttributes: ["id"],
+ elements: ELEMENTS,
+ });
+
+ assert_true(s.allowAttribute("id"));
+ assert_config(s.get(), {
+ removeAttributes: [],
+ elements: ELEMENTS,
+ });
+
+ assert_false(s.allowAttribute("href"));
+ assert_config(s.get(), {
+ removeAttributes: [],
+ elements: ELEMENTS,
+ });
+
+ assert_false(s.allowAttribute("title"));
+ assert_config(s.get(), {
+ removeAttributes: [],
+ elements: ELEMENTS,
+ });
+}, "sanitizer.allowAttribute() with global removeAttributes and element's removeAttributes");
+
test(() => {
let s = new Sanitizer({
@@ -166,33 +200,52 @@ test(() => {
test(() => {
let s = new Sanitizer({
removeAttributes: ["title"],
- elements: [{ name: "div", attributes: ["class"], removeAttributes: ["id"] }],
+ elements: [{ name: "div", attributes: ["class"] }],
});
assert_false(s.removeAttribute({ name: "title", namespace: null }));
assert_config(s.get(), {
removeAttributes: ["title"],
- elements: [{ name: "div", attributes: ["class"], removeAttributes: ["id"] }],
+ elements: [{ name: "div", attributes: ["class"] }],
});
assert_true(s.removeAttribute("dir"));
assert_config(s.get(), {
removeAttributes: ["dir", "title"],
- elements: [{ name: "div", attributes: ["class"], removeAttributes: ["id"] }],
+ elements: [{ name: "div", attributes: ["class"] }],
});
- assert_true(s.removeAttribute("id"));
+ assert_true(s.removeAttribute("class"));
assert_config(s.get(), {
- removeAttributes: ["dir", "id", "title"],
- elements: [{ name: "div", attributes: ["class"], removeAttributes: [] }],
+ removeAttributes: ["class", "dir", "title"],
+ elements: [{ name: "div", attributes: [] }],
});
+}, "sanitizer.removeAttribute() with global removeAttributes and element's attributes");
- assert_true(s.removeAttribute("class"));
+test(() => {
+ let s = new Sanitizer({
+ removeAttributes: ["title"],
+ elements: [{ name: "div", removeAttributes: ["id"] }],
+ });
+
+ assert_false(s.removeAttribute({ name: "title", namespace: null }));
+ assert_config(s.get(), {
+ removeAttributes: ["title"],
+ elements: [{ name: "div", removeAttributes: ["id"] }],
+ });
+
+ assert_true(s.removeAttribute("dir"));
+ assert_config(s.get(), {
+ removeAttributes: ["dir", "title"],
+ elements: [{ name: "div", removeAttributes: ["id"] }],
+ });
+
+ assert_true(s.removeAttribute("id"));
assert_config(s.get(), {
- removeAttributes: ["class", "dir", "id", "title"],
- elements: [{ name: "div", attributes: [], removeAttributes: [] }],
+ removeAttributes: ["dir", "id", "title"],
+ elements: [{ name: "div", removeAttributes: [] }],
});
-}, "sanitizer.removeAttribute() with global removeAttributes and elements");
+}, "sanitizer.removeAttribute() with global removeAttributes and element's removeAttributes");
test(() => {
let s = new Sanitizer({
@@ -473,15 +526,14 @@ test(() => {
elements: [{name: "p", attributes: undefined, removeAttributes: ["class"]}]
});
- // XXX https://github.com/WICG/sanitizer-api/issues/305
- assert_true(s.allowElement({ name: "p", attributes: ["id", "dir"], removeAttributes: ["id", "lang"] }))
+ assert_true(s.allowElement({ name: "p", attributes: ["id", "dir", "lang"], removeAttributes: ["id", "lang"] }))
assert_config(s.get(), {
- elements: [{name: "p", attributes: ["dir"], removeAttributes: ["lang"]}]
+ elements: [{name: "p", attributes: ["dir"], removeAttributes: undefined}]
});
// Duplicate
- assert_false(s.allowElement({ name: "p", attributes: ["id", "dir"], removeAttributes: ["id", "lang"] }))
+ assert_false(s.allowElement({ name: "p", attributes: ["id", "dir", "lang"], removeAttributes: ["id", "lang"] }))
assert_config(s.get(), {
- elements: [{name: "p", attributes: ["dir"], removeAttributes: ["lang"]}]
+ elements: [{name: "p", attributes: ["dir"], removeAttributes: undefined}]
});
assert_true(s.allowElement({name: "p"}));
diff --git a/testing/web-platform/tests/sanitizer-api/support/util.js b/testing/web-platform/tests/sanitizer-api/support/util.js
@@ -51,6 +51,14 @@ function assert_config_is_valid(config) {
// If config[attributes] exists:
if (config.attributes) {
} else {
+ if (config.elements) {
+ for (let element of config.elements) {
+ // Not both element[attributes] and element[removeAttributes] exist.
+ assert_false("attributes" in element && "removeAttributes" in element,
+ `Element ${element.name} can't have both 'attributes' and 'removeAttributes'`);
+ }
+ }
+
// config[dataAttributes] does not exist.
assert_false("dataAttributes" in config, "dataAttributes does not exist");
}
