Validate tag_attribute_values conflict with attributes#135
Merged
messense merged 1 commit intoJun 30, 2026
Merged
Conversation
`attributes` and `tag_attribute_values` are alternates in ammonia: once an attribute is whitelisted in `attributes` (under the tag or the generic "*" key) it is accepted with any value, so a per-value whitelist in `tag_attribute_values` for the same attribute is silently ignored. Listing both is a footgun -- the caller thinks the value is restricted when it is not. Raise ValueError when the same tag/attribute appears in both, mirroring the existing link_rel and clean_content_tags conflict checks, and update the docstrings to point at the correct usage. Also add the previously missing test that a non-matching value is stripped.
messense
approved these changes
Jun 30, 2026
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.
Closes #61.
Problem
tag_attribute_valuesandattributesare alternates in ammonia, not an intersection. ammonia keeps an attribute if it is allowed by eitherattributes(the per-tag set, or the generic*set) or bytag_attribute_values. So the moment an attribute is whitelisted in both, theattributesentry already accepts every value and the per-value whitelist intag_attribute_valuesis never consulted.That makes the example from #61 a silent footgun:
The caller thinks
my-attris limited tomy-attr-value; in reality every value is allowed. With an attribute likestyleorhrefthat is a real sanitisation hole, and nothing tells the user.Fix
Reject the contradiction up front instead of silently ignoring it. When the same tag/attribute is listed in both
attributes(under the tag or*) andtag_attribute_values, theCleanerconstructor now raisesValueError:This mirrors the existing conflict checks for
rel/link_rel(#117) andclean_content_tags/tags(#125), so it stays consistent with how nh3 already handles mutually-exclusive options. The check lives in the constructor, so it covers bothnh3.clean(...)andnh3.Cleaner(...).I also updated the docstrings to say the attribute must be left out of
attributesfor the value restriction to apply, and added the test #61 pointed out was missing — that a value outside the allowed set is stripped when the feature is used correctly.Scope
I kept this to the explicit
attributesoverlap, which is the case in the issue. Two adjacent cases are left out on purpose; happy to fold them in if you'd prefer:generic_attribute_prefixes(rarer, and prefix attributes likedata-*are generally inert);attributesis omitted (no footgun there — the defaults are already curated).Tests
pytestis green (28 tests). New:test_clean_tag_attribute_values_conflict/test_cleaner_tag_attribute_values_conflict— the conflict raises through both entry points, including the generic*form.test_tag_attribute_values_filters_non_matching_values— correct usage keeps a matching value and strips a non-matching one.