staging round 2: MHTML support (mirror of upstream #149) - #10
marcellmanfrin wants to merge 58 commits into
Conversation
Integrates, as one tree identical to candidate ee4aefd: - standalone HTML PR 147 head ce79482 (frameset + relative image fixes for the latest Cubic findings) - the MHTML resource-hardening follow-up validated in fork PR #4 (base64 pre-decode reserve check, encoded/decoded part size limits, MIME nesting depth bound, RTF/PDF detection precedence, README/metadata) Full gate passed on tree-identical candidate ee4aefd (fork Actions run on branch verify/mhtml-pr4-reconciled-full, success, 2026-08-30T08:12Z) and again locally on this exact commit. Canonical fixtures .local/fixtures/example.mhtml and example.root.html convert with clean Portuguese encoding on this tree.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 existing issue remains and 3 new issues found across 42 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/formats/html.rs">
<violation number="1" location="src/formats/html.rs:38">
P2: Standalone HTML preserves executable URL schemes such as `javascript:` in generated links. Override `link_target` for this context and allow only safe schemes (or reject `javascript:`, `data:`, and similar active schemes) before emitting Markdown.</violation>
</file>
<file name="src/formats/mhtml.rs">
<violation number="1" location="src/formats/mhtml.rs:27">
P2: An RFC 2557 MHTML file without a `Snapshot-Content-Location` header is not detected as Mhtml because `looks_like_mhtml` requires both `multipart/related` and that header. Such files already carry the `multipart/related` MIME type, which is the RFC container identity; consider treating a `multipart/related` content type alone as sufficient (or as a fallback) so valid archives without the Microsoft header still convert.</violation>
<violation number="2" location="src/formats/mhtml.rs:584">
P2: Base64 parts are limited by encoded size before decoded size, so valid parts larger than roughly 96 MiB are rejected despite fitting `MAX_ENTRY_BYTES`. Remove the encoded-size check and rely on the decoded upper-bound and final decoded-size checks.</violation>
</file>
Requires human review: Auto-approval blocked by 4 unresolved issues from previous reviews.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| } | ||
|
|
||
| let text = decode_html(bytes); | ||
| parse_text_with_context(&text, None, &StandaloneCtx, Vec::new()) |
There was a problem hiding this comment.
P2: Standalone HTML preserves executable URL schemes such as javascript: in generated links. Override link_target for this context and allow only safe schemes (or reject javascript:, data:, and similar active schemes) before emitting Markdown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/formats/html.rs, line 38:
<comment>Standalone HTML preserves executable URL schemes such as `javascript:` in generated links. Override `link_target` for this context and allow only safe schemes (or reject `javascript:`, `data:`, and similar active schemes) before emitting Markdown.</comment>
<file context>
@@ -0,0 +1,700 @@
+ }
+
+ let text = decode_html(bytes);
+ parse_text_with_context(&text, None, &StandaloneCtx, Vec::new())
+}
+
</file context>
| }; | ||
| let compact: String = content_type.chars().filter(|c| !c.is_ascii_whitespace()).collect(); | ||
|
|
||
| (compact == "multipart/related" || compact.starts_with("multipart/related;")) && snapshot |
There was a problem hiding this comment.
P2: An RFC 2557 MHTML file without a Snapshot-Content-Location header is not detected as Mhtml because looks_like_mhtml requires both multipart/related and that header. Such files already carry the multipart/related MIME type, which is the RFC container identity; consider treating a multipart/related content type alone as sufficient (or as a fallback) so valid archives without the Microsoft header still convert.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/formats/mhtml.rs, line 27:
<comment>An RFC 2557 MHTML file without a `Snapshot-Content-Location` header is not detected as Mhtml because `looks_like_mhtml` requires both `multipart/related` and that header. Such files already carry the `multipart/related` MIME type, which is the RFC container identity; consider treating a `multipart/related` content type alone as sufficient (or as a fallback) so valid archives without the Microsoft header still convert.</comment>
<file context>
@@ -0,0 +1,1098 @@
+ };
+ let compact: String = content_type.chars().filter(|c| !c.is_ascii_whitespace()).collect();
+
+ (compact == "multipart/related" || compact.starts_with("multipart/related;")) && snapshot
+}
+
</file context>
Review findings (staging PRs #11 and #10): - Duplicate <html>/<body> start tags are ignored by html5ever; pushing them moved the body-relative depth baseline upward and undercounted every later descendant. Skip a wrapper token when that wrapper is already on the modeled stack. - End-tag scope rules are now tag-specific like the parser's: the table family uses table scope (html, table, template), </li> uses list-item scope (adds ol/ul/button), </p> uses button scope (adds button), and svg/math foreign roots end every HTML scope search. Previously an end tag crossing a template, ol/ul, button, or foreign root truncated the modeled stack that html5ever leaves intact, undercounting depth. - Refactor the repeated inline depth-error assertions in tests/html.rs to the existing assert_preflight_depth_limit helper. Regressions: a duplicate <body> no longer moves the baseline (260 divs rejected before DOM construction); </p> blocked by button scope keeps 150+150 nested divs counted; </table> blocked by a template keeps 260 nested spans counted.
Review findings (staging PRs #11 and #10): - Duplicate <html>/<body> start tags are ignored by html5ever; pushing them moved the body-relative depth baseline upward and undercounted every later descendant. Skip a wrapper token when that wrapper is already on the modeled stack. - End-tag scope rules are now tag-specific like the parser's: the table family uses table scope (html, table, template), </li> uses list-item scope (adds ol/ul/button), </p> uses button scope (adds button), and svg/math foreign roots end every HTML scope search. Previously an end tag crossing a template, ol/ul, button, or foreign root truncated the modeled stack that html5ever leaves intact, undercounting depth. - Refactor the repeated inline depth-error assertions in tests/html.rs to the existing assert_preflight_depth_limit helper. Regressions: a duplicate <body> no longer moves the baseline (260 divs rejected before DOM construction); </p> blocked by button scope keeps 150+150 nested divs counted; </table> blocked by a template keeps 260 nested spans counted.
|
@cubic-dev-ai review this pull request |
@marcellmanfrin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/formats/html.rs">
<violation number="1" location="src/formats/html.rs:205">
P2: When `<html>` appears inside SVG, html5ever treats it as a foreign element, but this guard compares only local names and skips every subsequent `<html>`. Restrict duplicate-wrapper suppression to HTML content, or track namespaces, so foreign nesting cannot bypass the preflight depth check.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 42 files
Requires human review: Auto-approval blocked by 1 unresolved issue from a previous review of this commit.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
- close_element: </body> and </html> never truncate the open-element stack; html5ever only switches insertion modes and later start tags keep nesting, so truncating undercounted real open depth. - duplicate html/body wrapper suppression now skips foreign content, where <html> is an ordinary foreign element, not a duplicate wrapper. - is_end_tag_scope_marker: drop "button" from li list-item scope markers per the HTML5 spec; </li> must still close the item past an open button. - refresh stale hr comment: void tags reach the implied-close hook via close_implied, so <hr> does close an open <p>. Adds regression tests for all three behavior fixes.
The base64 arm enforced max_entry_bytes on the raw encoded body, but base64 expands data by 4/3: a part that decodes to exactly the limit has an encoded body ~33% larger and was wrongly rejected as over the limit. The decoded upper-bound check and the post-decode size check already cover the real limit, so the encoded-size check is removed. A regression test would need a ~128MB fixture, which is impractical for the suite; behavior is covered by the existing upper-bound and decoded-size error paths.
|
@cubic-dev-ai review this pull request |
@marcellmanfrin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 42 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Preflight fidelity fixes for the HTML complexity sink (cubic round C): - close_element: any-other end tags now stop at html5ever's full special category (plus svg/math foreign roots), not just the 9 generic scope markers. Previously </em> below <em><div><span> truncated real nesting the parser keeps open, undercounting depth and defeating the reject-before-DOM guarantee. The div family, button, form, headings, and template keep their generic-scope rule; dd/dt use button scope and rt/rp/rb/rtc use ruby scope, matching the parser. - close_element: </body>/</html> only stay no-ops in HTML content; inside foreign content they are ordinary foreign elements whose end tags pop. - close_element: option/optgroup end tags only check the current node, like html5ever; deeper searches truncated real nesting. - close_implied_before_start: li/dd/dt walk from the innermost element and stop at any special element other than address/div/p; p closes only within button scope; rt/rp pop only the current node; option pops only the current node and optgroup additionally closes an innermost optgroup inside a select context. - close_implied_before_start: table-family start tags (caption/col/ colgroup/tbody/td/tfoot/th/thead/tr) outside a table are ignored entirely, matching body-context parse errors; pushing them let a later implied close truncate divs opened between stray cells (undercount). Inside a table the row/body-context clears are unchanged. Adds 8 regression tests (66 html integration tests total).
The encapsulated-message preflight bounded base64/quoted-printable sizes but then recursed into the still-encoded bytes: a base64 body exposes no MIME headers to the raw scan, and a quoted-printable body can hide them behind =XX escapes, so the walk stopped at the first encoded level while mail-parser kept decoding and nesting. Deeply nested encoded message/rfc822 chains could therefore evade the max_mime_depth budget. Decode the bounded body (enforcing the decoded upper bound for base64, as the multipart-level walk already does) and recurse on the decoded bytes. Multipart bodies are still scanned raw: RFC 2046 forbids transfer encodings on multipart and mail-parser looks for the boundary in the undecoded bytes. Regression tests alternate base64/plain chain levels and quote-escape colons in quoted-printable chain levels; both fail without the decode step (verified) and hit max_mime_depth with it.
Cubic (run bad77c8a, conf6) proposed treating bare svg/math as pass-through in is_end_tag_scope_marker, aligning the markers with the in-body default_scope set. That proposal is rejected: while a bare svg/math root is reachable, html5ever resolves end tags in the foreign-content phase, whose walk stops at the first HTML element or integration point below the root and ignores the token — the same outcome as the modeled root stop. Integration points always sit below their root, so both marker sets stop identically; dropping the root stop instead would let scope-based end tags truncate through an open foreign root that the parser ignores, undercounting depth. Verified empirically: with svg/math made pass-through, this test fails (the deep foreign nesting passes preflight); with the root stop it rejects before DOM construction, matching the parser.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Cubic (run d03d9e02, tests/html.rs pin) proved — and html5ever 0.39 source confirms — that the previous root-stop model was wrong: - step_foreign (rules.rs 1652-1684): an end tag in foreign content walks the stack for a name match in ANY namespace and truncates there; on reaching an HTML element below the root without a match it REPROCESSES the token in the current insertion mode. - default_scope (tag_sets.rs): html_default_scope (incl. select) plus the MathML text and SVG HTML integration points; bare svg/math are NOT markers, so reprocessed scope searches pop through open foreign roots to an in-scope target (e.g. </p> under <p><svg><g> closes the p). - process_end_tag_in_body (mod.rs 1563): any-other end tags stop at special_tag (incl. isindex; integration points are not special). Changes: - close_element: unified walk without the svg/math root stop; body/html and option/optgroup use a foreign-region match (>= foreign_root_index) because their in-body rules never pop, while foreign-region matches do. - GENERIC_SCOPE_MARKERS: add select and the integration-point names (matched without namespaces; HTML title collision is harmless). - is_special_element: align exactly with html5ever special_tag. - pop_foreign_breakout refactored onto the new foreign_root_index helper. - Replace the wrong-premise pin test with two source-verified ones: scope end tags pop through foreign roots to their target (converts), and unmatched end tags still stop at special elements below roots (preflight rejects). All 68 html integration tests pass; fixtures byte-identical. Supersedes the round-C N2 rejection: the rejection was wrong about the mechanism (foreign-phase name-match pop is real); its undercount warning applies only to dropping the stop WITHOUT the region logic, which this commit implements properly.
|
@cubic-dev-ai review this pull request |
@marcellmanfrin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 42 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/formats/mhtml.rs">
<violation number="1" location="src/formats/mhtml.rs:187">
P2: When an MHTML multipart has no valid closing delimiter, this preflight treats EOF as a normal part end and allows truncated archives to convert. Track whether a proper closing delimiter was found and return `Malformed` when the multipart ends without one.</violation>
</file>
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| ))); | ||
| } | ||
|
|
||
| let next_parent = find_marker(bytes, body_start, end, &marker, true).unwrap_or(end); |
There was a problem hiding this comment.
P2: When an MHTML multipart has no valid closing delimiter, this preflight treats EOF as a normal part end and allows truncated archives to convert. Track whether a proper closing delimiter was found and return Malformed when the multipart ends without one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/formats/mhtml.rs, line 187:
<comment>When an MHTML multipart has no valid closing delimiter, this preflight treats EOF as a normal part end and allows truncated archives to convert. Track whether a proper closing delimiter was found and return `Malformed` when the multipart ends without one.</comment>
<file context>
@@ -0,0 +1,1221 @@
+ )));
+ }
+
+ let next_parent = find_marker(bytes, body_start, end, &marker, true).unwrap_or(end);
+
+ if headers_use_base64(headers) {
</file context>
Cubic round D (PR #10 run 10ff97ec, P2 conf8): html5ever ignores a stray frame start tag in body context and inserts-then-immediately-pops it in frameset context, so frames never nest in the real DOM. The preflight stacked every frame, falsely reporting max_xml_depth for framesets with more than 256 sibling frames. Treat frame like the void elements in HTML content (it still pushes inside foreign content, where it is an ordinary foreign element). Verified discriminating: without the fix, the new frameset test fails; with it, 70/70 html tests pass. Also pins two round-D rejections grounded in html5ever 0.39 source: - markup inside <select> really nests (html5ever has no in-select insertion mode; InBody inserts it), so the depth guard must fire — ignoring select content would undercount the real DOM. - the unconditional title/textarea raw-text switch cannot hide deep nesting: every mode that accepts those tags inserts them, and the frameset modes that ignore them also ignore all later start tags (comment only, no behavior change).
Cubic round D (PR #10 run 10ff97ec, P2 conf8): html5ever ignores a stray frame start tag in body context and inserts-then-immediately-pops it in frameset context, so frames never nest in the real DOM. The preflight stacked every frame, falsely reporting max_xml_depth for framesets with more than 256 sibling frames. Treat frame like the void elements in HTML content (it still pushes inside foreign content, where it is an ordinary foreign element). Verified discriminating: without the fix, the new frameset test fails; with it, 70/70 html tests pass. Also pins two round-D rejections grounded in html5ever 0.39 source: - markup inside <select> really nests (html5ever has no in-select insertion mode; InBody inserts it), so the depth guard must fire — ignoring select content would undercount the real DOM. - the unconditional title/textarea raw-text switch cannot hide deep nesting: every mode that accepts those tags inserts them, and the frameset modes that ignore them also ignore all later start tags (comment only, no behavior change).
|
@cubic-dev-ai review this pull request |
@marcellmanfrin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 42 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Cubic round E (PR #11 run e74d240e, P2 conf9): html5ever's InTable arms for caption, col, colgroup, tbody/tfoot/thead, and td/th/tr all begin with pop_until_current(table_scope) — stray foster-parented content above the table leaves the open-element stack. The preflight kept it, so repaired table markup with deep stray content accumulated phantom depth and could be falsely rejected at max_xml_depth. Model the clear (stopping at the innermost table/template/html, exactly the table_scope set); the phantom tbody/tr wrappers html5ever inserts afterwards remain unmodeled (bounded, documented deviation). The previous per-tag rposition truncation is subsumed by the clear and was removed — it could also reach below the current table for stale matches. Also strengthens the raw-text tests (P3 conf7): the script test moves into the body and asserts the script source is NOT rendered, so it actually distinguishes ScriptData swallowing from data-state parsing; the title test gains the same negative assertion. Regression test: table_family_starts_clear_stray_content_above_the_table (253 stray divs then tbody/tr/td must convert). Discrimination verified: without the clear the test fails; with it, 71/71 html tests pass.
|
@cubic-dev-ai review this pull request |
@marcellmanfrin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 42 files
You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Cubic round F (PR #11 run 7cf7afee, P2 conf9): when a block start tag arrived with an open <p> holding inline descendants, the preflight left the paragraph (and its descendants) on the modeled stack — html5ever closes any button-scope p first (close_p_element_in_button_scope, mod.rs), popping the descendants with it. The stale p was worse than an overcount: the next p start's button-scope walk would find it BELOW intervening blocks opened since (blockquote, etc.) and truncate through them, undercounting the nesting the parser really keeps open — <blockquote><p><b> repeated stays flat in the model while html5ever nests the blockquotes without bound, defeating the reject-before-DOM guarantee. Walk with the button-scope markers for every paragraph-closing block start (p included, so the duplicate p-start branch is removed). Regression test block_starts_close_paragraphs_with_inline_descendants: 255 iterations must reject at preflight depth. Discrimination verified against the exact previous state (innermost-only block close + p-start walk): the test fails there and passes with the fix. 72/72 html tests.
1 participant
Staging PR for Cubic ultrareview round 2 — mirrors the content of firecrawl#149 at candidate 932bf18 (round-1 staging #7 + HTML round-2 cherry-pick + ultrareview fixes: encapsulated message preflight, boundary/header caps, QP delimiter bound, encoding-problem rejection, base64 encoded-size bound). Do not merge; the real review happens upstream.
Summary by cubic
Adds standalone HTML and MHTML input support so
.html,.mhtml, and.mhtfiles now convert to GitHub-Flavored Markdown instead of failing as unrecognized formats.New Features
<html>marker, and MHTML from amultipart/relatedMIME type with archive evidence such asSnapshot-Content-Location.htmlandmhtmlformats in the Node, Python, and WASM bindings, content/extension detection, and CLI help.Hardening
message/rfc822chains recursively against those limits, decoding transfer-encoded bodies first so encoded chains can't hide their nesting./,?, or#.frametags never stack.Written for commit cfd9b80. Summary will update on new commits.