Bug 28123 - Commas in file names of uploaded files cause inconsistently broken 856$u links
Summary: Commas in file names of uploaded files cause inconsistently broken 856$u links
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-09 14:33 UTC by Andrew Fuerste-Henry
Modified: 2021-04-25 20:44 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05,20.05.11


Attachments
Bug 28123: Quote filename value in Content-Disposition header (1.62 KB, patch)
2021-04-13 00:43 UTC, David Cook
Details | Diff | Splinter Review
Bug 28123: Fix expected value in unit test (1.15 KB, patch)
2021-04-13 00:46 UTC, David Cook
Details | Diff | Splinter Review
Bug 28123: Quote filename value in Content-Disposition header (1.67 KB, patch)
2021-04-15 10:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28123: Fix expected value in unit test (1.19 KB, patch)
2021-04-15 11:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28123: Quote filename value in Content-Disposition header (1.67 KB, patch)
2021-04-15 19:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28123: Fix expected value in unit test (1.25 KB, patch)
2021-04-15 19:27 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2021-04-09 14:33:05 UTC
To recreate:
- have a file with a comma in its name
- enable the upload plugin on the 856$u in a framework
- edit a record in that framework, upload your file in the 856$u
- go to your record in the OPAC, click the link for the 856$u
- get the error "This page isn't [catalog name] sent an invalid response.
ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION"

This error is inconsistent. It might be simply that it works in Firefox but not in Chrome.
Comment 1 Jonathan Druart 2021-04-12 14:41:40 UTC
Working OK for me on master, tested with Firefox and Chromium (87).
Comment 2 David Cook 2021-04-13 00:06:03 UTC
Ah yes I know this problem from working on DSpace. I think that it's a Chrome-only bug.
Comment 3 David Cook 2021-04-13 00:37:34 UTC
This is actually just a problem for PDF downloads.
Comment 4 David Cook 2021-04-13 00:43:27 UTC Comment hidden (obsolete)
Comment 5 David Cook 2021-04-13 00:46:09 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2021-04-15 10:59:58 UTC
Created attachment 119610 [details] [review]
Bug 28123: Quote filename value in Content-Disposition header

Chrome throws a ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION
error if a Content-Disposition header includes an unquoted filename
that contains commas.

This patch adds quotes around the filename in the Content-Disposition
header, which fixes the problem.

Test plan:
0. Do not apply the patch yet
1. Obtain a PDF and rename it to include a comma in its name
2. Enable the upload.pl plugin in the 856$u of the Default framework
3. Add/edit a record in the Default Framework and upload a file
using the 856$u upload plugin.
4. Click on the resulting file URL in the saved file
5. Note the error ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION
is returned instead of the file

6. Apply the patch and restart your Plack instance

7. Click on the resulting file URL in the saved file
8. Note that you now get the PDF file instead of the error

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 7 Owen Leonard 2021-04-15 11:00:02 UTC
Created attachment 119611 [details] [review]
Bug 28123: Fix expected value in unit test

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Nick Clemens 2021-04-15 19:26:58 UTC
Created attachment 119684 [details] [review]
Bug 28123: Quote filename value in Content-Disposition header

Chrome throws a ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION
error if a Content-Disposition header includes an unquoted filename
that contains commas.

This patch adds quotes around the filename in the Content-Disposition
header, which fixes the problem.

Test plan:
0. Do not apply the patch yet
1. Obtain a PDF and rename it to include a comma in its name
2. Enable the upload.pl plugin in the 856$u of the Default framework
3. Add/edit a record in the Default Framework and upload a file
using the 856$u upload plugin.
4. Click on the resulting file URL in the saved file
5. Note the error ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION
is returned instead of the file

6. Apply the patch and restart your Plack instance

7. Click on the resulting file URL in the saved file
8. Note that you now get the PDF file instead of the error

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Nick Clemens 2021-04-15 19:27:02 UTC
Created attachment 119685 [details] [review]
Bug 28123: Fix expected value in unit test

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Jonathan Druart 2021-04-16 10:30:19 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 11 Fridolin Somers 2021-04-22 14:17:57 UTC
Pushed to 20.11.x for 20.11.05
Comment 12 Andrew Fuerste-Henry 2021-04-25 16:27:48 UTC
Pushed to 20.05.x for 20.05.11
Comment 13 Victor Grousset/tuxayo 2021-04-25 20:44:22 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.