fix(postprocess): keep the apply and skip-list walks in sync on tag interiors

Re-review of cbb6b2a surfaced two more instances of the same
divergence class, one on each side.

The nofollow path advanced only past "<a " after injecting rel,
leaving the cursor inside the href value; a <!-- or raw-text tag name
planted there (the comment form survives the default tagfilter) was
probed as a region opener and the closerless region ran to
end-of-document, stripping rel from every later link and id from every
later heading. The cursor now jumps the whole anchor tag with
scan_tag_close while the attribute bytes still flush verbatim.

compute_skip_list treated scan_tag_closes html_len return
(unterminated tag) as a jump to end-of-document and stopped recording
skip regions, while apply_transforms advances one byte past the same
tag and keeps skipping. The desynced skip list let a heading
fingerprint resolve inside a region the apply pass then skipped, and
the heading lost its id. The unterminated return now advances one byte
there too, matching the apply pass.

Both reproduced against the built extension before the change; the new
tests fail on the parent commit and pass now, full suite 35/35.

Found by a codesage re-review of the fix commits.

Author: iliaal

CHANGELOG.md

@@ -13,6 +13,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- `nofollowLinks`: a skip-region opener (`<!--`, `<script>`, ...) inside an
+  `href` value in raw HTML under `unsafe: true` fabricated a skip range to
+  end-of-document, silently stripping `rel="nofollow"` from every later
+  link and `id` from every later heading. Regression test:
+  `tests/044_nofollow_attr_opener.phpt`.
+- `headingAnchors`: an unterminated raw tag truncated the precomputed
+  skip list while the apply pass kept honoring later skip regions; the
+  desync silently dropped the `id` of a heading whose fingerprint also
+  appeared in such a region. Regression test:
+  `tests/045_skiplist_unterminated_tag.phpt`.
 - Module lifecycle: a second MINIT after MSHUTDOWN in the same process
   (embedded SAPI cycle, a dlclose that doesn't unload) failed with
   E_CORE_ERROR because cmark's one-shot registration guards survived

mdparser_html_postprocess.c

@@ -368,8 +368,14 @@ static bool compute_skip_list(const char *html, size_t html_len,
             i = (end > i) ? end : i + 1;
             continue;
         }
+        /* html_len means an unterminated tag: advance one byte and keep
+         * scanning, exactly like apply_transforms' literal-byte path.
+         * Jumping to end-of-document instead would leave every later
+         * skip region unrecorded while apply_transforms still honors
+         * them, and the desynced skip list lets a heading fingerprint
+         * resolve inside a region the apply pass then skips. */
         size_t tag_end = scan_tag_close(html, i, html_len);
-        i = (tag_end != SIZE_MAX && tag_end > i) ? tag_end : i + 1;
+        i = (tag_end != SIZE_MAX && tag_end < html_len) ? tag_end : i + 1;
     }
     return true;
 }
@@ -878,8 +884,17 @@ static zend_string *apply_transforms(const char *html, size_t html_len,
                 }
                 smart_str_appendl(&out, "<a ", 3);
                 smart_str_appendl(&out, rel_inject, rel_inject_len);
-                i += 3;
-                run_start = i;
+                /* Jump the rest of the anchor tag with the quote-aware
+                 * scanner. Advancing only past "<a " leaves the cursor
+                 * inside the href value, where an embedded region
+                 * opener (`<!--`, `<script>`) fabricates a skip region
+                 * running to end-of-document. The attribute bytes still
+                 * flush verbatim from run_start; an unterminated tag
+                 * (html_len) falls back to the old +3 advance and the
+                 * loop's literal-byte handling. */
+                size_t a_end = scan_tag_close(html, i, html_len);
+                run_start = i + 3;
+                i = (a_end != SIZE_MAX && a_end < html_len) ? a_end : i + 3;
                 continue;
             }
         }

tests/044_nofollow_attr_opener.phpt

