Bug 42080 - Invoice PDFs are forced downloads
Summary: Invoice PDFs are forced downloads
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Eric Phetteplace
QA Contact: Testopia
URL:
Keywords: dependency
Depends on: 41591 42087
Blocks:
  Show dependency treegraph
 
Reported: 2026-03-12 18:30 UTC by Eric Phetteplace
Modified: 2026-03-13 02:59 UTC (History)
3 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Crowdfunding committed: 0
Crowdfunding contact:
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 42080: Invoice PDFs are forced downloads (1.42 KB, patch)
2026-03-12 20:49 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 42080: Invoice PDFs are forced downloads (1.47 KB, patch)
2026-03-13 02:50 UTC, David Cook
Details | Diff | Splinter Review
Bug 42080: (follow-up) allow some file types, checked by libmagic, to be viewed inline (5.34 KB, patch)
2026-03-13 02:50 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Phetteplace 2026-03-12 18:30:09 UTC
Our acquisitions technician preferred being able to view invoice PDFs directly in the browser, behavior which bug 41591 changed to prevent a SVG XSS attack.

We would prefer if invoice-files.pl served files inline with a CSP header instead of forcing a download. Basically, replace -attachment => $file->{'file_name'} with 'Content-Security-Policy' => "default-src 'none'; script-src 'none';".

Arguably, Koha could add an op=view here so we could use JS to change the current op=download links to op=view if other libraries do not want to view files inline. But my assumption is that what our staff want is popular; it's not just extra clicks to find the downloaded PDF but a context shift to a reader app outside the browser.

A more involved solution would be using a condition to download SVGs but render other attachment types inline. I believe inferring mime type from file extension is fine in this context since a browser will not execute code in an SVG served with a PDF mimetype, so "malicious.svg.pdf" would not work.
Comment 1 Eric Phetteplace 2026-03-12 20:41:26 UTC
FYI I tested my proposed changed (CSP header instead of attachment header) and the results look correct. I have two identical files named malicious.svg and malicious.svg.pdf with a `console.log("hello")` script in them:

- malicious.svg: browser console shows "Executing inline script violates the following Content Security Policy directive 'script-src 'none''." and nothing is logged
- malicious.svg.pdf: browser says "Error: Failed to load PDF document" because file is not a valid PDF
Comment 2 Eric Phetteplace 2026-03-12 20:49:06 UTC
Created attachment 195226 [details] [review]
Bug 42080: Invoice PDFs are forced downloads

Serve invoice files with a CSP header that prevents XSS
instead of with an attachment header that turns them into
downloads.

Testing plan:
- Set AcqEnableFiles preference to Do
- Acquisitions > Vendor > Receive Shipment > Create invoice
- Finish Receiving > Manage invoice files
- Upload an SVG with a <script> tag in it
- Click the SVG filename to open it
- Inspect browser's console: it should say something about a
script being blocked due to the CSP directive
Comment 3 Eric Phetteplace 2026-03-12 20:50:18 UTC
OK I added my patch and testing plan just to clarify but I leave this at status NEW until I can get some feedback. I also asked in Mattermost.

For testing, you can use this as your "malicious.svg" file:

<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 400 400">
<title>malicious</title>
<text x="200" y="200" text-anchor="middle" font-size="20" fill="black">
  This SVG contains a script tag which prints "hello" to the console
</text>
<script>console.log("hello")</script>
</svg>
Comment 4 David Cook 2026-03-13 00:11:51 UTC
Hi Eric,

We're actually already working on bringing Content-Security-Policy into Koha with bug 38365, so it looks like great minds think alike!

That said, I think I'll need to think on this one a bit more. I love Content-Security-Policy, but it won't be turned on out of the box, and it'll be set globally so the same header will be sent for all pages sent by Starman. (I am curious though about layering Content-Security-Policy headers. Perhaps we can include the one you've provided PLUS the global one. I'm going to look into that shortly.)

Another thing is while Content-Security-Policy is great (seriously I truly love it), it is only one layer of defence. Ideally it would be good to have multiple. 

So I'm going to think a bit more about what we could do in terms of data validation. 

With bug 41591 I really didn't think that it would make that much of a difference to end users, so thanks for this report. It's interesting to know that people really do want to view the file inline. Different strokes for different folks, but Koha tries to be all things to all people, so let's try to figure this one out...
Comment 5 David Cook 2026-03-13 02:03:11 UTC
(In reply to David Cook from comment #4)
> That said, I think I'll need to think on this one a bit more. I love
> Content-Security-Policy, but it won't be turned on out of the box, and it'll
> be set globally so the same header will be sent for all pages sent by
> Starman. (I am curious though about layering Content-Security-Policy
> headers. Perhaps we can include the one you've provided PLUS the global one.
> I'm going to look into that shortly.)

Fortunately, it does appear to be possible to have multiple Content-Security-Policy headers. The browser will interpret both of them and if at least one of them fails it will block, so yeah this approach could work well in this case.

That said, I think it would probably be better to undo bug 41591 and re-implement with solutions from this bug report. 
 
> Another thing is while Content-Security-Policy is great (seriously I truly
> love it), it is only one layer of defence. Ideally it would be good to have
> multiple. 
> 
> So I'm going to think a bit more about what we could do in terms of data
> validation. 

It looks like we store the Content-Type header in misc_files.file_type. That and the file extension are both user-supplied fields though, which cannot inherently be trusted. 

I think a good first step would be magic number checking. There is a library available that can help with that. 

So I think your patch is a good starting place, Eric. I'm just going to build a bit on top of it.
Comment 6 David Cook 2026-03-13 02:50:13 UTC
Created attachment 195245 [details] [review]
Bug 42080: Invoice PDFs are forced downloads

Serve invoice files with a CSP header that prevents XSS
instead of with an attachment header that turns them into
downloads.

Testing plan:
- Set AcqEnableFiles preference to Do
- Acquisitions > Vendor > Receive Shipment > Create invoice
- Finish Receiving > Manage invoice files
- Upload an SVG with a <script> tag in it
- Click the SVG filename to open it
- Inspect browser's console: it should say something about a
script being blocked due to the CSP directive

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 7 David Cook 2026-03-13 02:50:16 UTC
Created attachment 195246 [details] [review]
Bug 42080: (follow-up) allow some file types, checked by libmagic, to be viewed inline

This patch uses File::LibMagic to determine the real file type using
libmagic and magic numbers of invoice files, and allows a select group
of file types to be viewed inline for user convenience.

Note that a default Content-Security-Policy is still included to offer
an extra layer of protection against XSS.

Test plan:
0a. Apply the patch
0b. koha-plack --restart kohadev
1. Enable the syspref AcqEnableFiles (ie set to Do)
2. Find or create an active vendor
3. Create a basket if one doesn't already exist
4. Receive a shipment for the vendor
5. Create an invoice
6. Click "Finish receiving"
7. Click "Manage invoice files"
8. Upload a range of files including SVG, PDF, PNG, TXT
9. Click on the "Download" button
10. Note that the file downloads for all types
11. Click on the link in the "Name" column
12. Note that only the PDF and PNG display inline in the browser.
The other file types should be downloaded same as if you clicked the
"Download" button.

Signed-off-by: David Cook <dcook@prosentient.com.au>