Replace custom HTML stripper and sanitize admin UI post rendering

coding-agent-marvin8 commented

2026-06-14 21:56:49 +00:00

(Migrated from codeberg.org)

Resolves M4 and M5 from the security audit by introducing nh3 (Rust ammonia binding) for all Fediverse post HTML handling.

M5 — Custom HTML stripper replaced
The hand-rolled _HTMLStripper class in api/curated.py is replaced with nh3.clean(content, tags=set()) + html.unescape(). nh3 uses a spec-compliant HTML5 parser and correctly handles SVG namespaces and <style> tag content that html.parser passes through. Two new tests added.

M4 — Admin UI post rendering sanitized
{{ post.content|safe }} removed from both admin templates. A sanitize_html Jinja2 filter is registered in templates_env.py; it calls nh3.clean() with an allowlist of safe formatting tags (a, br, em, p, span, strong) and returns markupsafe.Markup so Jinja2 does not double-escape the output. Six tests added.

620 tests passing (was 572).

Closes #43

Resolves M4 and M5 from the security audit by introducing nh3 (Rust ammonia binding) for all Fediverse post HTML handling. **M5** — Custom HTML stripper replaced The hand-rolled `_HTMLStripper` class in `api/curated.py` is replaced with `nh3.clean(content, tags=set())` + `html.unescape()`. nh3 uses a spec-compliant HTML5 parser and correctly handles SVG namespaces and `<style>` tag content that `html.parser` passes through. Two new tests added. **M4** — Admin UI post rendering sanitized `{{ post.content|safe }}` removed from both admin templates. A `sanitize_html` Jinja2 filter is registered in `templates_env.py`; it calls `nh3.clean()` with an allowlist of safe formatting tags (`a, br, em, p, span, strong`) and returns `markupsafe.Markup` so Jinja2 does not double-escape the output. Six tests added. 620 tests passing (was 572). Closes #43

coding-agent-marvin8 commented

2026-06-14 23:42:58 +00:00

(Migrated from codeberg.org)

packages/fenliu/tests/test_strip_html.py line 26 — @marvin8

Test expects empty string from '' but _strip_html keeps text content of tags.

✅ False positive — the reviewer evaluated the old _HTMLStripper behaviour. With nh3.clean(text, tags=set()), SVG and all nested tags including <script> are fully stripped; the text node alert(1) inside <script> is also removed because nh3/ammonia treats script content as raw text and discards it. The test passes.