@@ -0,0 +1,61 @@
+--TEST--
+nofollowLinks: skip-region opener inside an href must not eat later transforms
+--EXTENSIONS--
+mdparser
+--FILE--
+<?php
+
+/* After injecting rel into `<a href="`, apply_transforms used to
+ * advance only past the "<a " bytes, leaving the cursor inside the
+ * href value. A `<!--` or raw-text tag name embedded there (reachable
+ * under unsafe; the comment form survives the default tagfilter) was
+ * then probed as a region opener, and the closerless region ran to
+ * end-of-document, stripping rel from every later link and id from
+ * every later heading. The cursor now jumps the whole anchor tag,
+ * mirroring compute_skip_list's quote-aware walk. */
+
+function check(string $label, bool $cond): void {
+    echo ($cond ? "OK" : "FAIL"), ": $label\n";
+}
+
+$p = new MdParser\Parser(new MdParser\Options(unsafe: true, nofollowLinks: true));
+
+$h = $p->toHtml("<a href=\"x<!--y\">l</a>\n\n[z](http://example.com/)\n");
+check("comment opener in href: later link keeps rel",
+    str_contains($h, '<a rel="nofollow noopener noreferrer" href="http://example.com/">z</a>'));
+check("comment opener in href: decoy anchor itself got rel",
+    str_contains($h, '<a rel="nofollow noopener noreferrer" href="x<!--y">l</a>'));
+
+// Control: same document without the embedded opener.
+$h = $p->toHtml("<a href=\"xy\">l</a>\n\n[z](http://example.com/)\n");
+check("control: later link keeps rel",
+    str_contains($h, '<a rel="nofollow noopener noreferrer" href="http://example.com/">z</a>'));
+
+/* Raw-text tag name in the href needs tagfilter off to survive. */
+$p = new MdParser\Parser(new MdParser\Options(unsafe: true, tagfilter: false, nofollowLinks: true));
+$h = $p->toHtml("<a href=\"x<script>y\">l</a>\n\n[z](http://example.com/)\n");
+check("script opener in href: later link keeps rel",
+    str_contains($h, '<a rel="nofollow noopener noreferrer" href="http://example.com/">z</a>'));
+
+/* Heading anchors after the decoy must survive too. */
+$p = new MdParser\Parser(new MdParser\Options(unsafe: true, nofollowLinks: true, headingAnchors: true));
+$h = $p->toHtml("<a href=\"x<!--y\">l</a>\n\n# foo\n");
+check("comment opener in href: later heading keeps id",
+    str_contains($h, '<h1 id="foo">foo</h1>'));
+
+/* A real region after the decoy anchor is still honored. */
+$h = $p->toHtml("<a href=\"x<!--y\">l</a>\n\n<!-- <a href=\"http://spam.example/\">s</a> -->\n\n[z](http://example.com/)\n");
+check("real comment after decoy: body untouched",
+    str_contains($h, '<!-- <a href="http://spam.example/">s</a> -->'));
+check("real comment after decoy: later link keeps rel",
+    str_contains($h, '<a rel="nofollow noopener noreferrer" href="http://example.com/">z</a>'));
+
+?>
+--EXPECT--
+OK: comment opener in href: later link keeps rel
+OK: comment opener in href: decoy anchor itself got rel
+OK: control: later link keeps rel
+OK: script opener in href: later link keeps rel
+OK: comment opener in href: later heading keeps id
+OK: real comment after decoy: body untouched
+OK: real comment after decoy: later link keeps rel

tests/045_skiplist_unterminated_tag.phpt

@@ -0,0 +1,41 @@
+--TEST--
+headingAnchors: unterminated raw tag must not truncate the skip list
+--EXTENSIONS--
+mdparser
+--FILE--
+<?php
+
+/* scan_tag_close returns html_len for a tag with an unclosed quote.
+ * compute_skip_list used to take that as a jump-to-end and stopped
+ * recording skip regions, while apply_transforms advances one byte
+ * past the same tag and keeps skipping regions. The desync let
+ * resolve_heading_offsets place a heading fingerprint inside a region
+ * apply_transforms then skipped, so the real heading silently lost
+ * its id. compute_skip_list now treats the unterminated tag the same
+ * way: advance one byte, keep scanning. */
+
+function check(string $label, bool $cond): void {
+    echo ($cond ? "OK" : "FAIL"), ": $label\n";
+}
+
+$p = new MdParser\Parser(new MdParser\Options(headingAnchors: true, unsafe: true, tagfilter: false));
+
+$h = $p->toHtml("<div data=\"x\n\n<style>\n<h1>foo</h1>\n</style>\n\n# foo\n");
+check("unterminated tag + decoy region: heading keeps id",
+    str_contains($h, '<h1 id="foo">foo</h1>'));
+
+// Control: terminated tag, same decoy region.
+$h = $p->toHtml("<div data=\"x\">\n\n<style>\n<h1>foo</h1>\n</style>\n\n# foo\n");
+check("control: heading keeps id", str_contains($h, '<h1 id="foo">foo</h1>'));
+
+// Comment-region variant works with the default tagfilter.
+$p2 = new MdParser\Parser(new MdParser\Options(headingAnchors: true, unsafe: true));
+$h = $p2->toHtml("<div data=\"x\n\n<!-- <h1>foo</h1> -->\n\n# foo\n");
+check("unterminated tag + decoy comment: heading keeps id",
+    str_contains($h, '<h1 id="foo">foo</h1>'));
+
+?>
+--EXPECT--
+OK: unterminated tag + decoy region: heading keeps id
+OK: control: heading keeps id
+OK: unterminated tag + decoy comment: heading keeps id