Align literal-mode and marked-section handling with browsers on malformed input by oalders · Pull Request #64 · libwww-perl/HTML-Parser

oalders · 2026-07-27T21:31:59Z

Problem

While reviewing literal-mode and marked-section handling I found several cases where the parser's output diverges from how browsers treat the same malformed or truncated input. The common thread: bytes that a browser keeps as an element's raw text (or discards) are instead re-parsed as markup, so tags following an unclosed or mishandled construct surface as real start/end events.

The clearest example needs no exotic input:

HTML::Parser->new(api_version => 3)->parse("<script>safe<img src=x onerror=boom>")->eof;
# before: reports <img> as a start tag
# browsers: <img> is raw script text, no element is created

An end tag broken by an embedded NUL (</script\0>) is just one more way to leave the element unclosed and reaches the same path.

Fix

Unclosed literal elements at EOF. Report the remaining bytes of an unclosed script, style, title, xmp, iframe, or textarea as the element's raw text, as browsers do, instead of re-parsing them as markup.
Implicit end events. Emit the implicit end of an unclosed literal element after its text — for xmp, iframe, and textarea too (plaintext has no end tag and gets none) — with the start tag's name as written under case_sensitive, and not at all inside an element that ignore_elements is skipping.
End tags with attributes. Close a literal element at an end tag carrying attributes or a slash, e.g. </script foo=bar>, as browsers do. strict_end restores the old behaviour.
Marked sections. Recognise TEMP as a status keyword; close a section whose keywords are all unrecognised instead of leaving the ]]> in the text; stop reporting a zero-length text event at the ]]> of an INCLUDE section; and clear marked-section state at EOF so a later document is not parsed as if a section were still open.
EOF state. Clear the eof flag when a handler stops the parse from inside eof(), so the next document parsed by the same object is not discarded, and avoid a crash when such a handler drops the last reference to the parser.
HeadParser. Recover only the character encoding from a <meta> inside a title that is never closed, and set no other header from it. See ENCODING DECLARATIONS in HTML::HeadParser.

Impact

Well-formed input is unaffected — properly-closed elements and well-formed marked sections never reach these paths.
Only malformed or truncated input changes, and it changes toward browser conformance.
No API changes; strict_end is opt-in for the one behaviour that could matter to existing callers.

Tests

New and expanded tests cover unclosed literal elements at EOF (all six elements, both NUL placements, and the plain-unclosed case), implicit end events, end tags with attributes, marked-section keywords and EOF state, the eof() re-entry cases, and the HeadParser title-encoding path. Full suite passes.

Investigated and implemented by Claude (Opus 4.8, model claude-opus-4-8).

🤖 Generated with Claude Code

oalders · 2026-07-27T21:40:57Z

@pjcj could I get a review from you?

oalders

Code Review Intense Flow Summary

Reviewers Run

✅ General code-reviewer (always runs)
✅ Security review (C parser code touched; parsing-divergence class)
⏭️ Frontend (skipped: no UI files changed)
⏭️ SEO (skipped: no page templates/meta/routes)
⏭️ GEO (skipped: no content pages/schema)
⏭️ Playwright (skipped: no e2e tests, routes, or client-side interaction; new test is a Perl unit test)

Verdict

Both reviewers found no Critical issues and confirmed the fix is correct and complete. Both rebuilt the base commit and verified empirically that the old code reported the injected <img> as a real start event for </script\0>-style input, and that the fixed code reports the tail as a single text event across single-shot, chunked, mixed-case, and reused-parser scenarios. Full suite passes.

Security assessment: safe to merge; the change strictly reduces risk (closes a tokenizer-vs-browser divergence usable as a sanitizer-bypass primitive) and introduces no new memory-safety or DoS surface — the new report_event(E_TEXT, s, end, …) stays within the SvPV buffer, and s = end guarantees loop termination at lower cost than the old parse_buf re-entry.

Notable verified subtleties:

