Fix ReDoS in stripHtml

[y0d4a]

stripHtml() can exhibit quadratic runtime on malformed HTML containing repeated unterminated tags.

The current regex may repeatedly scan the remaining input while searching for a closing >, allowing crafted RSS/Atom content to block the Node.js event loop.

This change prevents tag matching from crossing another <, bounding each attempted match and avoiding the pathological repeated scanning behavior.

A regression test was added using ~160 KB of repeated unterminated <br sequences.

Benchmark on Node.js v22.16.0 with the same 160 KB input:

• Before: ~18.9 s
• After: ~1.08 ms

The patch is intentionally minimal and does not add dependencies.

[rbren]

🤖 OpenHands is reviewing this PR.

Trigger label: ai-review
Label event: 29743228944 at 2026-08-20T12:34:09Z
Head commit: f88550121f48e5b22eaac56486dce31dce2867c8
View the conversation: http://localhost:8000/conversations/9f536832-7c64-49f9-8457-d9b62d9a1bfd

This comment was posted by an AI agent (OpenHands).

[rbren]

This review was posted by an AI agent (OpenHands).

Taste Rating: 🟢 Good taste

This is exactly how a ReDoS fix should look: replace the unbounded lazy wildcard (?:.|\n)*? with the bounded character class [^<>]*. Because [^<>]* and the required closing > are disjoint, the regex engine cannot backtrack, and each match attempt is bounded by the distance to the next < — the quadratic scanning behavior is structurally eliminated, not papered over. No new dependencies, two-line production change.

I verified the claims locally at the head SHA:

• npx mocha test/html.js — both tests pass (11 ms total).
• The 160 KB adversarial input ('a<br'.repeat(40000)) runs in ~2 ms with the new regexes.
• Multiline tags (<div\nclass=x>) and attribute-bearing tags (<br class="x">) still strip correctly, since [^<>] includes \n.
• I probed for residual pathological inputs (runs of <, long unterminated segments, repeated <aaa<aaa patterns) — all linear.

Findings (non-blocking)

1. Behavior change on < inside quoted attribute values (lib/utils.js lines 6–7, inline comment): a<p title="5 < 3">b previously produced a\nb; it now produces a<p title="5 b, leaking a raw tag fragment into snippets. A literal < in a quoted attribute value is valid HTML5, so this is not purely a malformed-input case. Rare in real feeds and an acceptable trade-off for the security win, but it should be a conscious decision.
2. Test relies implicitly on Mocha's default timeout (test/html.js line 47, inline comment): the "efficiently" guarantee only holds because the pre-fix ~19 s runtime exceeds Mocha's 2 s default. Worth making explicit so a future global timeout bump doesn't silently mask a regression.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Two-line change confined to lib/utils.js stripHtml(), which only feeds contentSnippet/getSnippet output. No new dependencies, no API changes, existing tests pass, and the only behavioral delta is on an uncommon edge case in already-lossy snippet generation. The security posture strictly improves: crafted feed content can no longer block the event loop.

KEY INSIGHT: Bounding the tag regex at </> boundaries removes backtracking structurally rather than mitigating it, which is the right way to kill a ReDoS.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.
2. Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

---

✅ APPROVED

[rbren]

Edge case — behavior change for < in quoted attribute values. With the old lazy regex, a<p title="5 < 3">b stripped to a\nb. With [^<>]* the first regex can no longer cross the inner <, and the second regex on line 7 then consumes < 3">, leaving a<p title="5 b — a raw tag fragment leaks into the snippet. A literal < inside a quoted attribute value is valid HTML5, so this isn't strictly a malformed-input-only regression.

