You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Docbank now has a verified local PyMuPDF PDF-text provider without giving an ambient Python interpreter or filesystem surface to the rendition path. It sends the exact authorized PDF bytes over stdin to one operator-pinned executable and accepts only the fixed versioned stdout contract.
Docbank proves the PDF page count locally before publishing complete page evidence. Missing pages, partial output, runtime drift, oversized output, cancellation, expired authority, and executable replacement fail closed. Child stderr never crosses the public error boundary.
Why
Local PDF extraction should avoid network disclosure without becoming a privileged path that can reopen arbitrary files, discover runtimes, or publish page claims Docbank cannot verify.
Usage
For library use, construct the provider with pymupdf.New(profile). The profile pins the exact bridge executable, its SHA-256, immutable runtime identity, timeout, and byte/page limits; Docbank does not install Python or pass a source pathname.
This PR does not add daemon, API, or CLI selection. That application wiring is intentionally deferred to S1–S3.
High-severity XML parsing flaws can crash the daemon or exhaust memory; two medium-severity reliability issues can cause endless retries or misclassified timeouts.
High
internal/processing/source_metadata.go:335 — XML elements store strings.Builder by value in a growing slice. Appending a child after character data initializes its parent can copy a non-zero builder, causing the next write to panic. Common formatted XMP or OOXML can therefore crash the daemon. Store builders behind pointers or use copy-safe storage, and test nested, formatted XML.
internal/processing/source_metadata.go:337 — Every character-data token is copied into every open ancestor without nesting or cumulative-memory limits. Deeply nested input can amplify a bounded document into excessive memory use and terminate the daemon. Limit nesting and total buffered text—ideally collecting only recognized metadata elements—and warn when limits are exceeded.
Medium
internal/processing/source_metadata.go:174 — metadataCollector.strings limits entry count but not individual entries to the 64 KiB schema maximum. Oversized extracted values can later fail MarshalSourceMetadataV1, preventing metadata publication and causing indefinite retries. Validate entries against MaxSourceMetadataValueBytes and omit oversized values with a warning.
document/bridge/client.go:124, document/bridge/client.go:176 — Expiration of the bridge client’s total-timeout context returns raw context.DeadlineExceeded, which the provider wrapper treats as unclassified. Routine timeouts are consequently marked operator_required instead of retryable. Preserve the caller context and classify local deadlines by phase: ambiguous during submission, retryable during polling or artifact retrieval, while retaining errors.Is compatibility.
Review verdict: Four medium-severity issues could cause incorrect terminal failures, endless metadata retries, or bridge activity beyond authorization expiry.
Medium
Internal provider timeouts are misclassified as caller cancellation Locations:document/datalab/client.go:768, document/docling/client.go:503, document/marker/client.go:653
Each client classifies expiration of its internally configured operation timeout as Canceled. The rendition worker treats that classification as terminal, so routine provider timeouts permanently fail jobs rather than permitting retry or ambiguous-outcome recovery. Distinguish parent-context cancellation from client-imposed deadlines; reserve Canceled for actual caller cancellation and classify internal timeouts as transient or ambiguous as appropriate.
Bridge context errors bypass provider-error classification Location:document/bridge/client.go:174
Submission and polling return raw context.Canceled or context.DeadlineExceeded errors. The rendition worker does not recognize these as provider errors and moves affected jobs to operator_required, including during routine daemon shutdowns and bridge timeouts. Wrap context failures in classified RenditionProviderError values while distinguishing caller cancellation from bridge-imposed timeouts and preserving errors.Is behavior where needed.
Bridge operations can continue after authorization expires Location:document/bridge/client.go:124 Render checks authorization.ExpiresAt only before starting, then bounds execution solely by client.totalTimeout. Uploads, polling, and artifact retrieval can therefore continue after authorization expiry, while receipt validation trusts provider-supplied timestamps rather than actual completion time. Bound the operation by the earliest caller deadline, total timeout, and authorization expiry; recheck expiry before every outbound request and use a separate bounded context for any necessary cleanup.
Metadata list entries bypass the codec’s value-size limit Location:internal/processing/source_metadata.go:174 metadataCollector.strings validates UTF-8 and item count but does not enforce document.MaxSourceMetadataValueBytes for each entry. Oversized values can pass extraction and fail during marshaling, causing source-metadata backfill to retry indefinitely. Omit oversized entries with a warning, consistent with scalar metadata handling.
High-severity reliability issue found, plus three medium-severity retry and metadata-validation risks.
High
internal/processing/source_metadata.go:323 — extractXMLText stores strings.Builder values in a growing slice. Appending nested elements after character data may copy a builder, causing subsequent writes to panic. Ordinary indented nested XML could terminate the daemon during metadata backfill.
Fix: Store builder pointers or safely copyable buffers, bound XML depth and accumulated text, and test nested XML with whitespace and mixed character data.
Medium
document/docling/client.go:362 — Retryable responses to the asynchronous submission POST are treated as ordinary transient errors. A 408 or 5xx may occur after Docling creates the job, so worker retries can trigger duplicate conversions and charges without a task ID for reconciliation.
Fix: Classify these responses as RenditionErrorAmbiguousSubmission unless an idempotency key or durable task handle makes resubmission safe.
document/mistral/rendition.go:413 — Transport failures after the Mistral POST begins are retried internally and then classified as transient, prompting another full worker retry. Without an idempotency key, a lost response can cause repeated billed OCR work.
Fix: Surface post-send uncertainty as RenditionErrorAmbiguousSubmission without automatic resubmission, or use provider-supported idempotency.
internal/processing/source_metadata.go:174 — List-valued metadata does not enforce the canonical codec’s per-item byte limit. Oversized values pass collection but fail serialization, preventing publication and causing indefinite retries.
Fix: Validate codec limits while collecting each item, omit invalid values with a bounded warning, and test that oversized list values still yield a publishable record.
Code changes need revision: three Medium-severity reliability issues could cause unsafe retries, duplicate submissions, or publication with stale resume handles.
Medium
Provider deadlines are misclassified as caller cancellation Locations:document/bridge/client.go:174, document/datalab/client.go:768, document/docling/client.go:503, document/marker/client.go:653
Bridge returns an unclassified ctx.Err(), while the other adapters classify internal timeouts as canceled. When the worker context remains active, these can become operator_required or terminal failures instead of safe retry/ambiguous outcomes. Preserve the caller context, distinguish caller cancellation, authorization expiry, and adapter timeout, then return a classified transient or ambiguous-submission error depending on whether egress occurred.
Docling polling exhaustion can duplicate remote submissions Location:document/docling/client.go:217
Exhausting the polling limit after receiving a task ID returns retryable capacity. Because Docling has neither durable resume nor an idempotency key, retrying may resubmit the document while the original task is still running. Return RenditionErrorAmbiguousSubmission, or checkpoint the task ID through the resumable-provider contract before polling.
Resume checkpoint failures are discarded Location:document/execution.go:274 ResumeRendition does not retain errors encountered while checkpointing a replacement handle. A provider can ignore that failure and return success or a retryable error, allowing publication or retry with a stale handle. Record the first checkpoint error and return it after RenderResumable, mirroring RenderRenditionWithResume.
The changed provider and processing paths preserve authentication, destination pinning, bounded input/output handling, and consent checks without a concrete exploitable boundary bypass.
Verdict: Changes requested — one high-severity security issue and four medium-severity reliability/resource issues.
High
Validated PyMuPDF executable can be replaced before execution Location:document/pymupdf/provider.go:201 New validates the configured executable once with Lstat, but Render later executes the pathname without confirming it still refers to the same file. An account able to modify the executable or a parent directory could replace it and execute code as the Docbank daemon owner. RuntimeIdentity does not mitigate this because the replacement can return the expected value. Fix: Execute a pinned copy from an owner-private managed directory, or pin the executable to a stable file identity and prevent pathname replacement. Validate ownership and permissions/ACLs across the parent chain, derive runtime identity from verified contents, and include it in the policy fingerprint.
Medium
Metadata collection does not enforce canonical codec limits Location:internal/processing/source_metadata.go:174
Oversized list items, labels, or aggregate metadata can pass extraction but fail MarshalSourceMetadataV1, leaving documents permanently queued for backfill. Fix: Enforce all codec limits during collection, including aggregate encoded size, and omit invalid metadata with a bounded warning.
XML extraction permits depth-amplified CPU and memory consumption Location:internal/processing/source_metadata.go:321 extractXMLText copies each character-data token into every open ancestor builder, causing depth-times-text amplification for deeply nested XML. Fix: Capture text only for relevant elements and enforce explicit nesting-depth and aggregate-text budgets.
Stale retry entries can cause continuous SQLite scanning Location:cmd/docbank/backfill_retry.go:38
If a retry target disappears, its expired entry remains indefinitely. waitDelay then continually returns zero, causing both daemon backfill loops to scan SQLite without sleeping. Fix: Reconcile retries against targets seen during a complete scan, or discard expired entries that no longer correspond to missing work while retaining a positive fallback delay.
Docling polling exhaustion can duplicate remote work and charges Location:document/docling/client.go:217
Polling exhaustion after successful submission is treated as a retryable capacity failure. Because the task ID is not persisted, retrying submits another conversion while the original may still be running. Fix: Return an ambiguous-submission error, consistent with the result-fetch path, or persist the task ID and support resumable rendering.
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
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 changed
Docbank now has a verified local PyMuPDF PDF-text provider without giving an ambient Python interpreter or filesystem surface to the rendition path. It sends the exact authorized PDF bytes over stdin to one operator-pinned executable and accepts only the fixed versioned stdout contract.
Docbank proves the PDF page count locally before publishing complete page evidence. Missing pages, partial output, runtime drift, oversized output, cancellation, expired authority, and executable replacement fail closed. Child stderr never crosses the public error boundary.
Why
Local PDF extraction should avoid network disclosure without becoming a privileged path that can reopen arbitrary files, discover runtimes, or publish page claims Docbank cannot verify.
Usage
For library use, construct the provider with
pymupdf.New(profile). The profile pins the exact bridge executable, its SHA-256, immutable runtime identity, timeout, and byte/page limits; Docbank does not install Python or pass a source pathname.This PR does not add daemon, API, or CLI selection. That application wiring is intentionally deferred to S1–S3.
Part of #176 (R12). Stacks on #203.