Add add_*/rm_* keyword arguments for set-valued sanitizer options#128
Open
gghez wants to merge 3 commits into
Open
Add add_*/rm_* keyword arguments for set-valued sanitizer options#128gghez wants to merge 3 commits into
gghez wants to merge 3 commits into
Conversation
Documents the eight add_*/rm_* keyword arguments planned for clean() and Cleaner, plus the rationale for limiting scope to set-valued options (tags, clean_content_tags, url_schemes, generic_attribute_prefixes).
Closes messense#78. Expose ammonia's incremental builder methods alongside the existing replacement options: - add_tags / rm_tags - add_clean_content_tags / rm_clean_content_tags - add_url_schemes / rm_url_schemes - add_generic_attribute_prefixes / rm_generic_attribute_prefixes These apply on top of the corresponding replacement option (or the ammonia defaults when it is omitted), so a caller can extend the default whitelist without copying it: nh3.clean("<my-tag>x</my-tag>", add_tags={"my-tag"}) The existing clean_content_tags overlap guard now uses the effective tag sets (after add_*/rm_* are applied) to keep ammonia from panicking when overlap is introduced via the new add_* options. filter_style_properties from the original issue is already shipped.
…tions-design.md no need to commit the spec file
There was a problem hiding this comment.
Pull request overview
This PR extends nh3’s Python API to support ammonia’s incremental “builder” modifiers (add_* / rm_*) for set-valued sanitizer options, allowing callers to extend or subtract from defaults without copying full whitelists.
Changes:
- Added
add_*/rm_*keyword arguments fortags,clean_content_tags,url_schemes, andgeneric_attribute_prefixesto bothnh3.clean()andnh3.Cleaner. - Updated overlap validation between allowed tags and clean-content tags to consider the effective sets after modifiers are applied.
- Added tests, type stubs, and documentation covering the new options and examples.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/lib.rs |
Adds new config fields, wires the new keyword args through to ammonia builder methods, and updates overlap validation logic. |
tests/test_nh3.py |
Adds new pytest coverage for add_* / rm_* behaviors and modifier interactions. |
nh3.pyi |
Updates public type signatures to include the new keyword arguments. |
docs/index.rst |
Documents how to extend/remove from defaults using add_* / rm_* options with doctest examples. |
Comments suppressed due to low confidence (1)
src/lib.rs:456
- The ValueError message references only
tagsandclean_content_tags, but conflicts can now be introduced viaadd_tags/rm_tagsandadd_clean_content_tags/rm_clean_content_tagsas well. Consider updating the message to describe the effective allowed vs clean-content sets and suggest resolving it by either removing from the clean-content set or removing from the allowed set (including viarm_tags/rm_clean_content_tags), so callers know how to fix conflicts introduced by the new modifiers.
if let Some(tag) = effective_clean.intersection(&effective_allowed).next() {
return Err(PyValueError::new_err(format!(
"tag \"{}\" cannot appear in both `tags` and `clean_content_tags`; \
either remove it from `clean_content_tags` or pass an explicit \
`tags` set that excludes it",
tag
)));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // applying the add_*/rm_* modifiers, then check they are disjoint. | ||
| // Otherwise ammonia would panic at clean-time. When the caller omits | ||
| // the replacement option, ammonia falls back to its own defaults. | ||
| if clean_content_tags.is_some() || add_clean_content_tags.is_some() { |
Comment on lines
+70
to
+77
| def test_add_tags_extends_defaults(): | ||
| # "my-tag" is not in the defaults, so it would normally be stripped. | ||
| assert nh3.clean("<my-tag>x</my-tag>") == "x" | ||
| # add_tags extends the defaults without replacing them, so default tags | ||
| # like <b> are still preserved. | ||
| assert ( | ||
| nh3.clean("<b><my-tag>x</my-tag></b>", add_tags={"my-tag"}) | ||
| == "<b><my-tag>x</my-tag></b>" |
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 #78.
Summary
Exposes ammonia's incremental builder methods alongside the existing
replacement options on both
nh3.clean()andnh3.Cleaner:add_tags/rm_tagsadd_clean_content_tags/rm_clean_content_tagsadd_url_schemes/rm_url_schemesadd_generic_attribute_prefixes/rm_generic_attribute_prefixesEach new option applies on top of the corresponding replacement option
(or the ammonia defaults when it is omitted), so a caller can extend the
default whitelist without first copying it:
rm_*applies last, so it wins on conflict withadd_*— mirroringthe ammonia builder's ordering.
Notes
filter_style_propertiesoption that the issue also requestedhas already shipped (it is in v0.3.x), so this PR covers only the
remaining
add_tag-style request, generalized to the fourset-valued options that ammonia exposes
add_*/rm_*for.clean_content_tagsoverlap guard (PR Validate clean_content_tags conflict with tags #125) nowcomputes the effective tag sets after
add_*/rm_*are applied,so e.g.
add_tags={"p"}plusadd_clean_content_tags={"p"}raisesValueErrorrather than panicking ammonia.attributes,allowed_classes,tag_attribute_values,set_tag_attribute_values) keep thereplacement-only behaviour — designing an ergonomic additive API
for nested dicts is out of scope here.
Test plan
maturin develop --releasebuilds cleanlypython -m pytest -v— 23 passed (8 new tests + existing 14 + 1 RST doctest)cargo fmt --check— silentcargo check— exit 0docs/index.rstruns as part of the suite