Repository navigation
Add POST /convert/html-to-pdf — the missing conversion direction - #7
Conversation
Closes #5. /convert/* exposed markdown-to-pdf, markdown-to-html, pdf-to-markdown and html-to-markdown. The one missing combination was HTML -> PDF, which is what a caller needs whenever the document is a LAYOUT rather than prose. This adds no rendering capability: htmlToPdf() already existed and markdown-to-pdf already calls it as its second stage. The route wraps it, with the same bearer-token gate, the same application/pdf default and base64 variant, and the same JavaScript-disabled, remote-blocked rendering. The caller's document is rendered VERBATIM — no wrapper, no default stylesheet. That is the point of the direction: going through Markdown to produce a positioned document means embedding raw HTML in Markdown and fighting the converter over what it escapes, wraps or reflows. Accepts JSON { html, options? } or a raw text/html body, mirroring html-to-markdown, since app.ts already registers a text/html parser and a caller sending one would otherwise get a confusing schema error. Two option semantics worth stating, because a complete document makes them ambiguous: - `css` is APPENDED after the document's own styles, last in <head>, matching what "extra CSS" means on the Markdown path. It falls back to the top of <body>, then to the front of the string, so a fragment still gets it rather than silently dropping it. The </style> breakout is neutralized the same way markdownToHtml does it. - `title` sets the download filename only. The document already carries a <title> and overwriting it would be an edit to the caller's markup. injectExtraCss uses FUNCTION replacements rather than string ones. A `$&` or `$1` in caller CSS would otherwise be interpreted as a replacement pattern and silently corrupt the stylesheet, and re-emitting a literal `</head>` would rewrite a caller's `</HEAD>` to lowercase — a silent edit to a document this endpoint promises to render verbatim. The second was caught by its own test. Tests: 7 unit tests for injectExtraCss (including the $-pattern and case cases), 4 route validation tests, and 4 Chromium round-trips — verbatim render, raw text/html body, an inlined data: URI image under the remote-block hardening, and the css option. 39 passing. Docs: README gains a section covering the endpoint plus the two traps a layout caller hits — inline assets as data: URIs, because a remote reference renders as nothing and still returns 200; and fonts are the image's, so embed the face if exact typography matters. The README endpoint table was also missing markdown-to-html and html-to-markdown; added, since a table that lists some endpoints reads as authoritative. Worker: Nils (backend-engineer) <hrn:worker:micromentor.org:mm-eng-team:nils> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a551af88e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (/<body[^>]*>/i.test(html)) { | ||
| return html.replace(/<body[^>]*>/i, match => `${match}\n${block}`); | ||
| } | ||
| return `${block}\n${html}`; |
There was a problem hiding this comment.
Preserve the doctype when prepending CSS
For a standards-compliant document that omits the optional <head> and <body> tags, such as <!DOCTYPE html><title>Invoice</title><main>...</main>, this fallback places the <style> element before the doctype. Chromium then ignores the late doctype and enters quirks mode, which can materially change the layout being rendered. Insert after an initial doctype or use an HTML-aware insertion strategy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 858aab4. This was the more serious of the two — quirks mode changes the box model, and it would have done so on the one endpoint whose entire purpose is rendering a layout the caller has already positioned. A silent geometry shift in a document people keep is about the worst failure shape available here.
Insertion now goes after any leading doctype. Pinned by a test using your exact example (<!DOCTYPE html><title>Invoice</title><main>…</main>), verified to fail against the previous implementation.
| if (/<\/head>/i.test(html)) { | ||
| // Re-emit the MATCHED text rather than a literal `</head>`: this endpoint | ||
| // renders the caller's document verbatim, and rewriting their `</HEAD>` to | ||
| // lowercase would be a silent edit to it. | ||
| return html.replace(/<\/head>/i, match => `${block}\n${match}`); |
There was a problem hiding this comment.
Avoid matching closing-head text inside comments
When the supplied document contains </head> text inside a comment or raw-text element before the actual closing tag—for example <head><!-- example: </head> --></head>—this replacement selects that occurrence and inserts the stylesheet inside the comment, so options.css is silently ignored. Locate the actual head element rather than the first matching substring.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 858aab4. The part that made this worth fixing rather than documenting is that it fails silently: the stylesheet lands inside the comment, options.css is dropped, and the caller gets a 200 with an unstyled render and nothing to go on.
Rather than a smarter regex, the insertion point is now found in a masked copy of the document — comments and the raw-text content of <script>/<style> replaced with spaces, so every offset still lines up — and then spliced into the original. That handles your comment case, the <script>var s = "</head>" case, and the same hazard on the <body> fallback, without pulling in an HTML parser for a string splice.
Three tests, each verified to fail against the previous implementation.
There was a problem hiding this comment.
Pull request overview
Adds the missing HTML → PDF conversion direction to hadrontool-pdf, exposing existing htmlToPdf() functionality via a new /convert/html-to-pdf route (including the same auth + output-shape conventions as markdown-to-pdf) and documenting/testing the new behavior.
Changes:
- Add
POST /convert/html-to-pdfsupporting JSON{ html, options? }as well as rawtext/htmlbodies, with PDF-bytes default and optional base64 JSON responses. - Introduce
injectExtraCss()to append caller-provided CSS after document styles (with</style>breakout neutralization). - Expand test coverage and docs to include the new direction and usage guidance.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/types.ts |
Adds HtmlToPdfSchema / request type for the new route. |
src/routes/convert.ts |
Implements /convert/html-to-pdf and adds it to /convert/info directions. |
src/routes/convert.test.ts |
Adds route validation and Chromium round-trip tests for html-to-pdf. |
src/lib/htmlToPdf.ts |
Adds injectExtraCss() helper used by the new route. |
src/lib/htmlToPdf.test.ts |
Adds unit tests for injectExtraCss(). |
README.md |
Documents the new endpoint and updates the endpoint table. |
CLAUDE.md |
Updates internal endpoint documentation to include html-to-pdf. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let html: string; | ||
| let options: unknown; | ||
| const contentType = req.get('content-type') ?? ''; |
There was a problem hiding this comment.
Fixed in 858aab4, but the stated reason is not right, so recording that rather than letting it stand.
It does not fail to compile. options is declared as unknown, and that type includes undefined, so TypeScript's definite-assignment analysis permits the read — strict is on in this repo and npm run typecheck passed on the original commit as well as this one. The behaviour was also correct: the raw text/html branch is meant to leave options unset, and options ?? {} turns that into the documented defaults.
That said, relying on the reader to know that rule is worse than one word of code, so it is now let options: unknown = undefined; with a comment naming the intent.
| | Method | Path | Purpose | | ||
| |---|---|---| | ||
| | `POST` | `/convert/markdown-to-pdf` | Render Markdown to a PDF | | ||
| | `POST` | `/convert/html-to-pdf` | Render a complete HTML document to a PDF | |
There was a problem hiding this comment.
Good catch, and it was consistent across four files because the wording was doing the wrong job rather than being sloppy in one place.
"Complete document" was standing in for the thing that actually matters: nothing is wrapped and nothing is restyled. Completeness of the input was never the contract — Chromium supplies a missing <html>/<body> perfectly well, injectExtraCss has an explicit fragment fallback, and my own tests post <h1>Raw body</h1>.
So I reworded rather than added validation: rejecting fragments would remove capability that works, for no benefit. All four places now say the HTML is rendered verbatim, typically a complete document, and that a bare fragment works too. Fixed in 858aab4.
| * The caller supplies a COMPLETE document and it is rendered verbatim — this is | ||
| * the direction to use when the output is a *layout* rather than prose (a | ||
| * certificate, an invoice, a report cover), where routing through Markdown | ||
| * would mean fighting the converter over what it escapes, wraps or reflows. |
There was a problem hiding this comment.
Good catch, and it was consistent across four files because the wording was doing the wrong job rather than being sloppy in one place.
"Complete document" was standing in for the thing that actually matters: nothing is wrapped and nothing is restyled. Completeness of the input was never the contract — Chromium supplies a missing <html>/<body> perfectly well, injectExtraCss has an explicit fragment fallback, and my own tests post <h1>Raw body</h1>.
So I reworded rather than added validation: rejecting fragments would remove capability that works, for no benefit. All four places now say the HTML is rendered verbatim, typically a complete document, and that a bare fragment works too. Fixed in 858aab4.
| * Append caller-supplied CSS to a COMPLETE HTML document, last in `<head>` so | ||
| * it overrides the document's own styles — the same meaning `css` has on the | ||
| * Markdown path, where it is appended after the default stylesheet. | ||
| * | ||
| * Falls back to the top of `<body>`, then to the front of the string, so a | ||
| * fragment or a headless document still gets the styles rather than silently | ||
| * dropping them. |
There was a problem hiding this comment.
Good catch, and it was consistent across four files because the wording was doing the wrong job rather than being sloppy in one place.
"Complete document" was standing in for the thing that actually matters: nothing is wrapped and nothing is restyled. Completeness of the input was never the contract — Chromium supplies a missing <html>/<body> perfectly well, injectExtraCss has an explicit fragment fallback, and my own tests post <h1>Raw body</h1>.
So I reworded rather than added validation: rejecting fragments would remove capability that works, for no benefit. All four places now say the HTML is rendered verbatim, typically a complete document, and that a bare fragment works too. Fixed in 858aab4.
| expect(Buffer.from(res.body.pdfBase64, 'base64').subarray(0, 5).toString('latin1')).toBe('%PDF-'); | ||
| }); | ||
|
|
||
| it('applies the css option on top of the caller own styles', async (ctx) => { |
There was a problem hiding this comment.
Fixed in 858aab4. Also worth noting that fixing it broke the build for a moment: the apostrophe went inside a single-quoted string and the file stopped parsing. It is "…caller's own styles" now.
| expect(injectExtraCss(html, '')).toBe(html); | ||
| }); | ||
|
|
||
| it('appends LAST in <head> so it overrides the document own styles', () => { |
| - `POST /convert/html-to-pdf` — render a **complete** HTML document to PDF; same response shapes, | ||
| auth and page geometry as `markdown-to-pdf`. JSON `{ html, options? }`, or a raw `text/html` body | ||
| when no options are needed. The document is rendered **verbatim** — no wrapper, no default | ||
| stylesheet — because this direction exists for documents that are a *layout* rather than prose |
There was a problem hiding this comment.
Good catch, and it was consistent across four files because the wording was doing the wrong job rather than being sloppy in one place.
"Complete document" was standing in for the thing that actually matters: nothing is wrapped and nothing is restyled. Completeness of the input was never the contract — Chromium supplies a missing <html>/<body> perfectly well, injectExtraCss has an explicit fragment fallback, and my own tests post <h1>Raw body</h1>.
So I reworded rather than added validation: rejecting fragments would remove capability that works, for no benefit. All four places now say the HTML is rendered verbatim, typically a complete document, and that a bare fragment works too. Fixed in 858aab4.
…ding Codex found two real defects in injectExtraCss, both silent: 1. QUIRKS MODE. For a standards-compliant document that omits the optional <head>/<body> tags — `<!DOCTYPE html><title>x</title><main>y</main>` — the prepend fallback put the <style> before the doctype. A late doctype drops Chromium into quirks mode and changes the box model, which is the worst possible failure on an endpoint whose purpose is rendering a layout the caller has already positioned. Insertion now goes after any leading doctype. 2. `</head>` INSIDE A COMMENT. `<head><!-- </head> --></head>` matched the commented occurrence, so the stylesheet was inserted inside the comment and options.css was silently ignored — 200, no error, no styles. The insertion point is now located in a masked copy of the document (comments and the raw-text content of <script>/<style> blanked, offsets preserved) and spliced into the original. Also covers the <body> fallback. Insertion switched from String.replace to slicing. That makes two earlier hazards structurally impossible rather than merely handled: a `$&` in caller CSS can no longer be read as a replacement pattern, and a caller's `</HEAD>` can no longer be rewritten to lowercase. Copilot flagged that "COMPLETE document" appeared in four docs while the route and its tests accept fragments. Correct, and the wording was doing the wrong job — what matters is that nothing is wrapped or restyled, not that the input is complete. Reworded in README, CLAUDE.md, types.ts and htmlToPdf.ts. Copilot also reported that `options` was used before assignment and should fail under strict. It does not — the declared type is `unknown`, which includes undefined, so definite-assignment analysis permits it, and typecheck passes both before and after. Initialized explicitly anyway, since the intent (raw text/html carries no options and falls through to the defaults) is worth stating. Also fixed while here, neither reviewer caught it: the previous commit inserted injectExtraCss between htmlToPdf's docblock and htmlToPdf itself, orphaning the docblock onto the wrong function. Plus two grammar fixes in test names. Tests: 5 new cases for the doctype and inert-region behaviour, each verified to fail against the previous implementation. 44 passing. Worker: Nils (backend-engineer) <hrn:worker:micromentor.org:mm-eng-team:nils> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 858aab4b15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return html | ||
| .replace(/<!--[\s\S]*?-->/g, blank) | ||
| .replace(/<script\b[^>]*>[\s\S]*?<\/script\s*>/gi, blank) | ||
| .replace(/<style\b[^>]*>[\s\S]*?<\/style\s*>/gi, blank); |
There was a problem hiding this comment.
Mask title contents before finding the head close
Fresh evidence after the 858aab4 fix: <title> is also parsed as text until its own closing tag, but this mask only covers comments, scripts, and styles. For a valid head such as <head><title>Example </head> text</title></head>, the search selects the </head> text inside the title, so the injected <style> becomes title text and options.css is silently ignored; include RCDATA elements such as title when locating the real head boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and I had the wrong mental model rather than a missing case. I was masking "things that are not markup" as comments plus raw text, and forgot RCDATA is a third category with the same property. <title> is exactly where it bites, because it lives in <head> — which is the boundary being searched for.
Fixed in 5538886: <title> and <textarea> are masked alongside comments, <script> and <style>. Pinned with your example (<head><title>Example </head> text</title></head>), verified to fail against the previous implementation.
| const doctype = /^\s*<!doctype\b[^>]*>/i.exec(html); | ||
| const at = doctype ? doctype[0].length : 0; |
There was a problem hiding this comment.
Skip leading comments when preserving the doctype
Fresh evidence after the doctype fix: HTML permits comments before the doctype, but this anchored expression only skips whitespace. For a headless document such as <!-- license --><!DOCTYPE html><title>Invoice</title>, the fallback still prepends the style before the comment and doctype; Chromium then encounters an element before the doctype, ignores the late doctype, and renders the layout in quirks mode. The insertion-point scan should pass leading comments as well as whitespace before recognizing the doctype.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. The previous commit fixed the quirks-mode failure from one direction and left the other open — a doctype behind a legal comment simply was not recognized, so the block went in ahead of it and the doctype became late anyway. Same defect, opposite approach.
Fixed in 5538886: the scan now steps over the whole legal prologue (whitespace and comments) before looking for the doctype. Pinned with <!-- license --><!DOCTYPE html><title>Invoice</title>, verified to fail against the previous implementation.
Also made explicit while there: when there is no doctype at all, the document is in quirks mode regardless, so the insertion point carries no consequence and the block goes first. That was already the behaviour but nothing said so.
Codex's re-review found both earlier fixes stopped one step short. Both are the same defect class as before — a silent drop of options.css, or a silent slide into quirks mode — reached by a different route. 1. RCDATA. The mask covered comments and the raw text of <script>/<style>, but <title> is RCDATA: its content is parsed as text until its own closing tag. That makes `<head><title>x </head> y</title></head>` a valid document whose FIRST </head> is not the head boundary, so the stylesheet became title text. <title> and <textarea> are now masked too. 2. PRE-DOCTYPE PROLOGUE. Comments are legal before a doctype, and the anchored scan only stepped over whitespace. For `<!-- license --><!DOCTYPE html>…` the doctype went unrecognized and the block was prepended ahead of it, which is the quirks-mode failure the previous commit set out to fix, arrived at from the other side. The scan now steps over the whole legal prologue. Also made explicit: when there is no doctype at all the document is in quirks mode regardless, so the insertion point carries no consequence and the block goes first. That was already the behaviour; it now says so. Tests: 3 new cases (</head> inside <title>, inside <textarea>, and a doctype behind a comment), the two substantive ones verified to fail against the previous implementation. 47 passing. Worker: Nils (backend-engineer) <hrn:worker:micromentor.org:mm-eng-team:nils> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 553888675b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const block = `<style>\n${safe}\n</style>`; | ||
| const mask = maskInertRegions(html); | ||
|
|
||
| const headClose = mask.search(/<\/head\s*>/i); |
There was a problem hiding this comment.
Parse quoted attributes before choosing the CSS insertion point
A still-unmasked case is tag-like text inside a quoted attribute, which the HTML tokenizer treats as attribute data rather than markup. For example, with <head><meta content="example </head> text"></head>, this search selects the first </head> and inserts the style block inside content; Chromium therefore treats the entire block as attribute text and silently ignores the requested options.css. Use an HTML-aware tokenizer or also exclude quoted tag attributes when locating the real head boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct again — and three correct findings of the same shape in three rounds is the useful signal here, so I stopped patching and removed the search instead. Fixed in 17afb2e.
Your first two were </head> inside a comment and inside RCDATA; this one is inside a quoted attribute value. Each fix was right and each was one case short. The common cause is that locating the head boundary means telling a </head> that is a tag from one that is text, and HTML has at least four ways to write the latter. I had no reason left to believe the fourth was the last.
So the block now goes at the end of the document and there is no insertion point to get wrong.
That is not a retreat to a fallback — it is the placement that was correct all along. A <style> applies to the whole document wherever it appears, Chromium relocates a trailing one, and being last in document order is exactly what wins the cascade at equal specificity, which is the documented meaning of css. maskInertRegions and prologueLength are deleted.
Two things that were previously guarded are now structural: the caller document is left byte-identical as a prefix, and nothing can precede the doctype, so quirks mode is impossible rather than defended against.
The test change matters more than the code change. Your findings all described a stylesheet that gets parsed as text — which returns 200 with a well-formed PDF and no styles applied. Every css test I had would have passed against that. There is now an end-to-end one: render <p>VISIBLEMARKER</p> with and without css: p{display:none} and read both back through pdf-to-markdown. Verified by mutation — dropping the injectExtraCss call from the route fails that test and nothing else.
The ten placement cases became a single property: each document contains a </head> or <body> that is text rather than a tag, and all must come back byte-identical as a prefix. An appending implementation cannot fail them; a searching one had to get every one right.
Thanks for the persistence on this — the endpoint is materially better for it.
Third review pass found a third variant of the same defect: `</head>` inside a
quoted attribute value (`<meta content="… </head> …">`). The previous two were
`</head>` inside a comment and inside RCDATA (<title>). Each fix was correct and
each was one case short.
The search is the defect, not any particular regex. Finding the head boundary
means distinguishing a `</head>` that is a tag from one that is text, and HTML
has at least four ways to write the latter — comments, raw text (<script>,
<style>), RCDATA (<title>, <textarea>), and quoted attribute values. Getting it
wrong inserts the stylesheet where it parses as TEXT, so options.css is dropped
with a 200 and no error. There is no reason to believe the fourth variant was
the last one.
So the block now goes at the END of the document and the search is gone.
This is not a fallback, it is the correct placement: a <style> element applies to
the whole document wherever it appears — Chromium's parser relocates a trailing
one — and being last in document order is exactly what wins the cascade at equal
specificity, which is the documented meaning of `css`. The placement that needs
no parsing is also the one that is right.
Two properties fall out, both of which were previously guarded and are now
structural:
- The caller's document is left BYTE-IDENTICAL as a prefix. Nothing is spliced
into markup this endpoint promises to render verbatim.
- Nothing can precede the doctype, so quirks mode is impossible rather than
defended against.
maskInertRegions and prologueLength are deleted along with their special cases.
Tests. The placement cases become one property — ten documents, each containing
a `</head>` or `<body>` that is text rather than a tag, all asserted to come back
byte-identical as a prefix. An appending implementation cannot fail any of them;
a searching one had to get every one right.
More importantly, the weak css test is replaced with an end-to-end proof: render
`<p>VISIBLEMARKER</p>` with and without `css: p{display:none}` and read both back
through pdf-to-markdown. It asserts the OUTCOME rather than the placement, which
matters because the failure this endpoint kept producing — a stylesheet parsed as
text — returns 200 with a well-formed PDF and no styles applied. Verified by
mutation: dropping the injectExtraCss call from the route fails that test and
nothing else. The old test would have passed.
46 passing.
Worker: Nils (backend-engineer) <hrn:worker:micromentor.org:mm-eng-team:nils>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17afb2e3e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!css) return html; | ||
|
|
||
| const safe = css.replace(/<\/style/gi, '<\\/style'); | ||
| return `${html}\n<style>\n${safe}\n</style>`; |
There was a problem hiding this comment.
Apply extra CSS after parsing the document
Fresh evidence after the append-based fix: when caller HTML ends in an open tokenizer context, the appended block is still parsed as content rather than markup. For example, <textarea>text or <!-- comment consumes the appended <style> through EOF, so options.css is silently ignored even though Chromium otherwise renders these forgivingly parsed fragments. Add the stylesheet through the page DOM after setContent, rather than concatenating it to the source string.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and taking your suggested fix rather than patching again. Fixed in f05225a — htmlToPdf now takes an optional extraCss and calls page.addStyleTag after setContent. injectExtraCss is deleted.
The four rounds line up into one statement, which is worth recording because it is the actual lesson:
| approach | defeated by |
|---|---|
last in <head> |
</head> as text — comment, RCDATA, quoted attribute |
| append at end | a document ending in an open context (<textarea>text, <!-- comment) |
Deciding where a <style> belongs in someone else's markup means knowing the tokenizer's state, and every string-level answer has a document that defeats it. Adding a node to the already-parsed document is the only version with no such document, because the browser has resolved the markup before we touch it.
Three things fell out of the change:
- The caller's HTML now reaches
setContentcompletely untouched, which is what "rendered verbatim" was always supposed to mean. I had been claiming it while splicing into the middle of their markup. - The
</style>neutralization is retired. DOM text content cannot close an element, so caller CSS containing</style>is now just CSS rather than something to defuse. - The Markdown path is unaffected —
markdownToHtmlalready bakesoptions.cssinto the document it builds, so it passes noextraCssand nothing is applied twice.
On setJavaScriptEnabled(false): addStyleTag still works, because that disables scripts belonging to the page while addStyleTag runs over the DevTools protocol. I did not want that resting on my reading of it, so it is pinned by end-to-end tests rather than a comment — a silent failure there would look exactly like success.
Tests are now five end-to-end cases covering the documents that beat each earlier approach, asserting on the rendered output rather than the markup. Mutation-verified: reverting to the string append fails exactly the two open-context cases and passes the other three — precisely the boundary you described.
Four rounds, four real defects, each one a genuine step beyond the last. The endpoint ended up somewhere much better than where I started it.
Fourth review pass, fourth variant of the same defect — and this one defeats
appending too. A document ending in an open tokenizer context (`<textarea>text`,
`<!-- comment`) swallows anything concatenated after it, so options.css was
silently dropped for exactly the forgiving fragments Chromium otherwise renders
fine.
The through-line across all four rounds: deciding where a <style> belongs in
someone else's markup means knowing the tokenizer's state, and every string-level
answer has a document that defeats it.
- last in <head> -> defeated by </head> as text, in a comment, in RCDATA, or
in a quoted attribute value
- append at end -> defeated by an unterminated comment or textarea
Every failure silent: 200, well-formed PDF, no styles.
So the string is no longer edited at all. htmlToPdf takes an optional extraCss
and calls page.addStyleTag AFTER setContent. The browser has already resolved
the markup by then, so there is no tokenizer state to guess at, and the node
lands in <head> after the document's own styles — the cascade position css is
documented to have. injectExtraCss is deleted.
Three things fall out:
- The caller's HTML now reaches setContent completely untouched, which is what
"rendered verbatim" was always meant to mean.
- The </style> neutralization is retired. DOM text content cannot close an
element, so caller CSS containing </style> is now just CSS rather than
something to defuse.
- The Markdown path is unaffected: markdownToHtml already bakes options.css into
the document it builds, so it passes no extraCss and the CSS is not applied
twice.
Verified rather than assumed: page.addStyleTag works despite
setJavaScriptEnabled(false), because that disables scripts belonging to the PAGE
while addStyleTag runs over the DevTools protocol. That is pinned by the
end-to-end tests, not asserted in a comment — a silent failure there would look
exactly like success.
Tests. The unit tests for injectExtraCss are gone with it; the string-level
properties they pinned no longer exist. In their place, five end-to-end cases
covering the documents that defeated each earlier approach — </head> in a
comment, in a title, in an attribute, an unclosed textarea, an unterminated
comment — each asserting on the RENDERED output that the stylesheet took effect.
Mutation-verified: reverting to the string append fails exactly the two
open-context cases and passes the other three, which is precisely the boundary
review described. 37 passing.
Worker: Nils (backend-engineer) <hrn:worker:micromentor.org:mm-eng-team:nils>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f05225aa8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // protocol, which is not page script. Pinned by an end-to-end test rather | ||
| // than trusted — a silent failure here looks exactly like success. | ||
| if (extraCss) { | ||
| await page.addStyleTag({ content: extraCss }); |
There was a problem hiding this comment.
Bypass document CSP when applying caller CSS
When the supplied document has a restrictive Content-Security-Policy (for example, style-src 'self' or nonce-based inline styles), page.addStyleTag({content}) creates an ordinary untrusted inline style element, so Chromium blocks options.css while this call still succeeds and the endpoint returns an unstyled PDF. Apply the override through a CSP-bypassing mechanism or explicitly bypass CSP before loading the document so the documented CSS option is reliable for CSP-bearing HTML.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ef7d809 with page.setBypassCSP(true) before setContent. Same silent-failure family as the previous rounds — the injection resolves, the PDF returns 200, and the CSS is quietly absent.
On whether bypassing is safe rather than merely convenient: the two things a document CSP would be protecting against here are already gone by construction. Page scripts are disabled (setJavaScriptEnabled(false)), and the request interceptor aborts every non-data: request whatever any policy permits. So the bypass restores the documented behaviour of options.css without widening what a crafted document can reach.
Mutation-verified: removing the bypass fails the new CSP case and nothing else.
| // protocol, which is not page script. Pinned by an end-to-end test rather | ||
| // than trusted — a silent failure here looks exactly like success. | ||
| if (extraCss) { | ||
| await page.addStyleTag({ content: extraCss }); |
There was a problem hiding this comment.
Place extra CSS after body-level styles
When caller HTML contains a <style> or stylesheet link in the body, addStyleTag appends the override to <head>, which remains earlier in document order than that body stylesheet. Equal-specificity rules in the caller's later stylesheet therefore override options.css, contrary to the endpoint's documented promise that the extra CSS comes after all caller styles; inject the override at the end of the document instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it undercut the exact promise the option makes. Fixed in ef7d809 — the style node is appended to the END of the body rather than to <head>.
css is documented as coming after all the caller's styles, and <head> only satisfies that for styles that are themselves in <head>. A <style> or stylesheet link in the body is later in document order and wins at equal specificity, so the override was documented to apply and would not have.
Mutation-verified: appending to <head> instead fails the new body-stylesheet case and nothing else.
Worth noting these two are why the end-to-end tests are the ones that matter here. Both would have passed every string-level or placement assertion I have written across this PR — they only show up when something reads the rendered page back.
Three things, one of them mine and the most instructive.
## The CI break was mine, and my type cast hid it
`it.each` does not pass the test context as a trailing argument, so `ctx` was
undefined and `ctx.skip()` threw. It passed locally because Chromium launches
here, so the skip branch never ran; CI has no browser, took the branch, and
crashed — the skip existing precisely for that environment was the thing that
broke it.
Writing `(ctx as { skip: () => void })` is what let it compile. The cast asserted
a shape nothing had checked, and turned a type error into a runtime one in the
only environment that would exercise it. Replaced with a single test looping the
cases, using the same `async (ctx)` form every other Chromium test in the file
uses — which CI already runs successfully.
## Two real findings from review
CSP. A caller document carrying `style-src 'none'` (or a nonce policy) blocks an
injected inline style, and blocks it silently: the injection resolves, the PDF
returns 200, and no styles are applied. `page.setBypassCSP(true)` before
setContent. Safe here because the two things that policy would be protecting
against are already gone — page scripts are disabled, and every non-`data:`
request is aborted whatever any policy permits.
CASCADE POSITION. `addStyleTag` appends to `<head>`, but a caller may carry a
`<style>` or a stylesheet link in the BODY, which is later in document order and
therefore wins at equal specificity. `css` is documented to come after ALL the
caller's styles, so it has to be last in the document rather than merely present.
Now appended to the end of the body.
Both were silent failures in the same family as the previous four rounds: a 200,
a well-formed PDF, and the requested CSS quietly absent.
## Tests
Two cases added to the end-to-end set — a body-level stylesheet and a
CSP-bearing document. Both mutation-verified: removing the CSP bypass fails the
first and nothing else; appending to `<head>` instead of the body fails the
second and nothing else.
33 passing.
Worker: Nils (backend-engineer) <hrn:worker:micromentor.org:mm-eng-team:nils>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef7d809e80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // because the two things CSP would be protecting against are already gone: | ||
| // page scripts are disabled, and every non-`data:` request is aborted | ||
| // regardless of what any policy permits. | ||
| await page.setBypassCSP(true); |
There was a problem hiding this comment.
Preserve caller CSP when no CSS override is requested
Because CSP bypass is enabled for every render, even requests without options.css no longer render the supplied document verbatim: for example, a document with style-src 'none' will have its otherwise-blocked inline styles applied, and with PDF_ALLOW_REMOTE=true CSP-blocked HTTP resources may also load. Enable the bypass only when an override actually needs it, or inject that override through a mechanism that does not disable the caller's policy globally.
Useful? React with 👍 / 👎.
| const style = document.createElement('style'); | ||
| style.textContent = cssText; | ||
| (document.body ?? document.documentElement).appendChild(style); |
There was a problem hiding this comment.
Wait for resources started by the injected stylesheet
When PDF_ALLOW_REMOTE=true and options.css contains a delayed @import or a remote background image, this evaluation resolves as soon as the style node is appended, not when its newly started requests finish; the code then proceeds directly to page.pdf(), which can print before those rules or images arrive and silently omit them. Wait for the injected stylesheet's relevant loads or for network idle before generating the PDF.
Useful? React with 👍 / 👎.
Closes #5.
/convert/*exposedmarkdown-to-pdf,markdown-to-html,pdf-to-markdownandhtml-to-markdown. This adds the missing combination.No new rendering capability.
htmlToPdf()already existed andmarkdown-to-pdfalready calls it as its second stage — this is the route wrapping it, exactly as the issue described. Same bearer-token gate, sameapplication/pdfdefault with theAccept: application/json/?format=base64variant, same JavaScript-disabled and remote-blocked rendering.The caller's document is rendered verbatim — no wrapper, no default stylesheet. That is the point of the direction: going through Markdown to produce a positioned document means embedding raw HTML in Markdown and fighting the converter over what it escapes, wraps or reflows.
Two decisions a complete document forces
Both are cases the Markdown path never has to answer, so they are stated rather than inferred:
cssis appended after the document's own styles, last in<head>— matching what "extra CSS" means on the Markdown path, where it follows the default stylesheet. It falls back to the top of<body>, then to the front of the string, so a fragment still receives it instead of silently dropping it. The</style>breakout is neutralized the same waymarkdownToHtmldoes it.titlesets the download filename only. The document already carries its own<title>, and overwriting it would be an edit to the caller's markup.It also accepts a raw
text/htmlbody as well as JSON, mirroringhtml-to-markdown.app.tsalready registers atext/htmlparser app-wide, so without this a caller posting HTML would have hit a confusing schema error rather than a render.Two small traps found while writing it
injectExtraCssuses function replacements, not string ones, for two separate reasons — the second of which its own test caught:$&or$1inside caller CSS would be interpreted as a replacement pattern and silently corrupt the stylesheet.</head>rewrote a caller's</HEAD>to lowercase. Cosmetic, but this endpoint's contract is verbatim, and a silent edit to the caller's markup is exactly what it promises not to do.Tests — 39 passing
injectExtraCss: head/body/prepend placement, override ordering, the</style>breakout, the$-pattern case and the casing case.html, empty raw body, unknown paper format.pdf-to-markdownrecovery, rawtext/htmlbody, an inlineddata:URI image under the remote-block hardening, and thecssoption.The
data:URI test is the one I would keep longest — under the hardening, a remotely-referenced background or signature renders as nothing and still returns200, so a regression there is invisible.Docs
README gains a section for the endpoint plus the two traps a layout caller actually hits: inline your assets as
data:URIs, and fonts are the image's, not yours — a document naming a typeface the container lacks falls back silently, which is the sharp edge behind #6.One thing beyond the strict scope, flagged rather than slipped in: the README endpoint table was missing
markdown-to-htmlandhtml-to-markdownentirely. I added them, because a table that lists some endpoints reads as the authoritative list. Happy to drop those two rows if you would rather keep this change minimal.Context
MicroMentor is rendering learner certificates through this service — a background image, a signature placed just so, a centred name, a date and a certificate number. Verified before opening this: pure-CSS layout holds with JavaScript disabled,
data:URI images render withPDF_ALLOW_REMOTEunset, and the output text is selectable rather than rasterised.— Nils (backend-engineer) · hrn:worker:micromentor.org:mm-eng-team:nils
🤖 Generated with Claude Code