fix(pdf-cover): render page via go-pdfium WASM (bookshelf-uamr) #971
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-uamr"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
pdfcpuembedded-image extraction withgo-pdfium(WASM/wazero backend) page rendering for PDF cover extractionErrNoCoverand multi-image PDFs grabbed a logo instead of the coverpdfium.wasmis embedded viago:embedand compiled once at startup viasync.OnceErrNoCover(PERMANENT sentinel);wfengine.isPermanentCoverErralready covers thisWithMemoryLimitPages(16384)), read-only host FS (WithReadOnlyDirMount), render deadline with interruptible execution (WithCloseOnContextDone+ 60s timeout goroutine)Security hardening (3 MAJORs fixed)
WithMemoryLimitPages(pdfiumMaxMemoryPages)wherepdfiumMaxMemoryPages = 16384(~1 GB). A crafted PDF with heavily compressed streams can decompress to GBs in WASM linear memory; this caps growth.FSConfig: wazero.NewFSConfig().WithReadOnlyDirMount("/", "/"). Previously go-pdfium defaulted to read-write host-root mount; pdfium page rendering needs only read access.WithCloseOnContextDone(true)+context.WithTimeout(60s)goroutine inrenderPDFCover. A crafted PDF that makes pdfium spin held the MaxTotal=1 pool instance forever; now the instance is released on timeout and subsequent renders succeed.Test plan
ErrNoCover(permanent)/Count 0) returnsErrNoCoverExtractCoverFromFilefailingReadAtreturns errorErrCoverTooLarge)ErrNoCover, pool released (closeInstance called)make testgreen,make coveragegate passes (100%),make lint0 issuescheck-coverage.shexclusions — all branches covered by real testsCloses bead bookshelf-uamr on merge.
CODE REVIEW: APPROVED — bookshelf-uamr
Phase 0: DEMO Verification
No runnable DEMO block exists in the bead or PR body. This is a background-worker change (PDF cover extraction) with no interactive user surface to capture in a command DEMO. CI is green and the test plan is represented by the Ginkgo suite. The orchestrator has pre-cleared this; review proceeds to Phase 1.
Note: the PR body contains a stale statement — "4 infrastructure/dead-code paths excluded in
check-coverage.shwith justification." The actual branch has zero net changes toscripts/check-coverage.sh(confirmed by empty diff). The seam-based tests replaced those exclusions. The PR body was not updated after the revision. No code impact — pure documentation drift.Phase 1: Spec Compliance
All five review criteria verified against the diff:
Page-0 render correctness (
cover_extract.go:483-484): rendersIndex: 0bounded byWidth: maxPDFRenderWidth(1200),Height: maxPDFRenderHeight(1800). go-pdfium documents these as "The maximum width/height" — aspect ratio is preserved, so a wide page caps at 1200 px and a tall page caps at 1800 px. Both constants are documented with rationale. The existingmaxPDFBytes(200 MB) size guard fires before pdfium is invoked. ✓Permanent error classification (
cover_extract.go:469-496): corrupt/password-protected PDF →errors.Join(err, ErrNoCover)(permanent sentinel); zero-page PDF → bareErrNoCover; render failure →errors.Join(err, ErrNoCover); PNG encode failure →errors.Join(encErr, ErrNoCover). All four path-end errors wrapErrNoCover, which is already inwfengine.isPermanentCoverErratcover_workflow.go:112. Pool-init and GetInstance failures do not wrap ErrNoCover — correctly treated as transient infrastructure failures. Nogo-workflowsimports anywhere ininternal/files/. ✓pdfRenderSeams DI (
cover_extract.go:62-74): package-level struct; production defaults are bound to real pdfium/PNG functions; seams overridden only viacover_extract_export_test.go(sanctionedpackage filesexport file). All 8 error-branchItblocks make real assertions — pool failure assertserroccurred andresultis nil; instance failure same; render failure assertserrors.Is(err, files.ErrNoCover)and nil bytes; PNG encode failure same. No assert-nothing tests. ✓Pool init is sync.Once, concurrency-safe (
cover_extract.go:53-54, 96):pdfiumPoolOnce sync.Onceguardswebassembly.Initexactly once.MaxTotal: 1serialises concurrent callers viaGetInstance(pdfiumInstanceTimeout)(30 s timeout). Safe for the cover-generate worker. ✓check-coverage.sh: zero new exclusions — confirmed by empty diff. 100% coverage is achieved via the injectable seams. No new
golangci.ymlexclusions. ✓Phase 2: Code Quality
No blockers. No majors. Three minors.
[MINOR]
internal/files/cover_extract.go—extractPDFCover(line ~448) is a one-line passthrough that just callsrenderPDFCover. It adds an indirection layer with no caller distinction. Could be collapsed, but it's harmless.[MINOR]
internal/files/cover_extract.go:99— Comment ongetPdfiumPoolsays "Pool init failure is a transient condition...that may be retried." Thesync.Oncecaches the failure for the process lifetime, so within a single process the failure is effectively permanent (wfengine burns MaxAttempts on fast-returning cached errors before giving up). The comment is accurate only at the process-restart level. No correctness bug — eventual failure is the correct outcome — but the wording could mislead a reader into expecting in-process retry to succeed.[MINOR]
internal/wfengine/cover_workflow.go:96(on main, not this diff) — The comment enumeratingErrCorruptArchiveuse-cases still lists "corrupt PDF extraction." Corrupt PDFs now returnErrNoCover, notErrCorruptArchive. Both sentinels remain inisPermanentCoverErrso the behavior is correct; only the comment is stale. Worth a follow-up to keep the comment accurate.REVIEW VERDICT: 0 blocker, 0 major, 3 minor
Security Review — bookshelf-uamr (PR #971)
Reviewed:
git diff origin/main...origin/bd-bookshelf-uamrFocus areas: decompression-bomb/OOM, WASM sandbox bounds, permanent-error classification, supply chain, path traversal.
[MAJOR] internal/files/cover_extract.go:218-227 — pdfium.wasm declares no max memory; no wazero cap configured
The embedded
pdfium.wasmbinary'smemorysection specifies only a minimum (0 pages) with no maximum — confirmed by inspecting the WASM binary. wazero allows unbounded growth up to the 32-bit address limit (~4 GB). Thewebassembly.Initcall passes noRuntimeConfig, so the defaultwazero.NewRuntimeConfig()is used with no memory ceiling. The 200 MBmaxPDFBytesinput guard caps the compressed PDF bytes handed to pdfium, but pdfium internally decompresses stream objects (flate/LZW/JBIG2/etc.) before rasterising. A crafted 200 MB PDF with deeply compressed streams can expand to gigabytes of WASM linear memory on the host, OOMing the worker process. Fix: passRuntimeConfig: wazero.NewRuntimeConfig().WithMemoryLimitPages(N)(wazero supports this via the module-override path), or add a hard note that the worker container must be run under a cgroup memory limit that covers the expected WASM expansion headroom. At minimum, the current behaviour must be documented.[MAJOR] internal/files/cover_extract.go:218-227 — Default FSConfig mounts host root filesystem read-write into pdfium WASM module
The PR passes no
FSConfiginwebassembly.Config. go-pdfium'sInitthen defaults towazero.NewFSConfig().WithDirMount("/", "/")on Linux (source: go-pdfium@v1.19.4 webassembly/webassembly.go:94-95).WithDirMountprovides read AND write access via WASI. pdfium only requires read access to system font directories and colour profiles for page rendering — no write access is needed. If a crafted PDF triggered a pdfium path that exercised WASI filesystem write calls (including any future pdfium regression), the WASM module would have write access to the entire host root. Fix: explicitly setFSConfig: wazero.NewFSConfig().WithReadOnlyDirMount("/", "/")(wazero'sWithReadOnlyDirMount), or restrict to font directories only (e.g.,/usr/share/fonts,/etc/fonts).[MAJOR] internal/files/cover_extract.go:350-358 — RenderPageInPixels has no deadline; a hanging PDF permanently blocks the MaxTotal=1 pool
pdfiumInstanceTimeout(30 s) guards only pool acquisition — the time waiting for a free instance. TheRenderPageInPixelscall itself has no context or deadline. A crafted PDF that causes pdfium to spin or block inside its WASM execution will hold the single pool instance (MaxTotal: 1) indefinitely, preventing all subsequent cover extractions. go-pdfium documents a mechanism to interrupt in-flight WASM execution: passingRuntimeConfig: wazero.NewRuntimeConfig().WithCloseOnContextDone(true)makesinst.Close()cancel the running WASM goroutine via context cancellation (go-pdfium@v1.19.4 webassembly/webassembly.go:379-381). Without this flag,inst.Close()is a no-op on a running WASM execution. Fix: addRuntimeConfig: wazero.NewRuntimeConfig().WithCloseOnContextDone(true)and wrap the render call with a context that has a finite deadline (e.g. 60 s), so a hanging render can be interrupted and the pool instance returned.[MINOR] internal/files/cover_extract_export_test.go:1 — Declares
package files(white-box) and references unexportedpdfRenderSeamsThe file follows the established
export_test.goconvention already present in the package (export_test.goalso declarespackage filesand exposes unexported symbols). Per project convention every*_test.goshould bepackage files_test; export-bridge files are a controlled exception but still technically white-box. The seams are used by external (package files_test) tests that exercise the publicExtractCoverAPI — so the test intent is correct. No immediate correctness or security impact.Positive findings (confirmed safe):
maxPDFBytes(200 MB) input cap: retained and still checked before any PDF content is passed to pdfium.maxPDFRenderWidth = 1200,maxPDFRenderHeight = 1800(≈8.6 MB RGBA cap on raster output).OpenDocumentfailure) and zero-page PDF both wrapErrNoCover;isPermanentCoverErrin the wfengine mapsErrNoCovertogowf.NewPermanentError, preventing retry storms on poison PDFs. Pool-exhaustion and pool-init errors do NOT wrapErrNoCover, correctly leaving them retryable.&pdfData) directly to pdfium; no path construction from PDF content.github.com/klippa-app/go-pdfium v1.19.4andgithub.com/jolestar/go-commons-pool/v2 v2.1.2are established, well-known packages; go.sum hashes are present. The embeddedpdfium.wasm(5 MB binary) is the standard klippa-app build — expected and acceptable.REVIEW VERDICT: 0 blocker, 3 major, 1 minor
Security: closes the three [MAJOR] findings from the security review of the go-pdfium WASM setup (internal/files/cover_extract.go): 1. WASM memory cap (pdfiumMaxMemoryPages = 16384 = ~1 GB): passes a bounded RuntimeConfig.WithMemoryLimitPages to webassembly.Init, preventing a crafted PDF from growing WASM linear memory to ~4 GB and OOMing the worker. 2. Read-only FS: passes FSConfig: wazero.NewFSConfig().WithReadOnlyDirMount("/", "/") so pdfium page rendering has no write access to the host filesystem from WASM. 3. Render deadline + interruptible execution: adds RuntimeConfig.WithCloseOnContextDone(true) so inst.Close() interrupts in-flight WASM, and wraps the RenderPageInPixels call in a 60s context.WithTimeout goroutine. On timeout, closeInstance() is called (interrupting WASM), the goroutine is drained, and ErrNoCover is returned — the pool slot is always released, preventing permanent DoS of the MaxTotal=1 pool. Also fixes the sync.Once comment (init failure is cached permanently, not retryable) and adds injectable seams + tests for the deadline path: - SetPdfGetNilInstanceForTest / SetPdfSimpleRenderPage0ForTest / SetPdfSimpleCloseInstanceForTest / SetPdfRenderTimeoutForTest (export_test.go) - "PDF render: deadline exceeded" — verifies timeout error wraps ErrNoCover and closeInstance is called - "PDF render: subsequent render succeeds after deadline" — verifies pool is not permanently exhausted after a timed-out render Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>CODE REVIEW (focused re-review): render-deadline / pool-release concurrency — bookshelf-uamr
Reviewed diff:
origin/main...origin/bd-bookshelf-uamr— files examined:internal/files/cover_extract.go(production concurrency/deadline logic)internal/files/cover_extract_export_test.go(seam setters)internal/files/cover_extract_test.go(deadline tests).golangci.yml/scripts/check-coverage.sh(exclusion audit)Checklist results
1.
cancel()always called — PASScover_extract.go:511:defer cancel()is placed immediately aftercontext.WithTimeout(...). Deferred unconditionally; fires on both success and timeout paths.2. No goroutine leak — PASS
cover_extract.go:517: channel ismake(chan renderResult, 1)— buffered capacity 1. The goroutine can always send without blocking, regardless of which branch theselecttook. On the timeout path,<-chat line 532 explicitly drains the goroutine beforerenderPDFCoverreturns, so no goroutine is orphaned even in the adversarial case.3. Pool instance released on both paths — PASS
cover_extract.go:525):pdfRenderSeams.closeInstance(inst)called before returning.cover_extract.go:531):pdfRenderSeams.closeInstance(inst)called, then<-chdrains the goroutine, then returns. The MaxTotal=1 pool slot is released in both cases via thecloseInstanceseam (which in production callsinst.Close(), releasing the WASM worker back to the pool viaworkerPool.InvalidateObject— enabled by theWithCloseOnContextDone(true)pool config at line 145).4. ErrNoCover returned on timeout — PASS
cover_extract.go:533:return nil, fmt.Errorf("pdf: render: deadline exceeded: %w", ErrNoCover). A hanging PDF is classified as bad-input / PERMANENT — correct.5. Tests: timeout path is deterministic — PASS
SetPdfRenderTimeoutForTest(time.Millisecond)injects a 1 ms deadline; no real 60-second sleep. The goroutine stub blocks on<-unblockRender; thecloseInstancestub closesunblockRender, so the goroutine is guaranteed to unblock. Fully deterministic; no wall-clock flake.6. Tests:
closeInstancecalled assertion — PASScover_extract_test.go"PDF render: deadline exceeded" —closeInstanceCalledflag set inside stub, asserted inIt("calls closeInstance to release the pool slot").7. Tests: subsequent render succeeds after deadline — PASS
"PDF render: subsequent render succeeds after deadline" —
callNatomic counter differentiates first (blocking) from second (immediate-return) invocation. After first timeout,closeOnce.Doextends the timeout to 1 second and closesunblockRender, allowing the goroutine to drain. Second call proceeds with extended timeout and the stub returns immediately. Both assertions (secondResultErris nil,secondResultnon-empty) prove the pool-slot mechanism works end-to-end.8. No
.golangci.ymlorscripts/check-coverage.shexclusions added — PASSZero diff on both files; confirmed by
git diff --name-only.9. Test packages are black-box — PASS
cover_extract_test.goispackage files_test;cover_extract_export_test.goispackage files(sanctioned export seam file — correct use of the pattern).Findings
[MINOR]
internal/files/cover_extract.go:531— second//nolint:errchecklacks a justification commentThe timeout-path
closeInstancenolint on line 531 has no inline rationale, while the success-path nolint on line 525 carries// pool-instance close; errors are non-actionable. Inconsistent; a reviewer has to infer the reason. Add the same comment:// pool-instance close; errors are non-actionable.[MINOR]
internal/files/cover_extract_test.go"PDF render: subsequent render succeeds after deadline"BeforeEach—SetPdfRenderTimeoutForTestreturn value discarded inside stubInside the
closeOnce.Do(func() { files.SetPdfRenderTimeoutForTest(time.Second); ... })block, the returned restore function is silently discarded. The outerDeferCleanupcorrectly handles final cleanup so this is not a bug, but discarding a restore-func without a comment is surprising to readers. A one-line comment (// restore handled by outer DeferCleanup) would make the intent explicit.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
Security Re-Review — PR #971 (bookshelf-uamr)
Focused re-check of the three prior [MAJOR] findings on the go-pdfium/WASM PDF cover render path in
internal/files/cover_extract.go.Prior MAJOR #1 — No WASM memory cap (OOM risk)
Status: RESOLVED
getPdfiumPoolnow passes:pdfiumMaxMemoryPages = 16384caps wazero linear memory to exactly 1 GB. A 1200×1800 RGBA frame is ~8.6 MB so the cap is well above any real render and well below what would OOM a worker. ThemaxPDFBytes = 200 MBinput cap is still present atExtractCoverFromFilebeforeio.ReadAll, providing a second boundary. No concern.Prior MAJOR #2 — Host
/mounted read-write into WASMStatus: RESOLVED
FSConfig: wazero.NewFSConfig().WithReadOnlyDirMount("/", "/")is the only mount call in the file (verified:grep WithReadOnlyDirMount\|WithReadWrite\|WithDirreturns exactly one line — line 138). WASM has read-only access to the host filesystem; no write path from WASM to the host remains.Prior MAJOR #3 — Render has no deadline; hanging PDF permanently exhausts the MaxTotal=1 pool
Status: RESOLVED
renderPDFCovernow:context.WithTimeout(context.Background(), pdfRenderTimeout)(60 s default).ch(capacity 1 — goroutine can always send without blocking).<-ctx.Done()): callscloseInstance(inst)— which, withWithCloseOnContextDone(true), cancels the wazero worker context and interrupts in-flight WASM execution — then drains<-chto prevent goroutine leak.closeInstance(inst)to return the slot to the pool before returning.Pool slot release is verified: no double-close is possible (each select arm calls
closeInstanceexactly once; the goroutine never calls it). The test "subsequent render succeeds after deadline" drives the timeout path and asserts the second render completes, confirming no permanent pool exhaustion.Permanent error classification via
ErrNoCoverwrapping:OpenDocumentfailure): wrapsErrNoCover— PERMANENT.ErrNoCoverdirectly — PERMANENT.ErrNoCover— PERMANENT.ErrNoCover— PERMANENT.ErrNoCover— PERMANENT (correct: adversarial hangers get no retry).ErrNoCover— treated as transient (correct: infrastructure-level, not file-level;isPermanentCoverErrinwfenginewon't match, so go-workflows retries).The
wfengineisPermanentCoverErralready listsErrNoCoverand the domain package does not import the workflow engine — architecture boundary intact.Additional observations (not regressions, not blockers)
[MINOR] internal/files/cover_extract.go:88 —
init()used to assignpdfRenderPage0FnProject convention: "no side-effecting init". This
init()only assigns a function closure (no I/O, no external state), but the convention is stated broadly. The comment explains the necessity (breaking a var→func→var initialization cycle). A non-init alternative (lazy nil-check insiderenderPDFCover) would satisfy the convention. Not a security issue.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
d20053e1eed195b5a702