is_cdata reset ordering is correct: the tail text event fires before is_cdata = 0, so entity/dtext handling of the tail matches normal CDATA-element text, and the reset prevents state bleed into a reused parser.
Chunked input is safe: mid-stream, a NUL-broken close tag fails the close-tag check in parse_buf and the buffer is retained until EOF, so this EOF-only fix covers all arrival patterns.
xml_mode never enters literal mode, so this path is unreachable there.
Event stream stays balanced: START → TEXT(tail) → END for script/style (implicit) and title (via pending_end_tag).
The Changes entry is accurate: only script/style/title are newly affected; xmp/iframe/textarea/plaintext already took the safe "rest is text" path.

Inline comments cover the two Important findings (one pre-existing asymmetry worth a maintainer decision, one test-coverage gap — the latter already addressed in a follow-up commit) and one Minor cosmetic point.

🤖 Review by Claude Code · model: claude-fable-5[1m]

oalders · 2026-07-27T21:44:54Z

 			/* rest is considered text */
 			break;
                    }
+		    /* Unclosed script/style/title at end of document.  A browser


[general] Important — pre-existing EOF asymmetry across literal-mode elements. This fix gives script/style an implicit close event and title a pending_end_tag close at EOF, but xmp/iframe/textarea/plaintext hit the early break just above and fall through to the generic "rest is text" path with no synthesized end event — the element is left open when literal_mode/is_cdata are zeroed during state reset (verified: <textarea>foo at EOF produces a start but no matching end). This is not a regression from this PR (those four elements were already safe from the tail-re-parse bug, and the Changes entry correctly lists only script/style/title), but the resulting asymmetry is worth an explicit maintainer decision: either accept it as documented behaviour or extend end-event synthesis to the other four in a follow-up.

oalders · 2026-07-27T21:44:54Z

+		     * every browser.  So report the tail as text -- respecting the
+		     * element's CDATA-ness, still recorded in p_state->is_cdata --
+		     * then emit the end event, but never re-parse it. */
+		    if (s < end)


[general+security] Minor — if (s < end) is always true here: nothing between the enclosing while (s < end) loop guard and this check mutates s. Harmless; either drop it or keep it as a defensive mirror of the genuinely-needed guard in the EOF flush path below (line ~1793) — if kept, a one-line comment saying it's defensive would stop future readers from hunting for the mutation.

oalders · 2026-07-27T21:44:54Z

+    $parser->parse($html);
+    $parser->eof;
+
+    my $saw_img = grep { $_ eq 'img' } @start_tags;


[general+security] Important — test asserts only the negative, and only single-shot input. As committed, the test checks that no spurious <img> start event fires, but never asserts the positive contract from the Changes entry: that the tail (NUL included) is actually reported verbatim as text, and that it precedes the synthesized close event. It also never exercises chunked ->parse(...); ->parse(...); ->eof with the boundary inside the broken close tag — the arrival pattern the parser's buffering design exists for. A future regression that silently dropped the tail text or the close event would pass this test. Addressed in a follow-up commit on this branch: the hardened test adds verbatim-text, NUL-survival, and close-event-ordering assertions plus a chunked mode per case (18 cases → 132 assertions, all passing).

An unclosed literal-mode element (script, style, title) at the end of a document had its trailing bytes re-parsed as markup by the EOF flush, which exposed following tags as elements. This diverges from browsers, which keep the remaining bytes as the element's raw text and close it implicitly. The embedded NUL byte from the original repro is only one way to leave the element unclosed; a plainly unclosed element triggers the same behaviour with no NUL involved. Report the tail as text (preserving the element's CDATA-ness) and emit the end event instead of re-parsing it, matching browser behaviour. Well-formed input is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

- Hand the bytes left unparsed after an unclosed literal element back to `parse_buf` when a marked section is open, so its `]]>` still closes the section instead of becoming element text. - Clear marked section state at EOF, since an `IGNORE` section left open suppresses every event in the next document parsed by the same object. - Reword the entry for the unclosed literal element change to record the new event order.

- `TEMP` is a status keyword in ISO 8879 with the same effect as `INCLUDE`, but the parser did not know it, so it left the `]]>` in the document text rather than closing the section.

- The terminator check uses the section mode to spot `]]>`, so a section whose status keywords were all unrecognised could never be closed and leaked its `]]>` into the document text. - Fall back to `INCLUDE`, which `parse_marked_section` already pushes as the default when a section is written with no keywords at all. A recognised keyword still wins, so `<![FOO IGNORE[` is unaffected.

