fix: HTML parser panic protection with multiple fallback

[ehsandeep]

• Added aggressive HTML sanitization to reduce nesting depth
• Implement size limiting (1MB) to prevent processing huge documents
• Add plain text extraction fallback for complex HTML structures
• Enhance panic recovery with comprehensive error handling
• Remove deeply nestable elements (div, span, ul, ol, li) from sanitizer
• Add comprehensive test coverage for edge cases

Resolves HTML parser panic: 'html: open stack of elements exceeds 512 nodes' that occurred after switching to html-to-markdown/v2 library in PR #2255

$ ./httpx  -title -tech-detect -status-code -location -content-length -cname -web-server -follow-redirects -websocket -u https://unpkg.com/three@0.150.0/build/three.min.js

    __    __  __       _  __
   / /_  / /_/ /_____ | |/ /
  / __ \/ __/ __/ __ \|   /
 / / / / /_/ /_/ /_/ /   |
/_/ /_/\__/\__/ .___/_/|_|
             /_/

		projectdiscovery.io

[INF] Current httpx version v1.7.2 (latest)
[WRN] UI Dashboard is disabled, Use -dashboard option to enable
https://unpkg.com/three@0.150.0/build/three.min.js [200] [] [613667] [cloudflare] [Cloudflare,Fly.io,HSTS,HTTP/3]

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Improved HTML content processing with stricter sanitization policies, retaining only essential text and formatting elements
  • Added support for large HTML inputs with 1MB size limit
  • Enhanced fallback mechanisms for robust content extraction when HTML conversion encounters issues

• Tests

  • Expanded coverage for large input handling and edge case scenarios

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This change modifies HTML-to-text conversion logic in the page type classifier by introducing an ultra-aggressive sanitization policy that permits only basic text and formatting elements, enforcing a 1MB input size limit, implementing fallback mechanisms for conversion failures, and adding a helper function for plain-text extraction from sanitized HTML with corresponding test coverage updates.

Changes

Cohort / File(s)	Summary
HTML Sanitization & Text Extraction Logic common/pagetypeclassifier/pagetypeclassifier.go	Updated sanitizer policy from aggressive to ultra-aggressive (allows only p, br, h1-h6, strong, em, b, i tags with no attributes); added 1MB input size limit; implemented fallback to plain-text extraction when sanitized HTML is empty or markdown conversion fails; added new extractPlainText() helper function to strip scripts, styles, and HTML tags.
Test Coverage Updates common/pagetypeclassifier/pagetypeclassifier_test.go	Renamed and adjusted panic recovery tests to expect successful extraction; added assertions for non-empty results; introduced new subtests for large input handling (extremely large HTML) and plain-text extraction fallback validation (script/style exclusion).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Areas requiring extra attention:
  • Verify the ultra-aggressive sanitization policy doesn't inadvertently strip important content for the use case
  • Confirm the 1MB size limit is appropriate and gracefully handled in error scenarios
  • Review the fallback logic flow to ensure all edge cases (empty sanitized HTML, markdown conversion errors, panics) are correctly handled
  • Validate that extractPlainText() correctly removes script/style elements while preserving visible text content

Poem

🐰 With whiskers twitching, I sanitize the stream,
Ultra-aggressive now—a stricter, cleaner dream!
No sprawling divs or scripts to cause a fright,
Just plaintext fallbacks when markdown goes not-right.
A megabyte's enough; the HTML's made light! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: implementing panic protection with fallback mechanisms for the HTML parser to address deeply nested HTML issues.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/html-parser-panic-protection

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

common/pagetypeclassifier/pagetypeclassifier.go (3)

44-55: Sanitizer policy change aligns with goal; consider documenting text-retention behavior

The ultra-aggressive policy (only headings, p, br, and basic formatting tags) is consistent with minimizing nesting and keeping the parser safe. Since bluemonday.NewPolicy drops disallowed elements but keeps their inner text, you still preserve most page content for classification, which is what you want here.

It may be worth adding a short note in the comment that disallowed elements’ content is retained (not discarded), to reassure future maintainers that narrowing the tag set does not wipe out text signal for the classifier.

---

60-102: htmlToText fallbacks are good; consider enhancing panic recovery and rune-safety

The multi-step strategy (1MB cap → sanitize → html-to-markdown → plain-text fallback) looks solid and should prevent the previous parser panics while still extracting useful text.

Two possible improvements:

