Repository navigation
Conversation
When a PDF has files embedded at the document level, the sidebar gets a Files tab that lists them and saves each one exactly as it was attached. PDFs without attachments show no extra tab. Names come from the PDF, so they are reduced to a plain file name before being shown or saved. Files attached to a page as annotations and reading attached PDFs in their own tabs are left out, as agreed in Forward-Future#53.
mberman84
left a comment
There was a problem hiding this comment.
The Files tab covers the agreed first part of #53. On macOS the type check, production build, all 266 unit tests, and hidden smoke suite passed, including saving attachments byte for byte.
Please address the 300 MB limit before merging. Folio currently asks PDF.js for the complete attachment and checks the byte length after it returns. That keeps an oversized file off disk, but the reader has already had to allocate it. Please enforce a bound while reading the contents, or use an extraction path that can stop at the limit and keep the document usable. A small fixture with a reduced test limit is sufficient.
For the executable-file question in the description, I recommend an explicit confirmation that shows the full filename before handing an executable attachment to the save dialog. The automated desktop test verifies the suggested filename and downloaded bytes. It bypasses the native save dialog.
… one that could run An attachment was unpacked whole by pdf.js and only then checked against the 300 MB limit. It is now read from the PDF's own bytes and unpacked a small piece at a time, stopping once it passes the limit. Attachments stored in a form that can't be read that way (a protected PDF, a filter other than a single Flate, a predictor, a length that doesn't fit) are refused with a message instead of being saved wrong. Saving a file whose name could run when opened first shows its full name and asks.
…ke Folio unpack or wait for Finding an attached file means reading the whole PDF with pdf-lib, which unpacks every packed list of objects in it. That now happens in a worker, stops once 300 MB has been unpacked, and is given up after 30 seconds, so a PDF built to be expensive no longer freezes the window on Save. Also: an empty file, a single filter with empty settings, a name starting with the UTF-8 mark and data followed by a counted line break now save; the question before saving a file that could run always shows the whole name and covers more endings; one file is saved at a time.
|
Both points are addressed in 7dd0c65 and dc8ad0e, and the description above is updated to match. CI passed on Windows, macOS and Linux in my fork for dc8ad0e. Size limit. PDF.js has no way to stop partway through an attachment, so the contents are no longer read through it. pdf-lib locates the embedded stream and the browser's Executable attachments. Saving a file whose ending can run, install or mount something first asks, showing the whole file name, with Cancel focused. The smoke test attaches Three things I'd like your call on:
This PR is larger than the first version (a worker file and more tests). If you'd prefer it split, the confirmation dialog separates cleanly from the read path. |
mberman84
left a comment
There was a problem hiding this comment.
Thanks for addressing both earlier review points. The save now stops decoding the attachment when it reaches the limit and asks before saving a runnable file. I verified that all three platforms passed in your fork at dc8ad0e.
I’d address the remaining memory issue in this PR. The save worker still parses the whole PDF before it can extract the attachment. The 300 MB check covers decompression buffers, and the timeout covers elapsed time. Neither bounds the parser’s total memory. Your measurement of roughly 2.8 GB makes that a blocker for an everyday Save action: a reader could lose unrelated unsaved work if the renderer runs out of memory.
A small first version can refuse a file it cannot read within a real memory bound. The protected-PDF refusal is reasonable. Please keep the current document-level scope and verify that the parent PDF and its edits stay usable when extraction is refused.
Problem
A PDF can carry files embedded in the document itself (a spreadsheet, a source file, a signed copy), but Folio gave no sign they were there and no way to get them out.
Change
When a PDF has files attached at the document level, the sidebar gets a Files tab next to Pages, Outline and Notes. It lists each file with its name and description, and the button on a row saves that file exactly as it was embedded. PDFs without attachments show no extra tab.
The names, descriptions and contents come from the PDF, so they are treated as untrusted:
DecompressionStreamunpacks it in 16 KB input pieces; the read is cancelled once it passes 300 MB (the existing PDF size limit).Files attached to a page as annotations, and reading attached PDFs in their own tabs, are left out, as agreed in the issue. The README's "Read and navigate" list has one new line.
What this does not save, and what it does not bound
Reading within a limit narrowed what can be saved. These are refused with a message, never saved wrong:
Three things are for your judgement:
PDFDocument.loadin its worker, so this is not a new kind of exposure, but Save now reaches it too. Containing it fully would mean a separate process with a memory cap, or a purpose-built reader for the name tree; I can do either here or as a follow-up if you want it.js,pyandsh, so code attached to a paper will ask. It leaves outdoc,xls,htmlandsvg, so the question keeps its meaning.Issue or approval
Refs #53. Scope approved in #53 (comment) ("lists attachments embedded at the document level and lets the reader save one. A small PDF fixture can verify that the saved bytes match the attachment."), and the two changes requested in review.
How I checked it
src/attachments.test.ts(15 tests) builds small PDFs with pdf-lib and checks, against real pdf.js where a listing is involved: the listed names and descriptions; that each file's bytes come back identical (a buffer with every byte value); the storage forms that save and the ones that are refused; a protected PDF; the name cleanup and which names ask.scripts/smoke-attachments.mjs, called fromcheckReaderTools) opens a PDF with a binary file, a text file, a file with no extension andQuarterly report.pdf.bat. It saves each through the Files tab and compares the saved files with the originals byte for byte and the offered names with the listed ones. For the.batit checks the question shows that exact name, that Cancel saves nothing, and that Save does. It also checks that a PDF without attachments has no Files tab.npx tsc --noEmit, the attachment unit tests, and the attachments smoke check with a hidden window.Not checked: Windows and macOS by hand (CI only); the native save dialog, which the smoke check bypasses by setting the save path; whether Windows or macOS mark the saved file as downloaded; PDFs written by Acrobat, Word, LibreOffice or pdfTeX.
Before and after, the same PDF with three attached files:
Before:
After:
Model and harness: Claude Fable 5.2 in Claude Code.
🤖 Generated with Claude Code