PDF thumbnail generation false-positive for legitimate large scanned documents (MAX_MEDIABOX_MATCHES)

[mitpjones]

Uploading a legitimate large scanned PDF results in a broken-image placeholder in the gallery instead of a thumbnail of the first page, the file uploads fine (it's stored as the original), but no thumbnail/preview is ever generated for it, and there's no setting to fix this.

Root cause: ImagickHandler::assertPdfPageSizeIsSafe() rejects thumbnail generation for any PDF with more than MAX_MEDIABOX_MATCHES (25) /MediaBox occurrences in the first MEDIABOX_SCAN_LIMIT (1MB) of the file, a hardening check from #4511 and #4687 meant to stop crafted PDFs from making Ghostscript burn CPU rendering an absurd page size.

I hit this uploading a 42MB, ~112-page scanned yearbook: it has 112 real /MediaBox occurrences, all within the first 1MB (a normal PDF layout, the page tree is front-loaded, bulk image data comes after). That's enough to trip the cap and silently skip thumbnail generation, with only a log line (PDF declares more than 25 /MediaBox occurrences...) to show for it, nothing visible to the uploader explaining why their upload has no preview.

The match-count cap doesn't actually contribute to the security property here: every occurrence, no matter how many, is already individually checked against MAX_PDF_MEDIABOX_POINTS (max dimension) and MAX_MEDIABOX_AREA_PER_BYTE (implausible area for file size), so a genuinely malicious oversized/implausible page still gets caught on the first bad occurrence regardless of the cap. The cap only bounds the cost of the scan loop itself, which is already cheap (regex + arithmetic over an already-1MB-bounded buffer) even at much higher counts, raising it doesn't reopen the CPU-exhaustion hole #4511/#4687 closed.

Happy to send a PR raising MAX_MEDIABOX_MATCHES (and updating the doc comment claiming "a real-world PDF has no reason to contain more than a handful of /MediaBox tokens," which isn't true for scanned archival documents) if that approach sounds right, wanted to float the idea here first in case there's context I'm missing about why 25 was chosen.

[ildyria]

I think then a better approach is to make it configurable.
You can see https://github.com/LycheeOrg/Lychee/pull/4821/changes how to create a new config. In this specific case the type should be positive to ensure that only positive value are set:
https://github.com/LycheeOrg/Lychee/blob/master/app/Models/Configs.php#L154C21-L154C29

Using the ConfigManager (resolve(ConfigManager::class) if it is not present already) you can retrieve the value as int:
https://github.com/LycheeOrg/Lychee/blob/master/app/Repositories/ConfigManager.php#L94

This will allow an even better granularity. The point of the 25 is to avoid infinite recursions and set a limit.

A PR will be well appreciated :)

[ildyria]

merged and thus available on edve :)

[mitpjones]

Thanks