feat(export): rich branded-PDF engine for export_document (Slice 1) - #889
Merged
Conversation
Direct nh3.clean tight allowlist + BeautifulSoup decompose pre-pass; classes-only (no inline style -> no CSS url() exfil); no <img> (tool inlines its own post-sanitize). 25-case attack corpus + 13 adversarial probes, mutation-verified per layer. Slice 1 / Task 1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
THEME_CSS + component_css(): table variants (grouped/zebra/tree/conditional/rag), callout/kpi/cover/section-divider/signature, CSS .bar (width set inline by tool post-sanitize), print-color-adjust:exact; wkhtmltopdf-safe (no flex/grid). 40 pure tests. Slice 1 / Task 2. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
css_bar/kpi_tile/rag_chip (escaped inputs, width-clamped, fail-loud on unknown status) + fmt_amount/fmt_pct reusing frappe.fmt_money/get_number_format (None/NaN/Inf/-0.0 guarded, negatives->parenthetical .neg). 45 tests + 2 site-gated skips (guard logic stub-tested). Class contract matches theme.py. Slice 1 / Task 4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…furniture.py) render_pdf calls wkhtmltopdf directly via subprocess.run(timeout=) — a REAL hard bound that kills a runaway render (pdfkit.from_string has none; cycle-2 H2) and eliminates pdfkit <meta> option-injection. Unconditional security/fidelity flags (disable-local-file-access/javascript + background/images/print-media-type/disable-smart-shrinking + utf-8/page/margins), mutation-verified. Header/footer/watermark sanitized via sanitize_rich; letterhead = trusted operator config; watermark in header template (per-page); hash-named temp files, finally cleanup. 32 tests + 1 site-gated skip; real render = FC smoke gate. Slice 1 / Task 3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ice 1 orchestrator)
Plain path byte-preserved (extracted verbatim to _render_plain; legacy calls unchanged, no notes key). Rich path (any truthy rich kwarg): md_to_html -> sanitize_rich -> {{chart:N}} css_bar splice (post-sanitize) -> emptiness guard -> component_css wrap -> render_pdf -> save_export_file + notes[]. Caps (_MAX_CHARTS=20, render timeout 25s), format matrix (pdf/html/png), telemetry degraded/ok/no_data/rejected (closes the no-telemetry gap). 24 site-free tests + 15 subtests; pre-existing FrappeTestCase + site-gated tests run in the bench gate. Slice 1 / Task 6.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Back-gate review (10 lanes) found a Critical DoS, an SSRF, and a spread of
correctness/resilience/observability gaps. Fixes:
- sanitizer: collapse the bs4/html5lib decompose pre-pass to nh3's native
clean_content_tags (single Rust pass). Kills a quadratic-on-nesting DoS
(200k nested fragment: ~82s -> ~2.3s, benchmarked), removes the
double-parser mXSS surface, and drops the sanitizer<->export duplication.
Adds noscript/template/xmp/plaintext/textarea/title to the strip set.
- letterhead SSRF: resolve_letterhead now (a) defaults to the site's default
Letter Head when none is named, (b) read-perm-checks it as the impersonated
user, and (c) neutralises images -- remote <img> is dropped (no server-side
fetch) and same-site /files logos are base64-inlined in-process (which also
makes them actually render). Resolution moved to _render_rich so a not-found
name surfaces a note.
- malformed chart rows now degrade to a note instead of an uncaught 500;
a boundary catch-all logs + records telemetry + re-raises clean for any
other unexpected error (closed the no-telemetry blind spot).
- blank-PDF guard checks visible text (not raw markup) so a childless-tag
skeleton is caught; rich HTML output is now a full doctype/charset/title
document (no mojibake); {{chart:N}} splice is tree-aware (text nodes only,
skips pre/code and attributes; single-pass, whitespace-tolerant, 3-digit cap).
- render_pdf: header temp created inside try (no leak on 2nd-write failure),
_write_temp cleans a partial file, broadened except to OSError, %PDF- magic
check, stderr tail on empty output, frappe.log_error on every infra failure,
page_size allowlist + margins 0-100 validation, furniture length cap.
- telemetry.record_export_event gains rich flag + detail (rich vs plain,
reconstructable failures). Chat card carries notes deterministically
(api._maybe_attach_artifact + a ChatView caption) so a degraded document is
never visually identical to a clean one.
Tests: sanitizer corpus now sweeps the full decompose set (mutation-covered);
added malformed-row, blank-skeleton, letterhead inline/perm/note, timeout/
OSError/magic-byte/log_error, tree-aware-splice, and telemetry-rich cases.
Site-gated tests switched from unconditional skip to skipif. 247 pure tests
green; real wkhtmltopdf render remains the FC smoke gate.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round-2 verification reopened F1: the letterhead was folded in without a real
sanitizer pass, so an <img>-only regex missed many fetch-capable constructs
(no-quote src, <image> alias, alt=">" truncation, <link>, <style>url(), style=,
<iframe>, <input type=image>, protocol-relative //host). Real SSRF via an
agent/user-authored (or default) Letter Head.
- Add sanitize_letterhead() (nh3, url_schemes={"data"} + url_relative="deny",
<img> allowed data-only, <a> dropped) as the HARD gate, applied after
_inline_letterhead_images converts same-site logos to permission-checked
base64. Now every non-data image src (any syntax) and link/style/image/svg/
iframe/style= is stripped; only the inlined data: logo survives. Proven
against a bypass corpus (nh3 0.3.6 supports url_relative).
- Logo inlining: reject on File.file_size BEFORE reading; magic-byte image
sniff (don't trust the .png extension).
- Boundary log uses get_traceback() (not with_context) so document content /
base64 logos are not dumped into the operator Error Log.
- Non-zero-exit / empty-output errors keep wkhtmltopdf stderr (which can carry
the temp path) OUT of the user message; it goes to the Error Log only.
- _remove_quietly suppresses OSError (not just FileNotFoundError) so cleanup
can't abort the finally loop or mask the real error.
- Emptiness guard (_has_visible_content) unescapes entities (a -only doc
is caught) and treats a rendered bar-chart as visible (chart-only-empty-labels
no longer false-rejected).
- _splice_charts note accuracy: a chart referenced only in a <code> example (or
an attribute) is no longer mis-reported as "placeholder not found".
Tests: full-set sanitize_letterhead SSRF corpus; the has_permission negative
test now mocks get_doc (was passing for the wrong reason); size/magic/nonimage
inline cases; stderr-in-log-not-message; _remove_quietly OSError; entity/chart
blank edges; note accuracy; a DIRECT record_export_event rich/detail test; and
title/textarea/xmp/plaintext added to the corpus's content-bearing sweep.
280 pure tests + 15 subtests green; ruff clean. Real render stays the FC smoke.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round-3 verification confirmed the letterhead SSRF is closed, with one LOW residual: an inlined data:image/svg+xml logo is an OPAQUE blob nh3 does not look inside, so an SVG carrying <image href>/<script> would ride into the render and rely on QtWebKit's (untested-here) SVG-static mode for containment. Beyond the READ-only threat model (needs write to a File + a Letter Head), but cheap to remove the external-property dependency entirely. Inline RASTER logos only (png/jpeg/gif/webp/bmp): reject image/svg+xml by mime AND drop SVG bytes from the magic-byte sniff, so an SVG can never become a data: URI in the render regardless of file extension. A company logo is almost always raster; an SVG degrades to no-logo, not a risk. Tests: SVG-by-extension refused before read; SVG-bytes-behind-*.png refused by the sniff. 144 furniture+sanitizer tests green; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fix CI) The app CI runs `bench run-tests` (Frappe's unittest-based runner) with NO pytest installed; it IMPORTS every test_*.py, so the pytest-style pure-module tests died at import with `ModuleNotFoundError: No module named 'pytest'` and reds the shard. Zero other app tests use pytest — the convention is unittest/FrappeTestCase. Convert the 4 pure-module files to pure unittest.TestCase (behavior-preserving): parametrize -> subTest loops, pytest.raises -> assertRaises, fixtures/monkeypatch -> setUp + patch.object(...).start() + addCleanup(patch.stopall), skipif -> skipUnless, tmp_path -> tempfile.mkdtemp. No `import pytest` remains (verified via a forced-no-pytest import of each file); all tests live in TestCase classes (no bare module-level test fns that unittest discovery would miss). This also puts the sanitizer/letterhead SSRF corpus in the REAL bench gate, not a parallel runner. No production code changed; no assertion dropped. 159 tests + 151 subtests green (3 site-gated skips); ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mpany
Unrelated to the rich-PDF change, but this PR's new test files shifted the CI
shard split and exposed a latent fragility in test_tier2a_erpnext_reads: its
_ensure_company deliberately skips chart-of-accounts creation, so the company has
no default inventory account, and erpnext then refuses to create the fixture's
Warehouse under perpetual inventory ("Please set Account in Warehouse ... or
Default Inventory Account in Company") — failing setUpClass. It passed on develop
only because of the prior shard ordering.
Set enable_perpetual_inventory = 0 on this COA-less, permission-check-only company
(with a belt-and-suspenders re-assert after insert) so _ensure_warehouse succeeds
deterministically regardless of shard order. The fixture never posts stock, so
this changes nothing about what the tier2a permission tests exercise.
NOTE: not verifiable locally (jarvis.test has erpnext absent) — reasoned from
erpnext's is_perpetual_inventory_enabled gate on warehouse-account creation;
validated on CI.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Slice 1 of the rich-PDF engine.
export_documentbecomes dual-path: the existing plain path is byte-preserved (legacy calls unchanged), and a new opt-in branded rich path renders letterhead / header / footer / watermark + styled tables / KPI / callout / RAG components + CSS bar charts, hard-timed on wkhtmltopdf.New modules under
jarvis/tools/_export/document/:sanitizer.py(nh3 airtight sanitize),theme.py(branded stylesheet),graphics.py(locale number formatting + CSS bar/KPI/RAG builders),furniture.py(directwkhtmltopdfsubprocess with a real hard timeout + letterhead resolution), and the dual-path orchestrator inexport_document.py.Reviewed — full
/review-loop(3 verification rounds)A 10-lane back-gate panel + 2 focused re-verification rounds ran on this diff. It found and this PR fixes a CRITICAL DoS, a letterhead SSRF, and a spread of correctness / resilience / observability gaps:
clean_content_tags(single Rust pass, ~2.3s worst case — 36×), which also removed a double-parser mXSS surface and a code duplication.<img>-only regex missed<link>/<style>url()/<image>/<iframe>/no-quote///host/etc. → server-side fetch. Addedsanitize_letterhead()= nh3 withurl_schemes={"data"}+url_relative="deny"(data-only images,<a>dropped) after inlining same-site logos to permission-checked base64. Verified against a 55-payload bypass corpus. SVG logos are refused (raster only — an inlined SVG is an opaque active-content blob).{{chart:N}}splice is tree-aware (skipspre/code+ attributes, no double-substitution); header temp-leak, exec-OSError,%PDF-magic check,log_erroron infra failures, page-geometry validation, stderr-out-of-user-message; telemetry gainsrich/detail; degrade notes surface deterministically on the chat card (not model prose).Tests
282 pure unit tests + 15 subtests green (3 site-gated skips), ruff clean. The
sanitize_rich/sanitize_letterheadsecurity cores are adversarially probed and the decompose set is mutation-covered; the plain path has a byte-shape regression test. The real wkhtmltopdf render is mocked (no binary in CI) — see the smoke gate below.Rollout
Deploy order (one-way): app → plugin (#75) → persona (#195). App-first is required (
registry.dispatchfilters unknown kwargs, so a plugin-first deploy silently ships plain PDFs). Old-app+new-plugin and new-app+old-plugin both coexist safely. Zero new pip deps (nh3/beautifulsoup4/pdfkit are already frappe-pinned).bench --site <test-site> run-tests --app jarvis --module jarvis.tests.test_export_document(+ thetest_export_document_*siblings) — runs theFrappeTestCase+ site-gated tests too.frappe-claw-test, callexport_document(theme=True, letterhead=…, header=…, charts=…)and eyeball the PDF (+ confirm the notes caption renders — the SPA needs a rebuild).Scope / deferred
Slice 1 = branded PDFs via letterhead + CSS + CSS bar charts. Slice 2 (deferred, behind a matplotlib-on-FC spike) = image charts, QR, embedded raster logos beyond letterhead. Surfaced by the Approach lane (not blocking): the plain path still rolls its own
save_file(a known deferred migration in_export/envelope.py) and retains a latent</b>-title slash crash + the pre-existing plain-path html5lib DoS — both untouched here to keep the byte-preserved contract.DO NOT MERGE — navin merges (after the gates above, in app → plugin → persona order).
🤖 Generated with Claude Code