- Note which keywords are recognised, that the most restrictive of several wins, and that a section with no recognised keyword falls back to `INCLUDE` rather than being rejected as SGML would have it.

- A handler that calls `$p->eof` while the EOF flush is running sets the flag, but the flush path never clears it, so the next document parsed by the same object is dropped and reports the previous document's pending end tag in place of its own events. - Present since b3822f8 stopped the flush recursing by marking it as parsing, which turned the nested call into a flag that nobody reset.

- A title that is never closed swallows the rest of the head as its text, so a `<meta>` there is no longer an element and set no header at all, losing the response charset `LWP::UserAgent` relies on. - Scan the title's text for the two forms the HTML5 prescan recognises, since browsers find the encoding without tracking element context. Everything else stays lost, so a truncated document can no longer contribute an `X-Meta-Keywords`, a `Refresh` or a `Set-Cookie`.

- The INCLUDE terminator path reported text unconditionally, where the CDATA path fires the event only when text precedes the terminator, so `<![INCLUDE[]]>` produced a zero-length text event, as did a terminator directly after a tag inside the section. - Apply the same guard as the CDATA path.

- The close-tag detector required ">" straight after the name, so </script foo=bar> or </script/> kept the parser in literal mode where browsers close the element. The parser said inert where a browser executes, the dangerous direction for a scanner. - Accept whitespace, "/" or ">" after the name, as HTML5 does, and find the real ">" with the same quote handling end tags get outside literal mode. strict_end keeps the old requirement.

- The implicit end for an unclosed script, style or title was emitted only from the EOF buffer flush, so a document ending exactly at the start tag got a start event and nothing else, while a single trailing byte produced the end event. - Extract the emission into report_literal_end and call it after the flush too, when literal mode is still set with nothing buffered.

- The end-of-document event flushed pending_end_tag after the unbalanced-document cleanup had already cleared ignoring_element, so an unclosed title inside an element skipped by ignore_elements leaked its implicit end event through the filter. - Flush it explicitly after the pending text, while the ignore state still applies. Event order is unchanged for unfiltered documents.

- A handler that dropped the last reference to the parser while eof() was flushing left the XS call working on a freed object, which segfaulted. parse() has held a reference across its handlers since b83f708, but eof() runs handlers too and was never given the same treatment.

- Only script, style and title reported an implicit end when the document ran out with the element open, so xmp, iframe and textarea gave a start and text with no end at all. - Route on is_cdata, which is the rule the named elements already followed, and run it for every literal element but plaintext, which has no end tag.

- The branch reported its text and ended the element itself, then set s to end so the loop would exit, duplicating the shared text report just below it. Its `s < end` test could never be false either, since the enclosing loop already guarantees it. - Break instead, as the branch above already does, and let the shared report and the end-of-flush block do the work. No change in output.

- The single negative assertion in t/nul-literal-mode.t passed for a parser that emitted nothing at all, and twelve of the eighteen cases passed without the fix they were written for. - Compare the whole event list instead, in the style of t/literal-end-tag.t, so every case now fails against the old close-tag detector.

- At EOF an unclosed script or style inside a marked section reported its implicit end before its text, because the flush emitted the end and then handed the remaining bytes to parse_buf. Title deferred its end as pending_end_tag and came out text first. - Route script and style through the same deferral, so all three match the order used outside marked sections.

- Add `report_synthetic_end` and call it from the three sites that built the same end event by hand. - Keep clearing `pending_end_tag` at the call sites, since the helper recurses into `report_event` and would loop otherwise. - No behaviour change.

- Build the synthetic end from the name as written rather than the static lowercase name literal mode matched against, since under case_sensitive the pair disagreed and a report_tags filter naming the tag as written missed the end event entirely. - The defect is pre-existing for script and style. The other literal elements had no implicit end at all before this branch.

- The EOF flush for an unclosed script, style or title inside a marked section handed the whole remainder back to the parser, which is the re-parsing this branch exists to stop. A close tag broken by an embedded NUL exposed the tags after it as elements again whenever marked sections were on. - Scan for the section terminator instead, since only it is structure in those bytes. The text before it stays the element's text, the element closes there, and the parser resumes at the terminator, whose existing handling pops the section.

