Enable Chromium to display PDFs in the viewer iframe #924
Conversation
| @@ -412,6 +412,12 @@ ContentResponse::ContentResponse(const std::string& root, bool verbose, const st | |||
| m_mimeType(mimetype) | |||
| { | |||
| add_header(MHD_HTTP_HEADER_CONTENT_TYPE, m_mimeType); | |||
| if ( !startsWith(m_mimeType, "application/pdf") ) { | |||
There was a problem hiding this comment.
@veloman-yunkan Have you tested that this actually catches PDF files? E.g., what if the mimeType isn't set? You might want to check also the file name with a case-insensitive regex like /\.pdf$/i.
There was a problem hiding this comment.
Have you tested that this actually catches PDF files?
On the test case that I am using - it does.
| @@ -412,6 +412,12 @@ ContentResponse::ContentResponse(const std::string& root, bool verbose, const st | |||
| m_mimeType(mimetype) | |||
| { | |||
| add_header(MHD_HTTP_HEADER_CONTENT_TYPE, m_mimeType); | |||
| if ( !startsWith(m_mimeType, "application/pdf") ) { | |||
| add_header("Content-Security-Policy", | |||
| "default-src 'self' data: blob: about: chrome-extension: 'unsafe-inline' 'unsafe-eval'; " | |||
There was a problem hiding this comment.
chrome-extension: would only be needed if you were planning to package Kiwix Serve inside an extension or inside an Electron App (the latter, at least, would be possible, but probably using a node-based server). Seems unlikely, so this could be removed.
There was a problem hiding this comment.
Thanks for the explanation. I thought the meaning of chrome-extension: was to allow running the PDF viewer (which is an browser extension under Chrome) in the iframe.
There was a problem hiding this comment.
@veloman-yunkan I've just tested that in a local copy of Kiwix JS by removing chrome-extension: here and in the meta http-equiv CSP, and it doesn't affect anything even in Chromium 90 (PDFs still load in iframe). In any case, the CSP header here won't be served if the requested file is a PDF. I mean, there is no harm in keeping these extension schemata, but they are not necessary for Kiwix Serve, only for Kiwix JS.
| @@ -2,6 +2,10 @@ | |||
| <html> | |||
| <head> | |||
| <meta charset="UTF-8"> | |||
| <meta http-equiv="Content-Security-Policy" | |||
| content="default-src 'self' data: 'unsafe-inline' 'unsafe-eval'; | |||
| frame-src 'self' moz-extension: chrome-extension:; | |||
There was a problem hiding this comment.
Ditto for both moz-extension: and chrome-extension:. Unlikely that you'd be serving extension URIs. The reason we have these in Kiwix JS is that we package the app inside both types of extension.
|
@veloman-yunkan Just as a general comment on this PR, it will probably need to deal with opening external links, which will be blocked from opening in the iframe (or should be, though I no longer trust the parallels with the case of Kiwix JS). The way I suggested dealing with this was to add an on-click event listener on the iframe's content window (code provided in the relevant issue). Of course it would be worth testing first what happens to external links in this implementation. Another way to deal with it would be to alter the HTML of all external links in the server backend, adding a |
|
@Jaifroid Sounds something maybe better to discuss in kiwix/kiwix-tools#591 which is anyway one of the last tickets still open in kiwix-tools 3.5.0 milestone? |
OK, but I fear this PR will end up blocking any external links from loading if there is no attempt to handle the situation. They should never load in the iframe, but it would be terrible UX if clicking on one just produces the result below.
|
@Jaifroid I primarily published this PR as a means of sharing my changes with you. At the moment my goal is to reproduce your solution in |
|
OK, to understand why this PR isn't working in the same way as Kiwix JS at least vis-à-vis Chromium <= 90 I would need to be able to run it locally, and I can't manage that on my Windows machine unfortunately. I wonder if we might deploy a version of this to the dev server ( |
This fix requires Chromium version above 90.
mgautierfr
force-pushed
the
pdf-friendly-kiwix-serve
branch
from
795c7f3
to
a6659cb
Compare
|
LGTM. At least I haven't found blockers. |
The previous "fix" (merged PR #924) was buggy. It not only didn't work in Chromium v90, but in more recent versions too. I verified that this fix works in Firefox (v111) and Chromium (v90): - Attempts by the ZIM content to break out of the viewer iframe are blocked. - PDFs are displayed in the viewer.
Fixes #916
The fix doesn't work for Chromium versions under 91. Frankly I haven't verified that it works for Chromium 91+ because I only have 90.