1. Use extractPlainText in the panic path as well.
   In the defer recovery you currently return an error and empty text, which forces Classify to fall back to "other". If you want “multiple fallbacks” to apply even on parser panics, you could salvage text via extractPlainText instead of returning empty:

func htmlToText(html string) (text string, err error) {
    defer func() {
        if r := recover(); r != nil {
-           err = fmt.Errorf("html parser panic: %v", r)
-           text = ""
+           // Recover from parser panic and fall back to plain text extraction.
+           text = extractPlainText(html)
+           err = nil
        }
    }()

2. Rune-safe truncation (optional).
   html = html[:maxHTMLSize] truncates by bytes, which can split multi-byte runes in UTF‑8 content. If you expect a lot of non-ASCII text and want to avoid malformed characters, consider truncating by runes (e.g., via []rune(html) or a streaming limiter) at some cost to performance.

Neither is a blocker, but they would make the behavior more robust and more in line with the documented “fallback” intent.

---

104-155: Plain-text fallback works but has sharp edges and could be more efficient

extractPlainText does the job for a last-resort fallback, but a few details are worth noting:

• The comment says “regex-based” but the implementation now uses index/search loops only; updating the comment would avoid confusion.
• In both script/style loops, if a closing tag is missing, you drop everything from the opening tag to the end of the document (text = text[:start]). That’s acceptable for a best-effort fallback, but it can erase a lot of legitimate text on malformed HTML; a safer option is to just break and keep the remainder.
• The tag-stripping loop builds result by repeatedly concatenating strings in a for over runes, which is O(n²) on large inputs. Using a strings.Builder (or bytes.Buffer) would keep it linear and cheaper on the worst‑case 1MB inputs.
• Script/style removal and tag matching are strictly lowercase ("<script", "<style", "</script>", "</style>"); if you care about robustness, you might want case-insensitive handling.

All of these are non-critical since this is a fallback path, but tightening them up would make the behavior more predictable and scalable.

common/pagetypeclassifier/pagetypeclassifier_test.go (2)

60-82: Deeply nested HTML tests are helpful; tighten assertions to validate behavior

The new subtests around deeply nested HTML are valuable and directly exercise the panic-protection and fallback logic.

A couple of tweaks could make them more meaningful:

• In test resilience with deeply nested HTML, require.NotEmpty and require.NotEqual(t, "", result) are redundant, and since Classify never returns an empty string, this test will always pass regardless of whether real text was extracted or if it just fell back to "other". Consider asserting on something stronger (e.g., require.NotEqual(t, "other", result) or checking via htmlToText as you do below) if you want to ensure the classifier is using extracted content.
• In test htmlToText with deeply nested HTML, the require.Contains(t, result, "Some text content") is exactly the kind of assertion that proves your sanitization/fallback pipeline now returns usable text; this is good.

No blockers here; these are just suggestions to better lock in the intended behavior.

Also applies to: 84-101

---

121-142: extractPlainText fallback test gives good coverage; consider one more positive assertion

This test does a nice job validating that extractPlainText:

• Produces non-empty output.
• Retains visible content such as "Title", "important", and "content".
• Excludes script/style content like "alert" and "color: red".

As a small enhancement, you might also assert that “Nested content” (inside <div><span>…</span></div>) survives, to confirm nested non-allowed container tags are handled as expected by the fallback too.

Otherwise, this test accurately captures the intended behavior.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b04cd9 and a260b32.

📒 Files selected for processing (2)

• common/pagetypeclassifier/pagetypeclassifier.go (3 hunks)
• common/pagetypeclassifier/pagetypeclassifier_test.go (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

common/pagetypeclassifier/pagetypeclassifier_test.go (2)

common/pagetypeclassifier/pagetypeclassifier.go (1)

• New (21-27)

runner/runner.go (1)

• New (117-406)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: Functional Test (macOS-latest)
• GitHub Check: Functional Test (windows-latest)
• GitHub Check: Functional Test (ubuntu-latest)
• GitHub Check: Analyze (go)
• GitHub Check: release-test

🔇 Additional comments (1)

common/pagetypeclassifier/pagetypeclassifier_test.go (1)

3-5: Large HTML test and strings usage look appropriate

Importing strings and using strings.Repeat to generate a >1MB HTML document is a clean way to exercise the new size-limiting behavior in htmlToText. The test asserts that:

• No error is returned.
• The result is non-empty.

This is a sensible regression test for the previous panic issue on very large inputs. If you ever want to be stricter, you could additionally assert that processing time stays reasonable via a benchmark, but that’s not necessary for unit tests.

Also applies to: 110-119