- The POD claimed HeadParser prescans like a browser, reading the leading bytes regardless of element, but the scan reads only a title's text. A charset inside an unclosed script or style is still lost, as both elements are ignored wholesale. That loss predates this branch. - Reword the section to describe that boundary, and assert it in t/headparser.t.

- The scan of an unclosed title's text is meant to recover the encoding and nothing else, but a meta carrying both a charset and an http-equiv attribute was accepted for its charset and then set the header its http-equiv named. So a truncated title could still set a header such as Set-Cookie, and the charset itself was lost. - Decide the header inside the scan and push it directly, so the attribute that admits the meta is the attribute that names the header. - Add a charset attribute to copies of the three existing guard cases. Without one the scan rejected them outright, so they never exercised the header choice they were written to check.

The EOF flush sets no_dash_dash_comment_end when it recovers unterminated markup at the end of a document, but the end-of-document reset never cleared it. Once set, it stuck for the life of the parser object and silently changed how comments in a later document parsed by the same object were terminated (a lone > closing a comment instead of -->). Reset it alongside the other per-document state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5bVzBjNdZJuhS1dSpHsh7

dup_pstate() copied literal_mode (a pointer to a static string) but not literal_mode_name, the buffer holding the open element's as-written name. A parser cloned across ithreads while a literal element was open would leave the clone's name buffer empty and report the implicit end event with an empty tag name. Copy the buffer alongside literal_mode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5bVzBjNdZJuhS1dSpHsh7

The Copy into the fixed literal_mode_name buffer was bounded only by an assert, which a release build compiles out under NDEBUG. The length can only be one of the known literal element names today, so it always fits, but a future longer name would overflow the buffer silently. Add a runtime croak so the bound holds regardless of NDEBUG. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5bVzBjNdZJuhS1dSpHsh7

Note in the HeadParser POD that the meta prescan runs on the text of every title, not only an unclosed one, so a reader does not assume it fires only at EOF. Add a comment on why the marked-section EOF flush reports the implicit end directly instead of through report_literal_end. Documentation only; no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5bVzBjNdZJuhS1dSpHsh7

report_literal_end takes the deferred pending_end_tag path for a non-cdata element, not a cdata one; the comment had it backwards. Comment only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5bVzBjNdZJuhS1dSpHsh7

oalders commented Jul 27, 2026

View reviewed changes

oalders changed the title ~~Report unclosed script/style/title tail as text at EOF~~ Align literal-mode and marked-section handling with browsers on malformed input Sep 1, 2026

oalders and others added 27 commits September 1, 2026 19:14

Recognise TEMP as a marked section keyword

d75539f

- `TEMP` is a status keyword in ISO 8879 with the same effect as `INCLUDE`, but the parser did not know it, so it left the `]]>` in the document text rather than closing the section.

Document marked section status keywords

2d6aa8e

- Note which keywords are recognised, that the most restrictive of several wins, and that a section with no recognised keyword falls back to `INCLUDE` rather than being rejected as SGML would have it.

Document the implicit end of literal elements

dde8b67

Condense the Changes entries

dbefb57

oalders force-pushed the nul-byte branch from 733ac1c to dab4520 Compare September 1, 2026 19:30

Navigation Menu

Uh oh!

Align literal-mode and marked-section handling with browsers on malformed input - #64

Align literal-mode and marked-section handling with browsers on malformed input#64
oalders wants to merge 28 commits into
masterfrom
nul-byte

oalders commented Jul 27, 2026 •

edited

Loading

Uh oh!

oalders commented Jul 27, 2026

Uh oh!

oalders left a comment

Uh oh!

oalders Jul 27, 2026

Uh oh!

oalders Jul 27, 2026

Uh oh!

oalders Jul 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

oalders commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Impact

Tests

Uh oh!

oalders commented Jul 27, 2026

Uh oh!

oalders left a comment

Choose a reason for hiding this comment

Code Review Intense Flow Summary

Reviewers Run

Verdict

Uh oh!

oalders Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

oalders Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

oalders Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oalders commented Jul 27, 2026 •

edited

Loading