Skip to content

Fix ReDoS in stripHtml #308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
y0d4a wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix ReDoS in stripHtml #308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f885501
Fix ReDoS in stripHtml
y0d4a Aug 14, 2026
cdf023d
Make ReDoS regression timeout explicit
y0d4a Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ const entities = require('entities');
const xml2js = require('xml2js');

utils.stripHtml = function(str) {
str = str.replace(/([^\n])<\/?(h|br|p|ul|ol|li|blockquote|section|table|tr|div)(?:.|\n)*?>([^\n])/gm, '$1\n$3')
str = str.replace(/<(?:.|\n)*?>/gm, '');
str = str.replace(/([^\n])<\/?(h|br|p|ul|ol|li|blockquote|section|table|tr|div)[^<>]*>([^\n])/gm, '$1\n$3')

@rbren rbren Aug 20, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

str = str.replace(/<[^<>]*>/gm, '');

@nikanand04 nikanand04 Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

return str;
}

Expand Down
6 changes: 6 additions & 0 deletions test/html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,11 @@ describe('Utils', function() {
Expect('|' + utils.getSnippet(tc.input) + '|').to.equal('|' + tc.output + '|', tc.input);
});
})

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);

@rbren rbren Aug 20, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nikanand04 Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})
});