Skip to content

Validate tag_attribute_values conflict with attributes#135

Merged
messense merged 1 commit into
messense:mainfrom
Mohammadjavad2412:validate-tag-attribute-values-conflict
Jun 30, 2026
Merged

Validate tag_attribute_values conflict with attributes#135
messense merged 1 commit into
messense:mainfrom
Mohammadjavad2412:validate-tag-attribute-values-conflict

Conversation

@Mohammadjavad2412

Copy link
Copy Markdown
Contributor

Closes #61.

Problem

tag_attribute_values and attributes are alternates in ammonia, not an intersection. ammonia keeps an attribute if it is allowed by either attributes (the per-tag set, or the generic * set) or by tag_attribute_values. So the moment an attribute is whitelisted in both, the attributes entry already accepts every value and the per-value whitelist in tag_attribute_values is never consulted.

That makes the example from #61 a silent footgun:

nh3.clean(
    "<p my-attr='my-WRONG-attr-value'>text</p>",
    tags={"p"},
    attributes={"p": {"my-attr"}},
    tag_attribute_values={"p": {"my-attr": {"my-attr-value"}}},
)
# -> '<p my-attr="my-WRONG-attr-value">text</p>'   # the value restriction is ignored

The caller thinks my-attr is limited to my-attr-value; in reality every value is allowed. With an attribute like style or href that 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 *) and tag_attribute_values, the Cleaner constructor now raises ValueError:

attribute "my-attr" on tag "p" is whitelisted in both `attributes` and
`tag_attribute_values`, which are alternates; `attributes` already permits every
value, so the `tag_attribute_values` whitelist would be silently ignored. Drop
"my-attr" from `attributes` (the "p" entry or "*") to restrict it by value

This mirrors the existing conflict checks for rel/link_rel (#117) and clean_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 both nh3.clean(...) and nh3.Cleaner(...).

I also updated the docstrings to say the attribute must be left out of attributes for 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 attributes overlap, which is the case in the issue. Two adjacent cases are left out on purpose; happy to fold them in if you'd prefer:

  • overlap with generic_attribute_prefixes (rarer, and prefix attributes like data-* are generally inert);
  • the attribute colliding with ammonia's default attribute whitelist when attributes is omitted (no footgun there — the defaults are already curated).

Tests

pytest is 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.

`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
messense merged commit 567c411 into messense:main Jun 30, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tag_attribute_values doesn't filter out the attribute if value doesn't match

2 participants