This is a reasonable trade-off for eliminating the ReDoS (and this function is a best-effort stripper, not a parser), but it should be a deliberate choice. If you want to keep coverage for this case, an alternative that stays linear is allowing quoted strings inside the tag, e.g. (?:[^<>"']|"[^"]*"|'[^']*')* in place of [^<>]*. Otherwise, consider noting the limitation so a future "why is a tag fragment in my snippet" report doesn't get reverted back to the vulnerable pattern.

[rbren]

Implicit timeout dependency. This test only fails on a performance regression because the pre-fix runtime (~19 s) exceeds Mocha's default 2 s timeout. If someone later raises the suite-wide timeout (a common CI tweak), this test silently degrades into a plain correctness check and stops guarding against the ReDoS.

Make the intent explicit — e.g. use a function() callback with this.timeout(2000), or assert on elapsed time directly:

it('should handle repeated unterminated HTML tags efficiently', function() {
  this.timeout(2000);
  var input = 'a<br'.repeat(40000);
  Expect(utils.getSnippet(input)).to.equal(input);
});

[nikanand04]

🤖 OpenHands is reviewing this PR.

Trigger label: ai-review
Label event: 29743228944 at 2026-08-20T12:34:09Z
Head commit: f88550121f48e5b22eaac56486dce31dce2867c8
View the conversation: http://localhost:8000/conversations/43adfbea-30ac-4f5e-b052-a666cbafb01d

This comment was posted by an AI agent (OpenHands).

[nikanand04]

This review was posted by an AI agent (OpenHands).

Taste Rating: 🟢 Good taste (with two non-blocking notes)

This is the right shape for a ReDoS fix: the unbounded lazy wildcard (?:.|\n)*? is replaced with the bounded class [^<>]*. Because [^<>]* and the required > are disjoint, the engine has nothing to backtrack over, and every match attempt is bounded by the distance to the next < or >. The quadratic behavior is eliminated structurally, not mitigated. Two-line production change, no new dependencies. [^<>] still matches \n, so multiline tags keep working — that's the subtle part the patch got right.

Verification I ran at the head SHA

I reimplemented both the old and new stripHtml bodies verbatim and measured them on this machine (Node v22):

Input	Old	New
'a<br'.repeat(40000) (~160 KB)	8451 ms	0.71 ms

I also probed for residual pathological inputs against the new regexes — all linear:

• '<'.repeat(80000) → 0.35 ms
• ('<' + 'a'.repeat(50)).repeat(4000) → 0.76 ms
• ('x<div ' + 'a'.repeat(100)).repeat(2000) → 0.85 ms
• '<' + 'a'.repeat(160000) → 0.30 ms
• ('a<br\n').repeat(30000) → 1.28 ms

And the existing behavior in test/html.js is preserved: hello<br>world, <h4>hi</h4>my name is, x<div\nclass=y>z, <!-- c -->x, and a<p title="a>b">c all produce identical output before and after. The class of bug is real for this project: stripHtml runs on attacker-supplied feed content via getSnippet → contentSnippet (lib/parser.js:171, lib/parser.js:240), on the main thread. Polynomial rather than exponential, but 8+ seconds of blocked event loop from 160 KB of feed text is a genuine DoS.

Notes (non-blocking)

1. Behavior change on < inside a tag — inline comment on lib/utils.js:7. This is the only observable output delta I could find, and there is no test pinning it.
2. The regression test's "efficiently" claim is implicit — inline comment on test/html.js:47.
3. The committed browser bundles still carry the vulnerable regex. dist/rss-parser.js and dist/rss-parser.min.js are tracked in this repo and still contain (?:.|\n)*?> (0 occurrences of [^<>]). The repo's convention is clearly to rebuild dist/ in separate "build distro" commits (bba9cf3, 0413e12), so regenerating it is not this PR's job — but bower/browser consumers stay exposed until a maintainer rebuild + release, which is worth tracking on the merge.
4. No CI evidence on this head SHA. GET /commits/f8855012/check-runs returns an empty list, so the tests workflow has no recorded run for this commit. The before/after numbers in the description are also just prose. That's why I re-measured independently above; a maintainer should make sure CI actually runs before merging.

One thing worth stating explicitly so it isn't mistaken for a regression in this PR: getSnippet calls entities.decodeHTML() after stripping, so its output can already contain raw </> from </> in the source feed. Snippets are plain text and were never HTML-safe. The fragment leak in note 1 is therefore an output-quality change, not a new XSS class.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW

Two lines inside one pure string function whose output feeds only contentSnippet/*Snippet fields. No API surface change, no dependency change, no state. The security posture strictly improves (measured 8451 ms → 0.71 ms on the adversarial input), and the single behavioral delta is confined to malformed-or-unusual markup in already-lossy snippet text. Main residual risks are process, not code: missing CI run on the head SHA and the stale dist/ bundles.

VERDICT: ✅ Worth merging — address notes 1 and 2 if convenient; neither blocks.

KEY INSIGHT: Bounding the tag body at </> removes the backtracking rather than merely making it cheaper, which is the only kind of ReDoS fix that stays fixed — the cost is that a literal < inside a tag now truncates the match, and that trade-off deserves a test rather than silence.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

---

✅ APPROVED

[nikanand04]

Behavior change: a literal < inside a tag now truncates the match, leaking a fragment into the snippet.

Measured with the old vs. new bodies of stripHtml:

'a<p title="5 < 3">b'  OLD -> 'a\nb'      NEW -> 'a<p title="5 b'
'a<b<c>d'              OLD -> 'ad'        NEW -> 'a<bd'

A literal < in a quoted attribute value is valid HTML5, and the HTML5 tokenizer also treats < in tag-name/attribute-name position as an ordinary character, so both cases above are "a tag" to a browser but now leave visible markup in contentSnippet. This is a reasonable trade-off for killing the quadratic scan — the leftover fragment can never be a complete tag, since the match stops precisely at the next < — but it should be a deliberate decision rather than a side effect.

Concretely: add a case to the testCases table in test/html.js pinning the new output for < inside a tag. That documents the intent, and it means a future "improvement" to this regex can't silently change snippet text again.

(If you ever want the old semantics back without the ReDoS, /<[^<>"']*(?:"[^"]*"|'[^']*')?[^<>"']*>/ style attribute-aware matching is possible, but it is materially more complex for a case this rare — I would not do it here.)

[nikanand04]

The test's performance guarantee is implicit, and the assertion is the wrong shape for a 160 KB string.

The test name promises "efficiently", but nothing here asserts anything about time. It only fails pre-fix because ~8.5 s (measured locally) exceeds Mocha's 2 s default timeout — there is no .mocharc, so that default is the entire safety net. Bump the global timeout for an unrelated slow test later and this regression test goes green against the vulnerable regex without anyone noticing.

Make the bound explicit. Note the arrow function on line 45 also prevents this.timeout() from working, so it needs to become a function:

it('should handle repeated unterminated HTML tags efficiently', function() {
  this.timeout(1000);
  var input = 'a<br'.repeat(40000);
  Expect(utils.getSnippet(input)).to.equal(input);
})

Secondary: Expect(bigString).to.equal(bigString) with --reporter-option maxDiffSize=0 (unlimited) in the test script means a failure dumps a 160 KB character diff into CI logs. Expect(utils.getSnippet(input) === input).to.equal(true) — or comparing lengths plus a prefix — keeps the failure readable.