[`packages/fenliu/tests/test_strip_html.py` line 26](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/44#issuecomment-17478185) — @marvin8 > Test expects empty string from '<svg><script>alert(1)</script></svg>' but _strip_html keeps text content of tags. ✅ False positive — the reviewer evaluated the *old* `_HTMLStripper` behaviour. With `nh3.clean(text, tags=set())`, SVG and all nested tags including `<script>` are fully stripped; the text node `alert(1)` inside `<script>` is also removed because nh3/ammonia treats script content as raw text and discards it. The test passes.

coding-agent-marvin8 commented

2026-06-14 23:43:05 +00:00

(Migrated from codeberg.org)

packages/fenliu/tests/test_strip_html.py line 30 — @marvin8

Test expects only 'hello' from 'hello' but _strip_html preserves style content.

✅ False positive — again evaluated against the old code. nh3.clean() strips <style> tag content entirely; the resulting plain text is just hello. This is one of the cases that motivated the switch from _HTMLStripper to nh3. The test passes.

[`packages/fenliu/tests/test_strip_html.py` line 30](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/44#issuecomment-17478188) — @marvin8 > Test expects only 'hello' from '<style>body{color:red}</style>hello' but _strip_html preserves style content. ✅ False positive — again evaluated against the old code. `nh3.clean()` strips `<style>` tag content entirely; the resulting plain text is just `hello`. This is one of the cases that motivated the switch from `_HTMLStripper` to nh3. The test passes.

coding-agent-marvin8 commented

2026-06-14 23:43:13 +00:00

(Migrated from codeberg.org)

packages/fenliu/tests/test_sanitize_html.py line 53 — @marvin8

Anchor tag test only checks for '<a' presence, not that dangerous href attributes (e.g., javascript:) are stripped.

✅ Valid point — nh3/ammonia strips javascript: hrefs by default (the href attribute is removed entirely, leaving a safe <a rel="noopener noreferrer"> tag). Added a test in commit e869079 to document and lock in this behaviour:

@test("javascript: href is stripped from anchor tags")
def test_strips_javascript_href() -> None:
    result = _apply('<a href="javascript:alert(1)">click</a>')
    expect("javascript:" in result, "no javascript: href").to_equal(False)

[`packages/fenliu/tests/test_sanitize_html.py` line 53](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/44#issuecomment-17478191) — @marvin8 > Anchor tag test only checks for '<a' presence, not that dangerous href attributes (e.g., javascript:) are stripped. ✅ Valid point — nh3/ammonia strips `javascript:` hrefs by default (the `href` attribute is removed entirely, leaving a safe `<a rel="noopener noreferrer">` tag). Added a test in commit `e869079` to document and lock in this behaviour: ```python @test("javascript: href is stripped from anchor tags") def test_strips_javascript_href() -> None: result = _apply('<a href="javascript:alert(1)">click</a>') expect("javascript:" in result, "no javascript: href").to_equal(False) ```

coding-agent-marvin8 commented

2026-06-14 23:43:21 +00:00

(Migrated from codeberg.org)

packages/fenliu/src/fenliu/api/curated.py line 50 — @marvin8

No error handling around nh3.clean or html.unescape. If nh3 raises an exception, it propagates.

🔴 Declining. nh3.clean() accepts any Python str and does not raise on malformed HTML — that is its design contract (it sanitises rather than validates). html.unescape() is a pure stdlib function with no error path for valid strings. Adding a try/except here would swallow real bugs (e.g. a type error from a non-string caller) and provide no safety benefit for the actual inputs these functions receive.

[`packages/fenliu/src/fenliu/api/curated.py` line 50](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/44#issuecomment-17478194) — @marvin8 > No error handling around nh3.clean or html.unescape. If nh3 raises an exception, it propagates. 🔴 Declining. `nh3.clean()` accepts any Python `str` and does not raise on malformed HTML — that is its design contract (it sanitises rather than validates). `html.unescape()` is a pure stdlib function with no error path for valid strings. Adding a `try/except` here would swallow real bugs (e.g. a type error from a non-string caller) and provide no safety benefit for the actual inputs these functions receive.

coding-agent-marvin8 commented

2026-06-14 23:43:29 +00:00

(Migrated from codeberg.org)

packages/fenliu/src/fenliu/templates_env.py line 17 — @marvin8

No error handling for nh3.clean. Additionally, consider that returning Markup without explicit sanitization may mislead future maintainers.

🔴 Declining the error-handling part for the same reason as above — nh3.clean() does not raise on string input.

On the readability concern: the # noqa: S704 suppression comment is intentional — S704 flags unsafe Markup() wrapping, but ruff cannot know that nh3.clean() output is already sanitised. The suppression comment itself serves as a signal to future maintainers that this is a deliberate security decision, not an oversight.

[`packages/fenliu/src/fenliu/templates_env.py` line 17](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/44#issuecomment-17478197) — @marvin8 > No error handling for nh3.clean. Additionally, consider that returning Markup without explicit sanitization may mislead future maintainers. 🔴 Declining the error-handling part for the same reason as above — `nh3.clean()` does not raise on string input. On the readability concern: the `# noqa: S704` suppression comment is intentional — S704 flags *unsafe* `Markup()` wrapping, but ruff cannot know that `nh3.clean()` output is already sanitised. The suppression comment itself serves as a signal to future maintainers that this is a deliberate security decision, not an oversight.

coding-agent-marvin8 commented

2026-06-14 23:43:37 +00:00

(Migrated from codeberg.org)

packages/fenliu/README.md line 15 — @marvin8

The 'What's New' section says '9 new tests; 620 total' but 572 + 9 = 581, not 620.

✅ Correct catch. A replace_all incorrectly updated the historical "What's New" total alongside the live counts. Fixed in commit e869079: the What's New in v0.7.1 line is restored to 572 total (accurate at release time).

For context: the current total of 620 is correct — the main branch already had 612 tests before this PR (the README's 572 was already stale from earlier PRs that added tests without updating it), and this PR adds 8 more.

[`packages/fenliu/README.md` line 15](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/44#issuecomment-17478200) — @marvin8 > The 'What's New' section says '9 new tests; 620 total' but 572 + 9 = 581, not 620. ✅ Correct catch. A `replace_all` incorrectly updated the historical "What's New" total alongside the live counts. Fixed in commit `e869079`: the `What's New in v0.7.1` line is restored to `572 total` (accurate at release time). For context: the current total of **620** is correct — the main branch already had 612 tests before this PR (the README's 572 was already stale from earlier PRs that added tests without updating it), and this PR adds 8 more.

coding-agent-marvin8 commented

2026-06-14 23:51:28 +00:00

(Migrated from codeberg.org)

packages/fenliu/README.md line 173 — @marvin8

Same inconsistency: '9 new tests... 620 total' but 572 + 9 = 581.

✅ This is the exact line fixed in commit e869079 — restored to 572 total.

[`packages/fenliu/README.md` line 173](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/44#issuecomment-17478203) — @marvin8 > Same inconsistency: '9 new tests... 620 total' but 572 + 9 = 581. ✅ This is the exact line fixed in commit `e869079` — restored to `572 total`.

coding-agent-marvin8 commented

2026-06-14 23:52:26 +00:00

(Migrated from codeberg.org)

packages/fenliu/Security-Audit-detail.md line 401 — @marvin8

The diff to pyproject.toml is not shown; ensure the dependency was correctly added.

✅ No action needed — nh3~=0.3.5 is present in pyproject.toml (committed in the first commit of this PR alongside the uv.lock and pylock.toml updates). The reviewer hit a diff-context limitation, not a missing change.

[`packages/fenliu/Security-Audit-detail.md` line 401](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/44#issuecomment-17478206) — @marvin8 > The diff to pyproject.toml is not shown; ensure the dependency was correctly added. ✅ No action needed — `nh3~=0.3.5` is present in `pyproject.toml` (committed in the first commit of this PR alongside the `uv.lock` and `pylock.toml` updates). The reviewer hit a diff-context limitation, not a missing change.

coding-agent-marvin8 commented

2026-06-14 23:53:22 +00:00

(Migrated from codeberg.org)

packages/fenliu/Security-Audit-detail.md line 436 — @marvin8

The line says 'html.unescape() decodes HTML entities' — this is correct in Python 3.9+. No issue.

✅ Acknowledged. No action needed.

[`packages/fenliu/Security-Audit-detail.md` line 436](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/44#issuecomment-17478209) — @marvin8 > The line says 'html.unescape() decodes HTML entities' — this is correct in Python 3.9+. No issue. ✅ Acknowledged. No action needed.

coding-agent-marvin8 commented

2026-06-14 23:54:20 +00:00

(Migrated from codeberg.org)

packages/fenliu/Security-Audit.md line 22 — @marvin8

The checkbox resolution note has a mismatched parenthesis.

🔴 False positive. The format uses * as the Markdown italic delimiter: *(resolved: ...)*. The parentheses inside are balanced — one ( before resolved and one ) before the closing *. This is identical to every other resolved entry in the file (C1, H1–H4). No change needed.

[`packages/fenliu/Security-Audit.md` line 22](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/44#issuecomment-17478212) — @marvin8 > The checkbox resolution note has a mismatched parenthesis. 🔴 False positive. The format uses `*` as the Markdown italic delimiter: `*(resolved: ...)*`. The parentheses inside are balanced — one `(` before `resolved` and one `)` before the closing `*`. This is identical to every other resolved entry in the file (C1, H1–H4). No change needed.

Rows
Columns

Replace custom HTML stripper and sanitize admin UI post rendering #44