The module lets the admin user choose between two possible "Download behavior" values: "attachment" (default) or "inline". However, the module always sends a "Content-Disposition" header field, regardless of the selection. Sending that header field would only be required to trigger a "save as..." dialog ("attachment" behaviour) for the requested file (by requesting the disposition-type "attachment") , but not if the requested file should be processed directly by the browser ("inline" behaviour). Currently, the module sends a "Content-Disposition" header field with the disposition-type "inline" in that case.
Sending a "Content-Disposition" header field with a disposition-type other than "attachment" should generally be avoided during HTTP communications. It has a dangerous potential and can cause incompatibilities with certain browsers (especially on mobile devices), so it would be better to entirely unset the "Content-Disposition" header field at all if it is not of disposition-type "attachment".
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | private_download-2010092-D6.patch | 1.42 KB | roball |
Comments
Comment #1
roball commentedThe attached patch against 6.x-1.x fixes the issue.
Comment #2
roball commentedComment #3
johnhanley commented@roball,
Thanks for your contribution.
The patch looks reasonable. I don't personally have time to test it, but perhaps somebody from the community can help.
Community, what say you?
John
Comment #4
anrikun commentedHi roball,
Can you provide some reference about this:
Comment #5
roball commentedHi anrikun,
sure. The standard for defining HTTP/1.1 is RFC 2616. The "Content-Disposition" header field is not even part of any HTTP standard, however RFC 2616 mentions it because it is actually being used even if it's non-standard, and it's a potential security risk. See section 15 - Security Considerations of the RFC:
Comment #6
anrikun commentedThanks roball.
You said:
Is it normal that your patch sends string 'Content-Disposition: ' instead of nothing when download behavior is inline then?
Comment #7
roball commentedYou are welcome :-)
Yes, absolutely. Actually, returning the field without a value unsets it. If we would return no field at all, then we might end up in being the "Content-Disposition" header field sent with a disposition-type of "attachment". This was the case in my tests. I think the header field was sent by Drupal core. It's also possible that is was caused by another module that implemented the hook file_download. If we send the empty header value, we ensure that it gets unset.
Comment #8
anrikun commentedRewritten a bit and committed.
Thanks roball!
Comment #9
roball commentedThank you very much anrikun for taking rapid action and releasing 6.x-1.5. Now we are on the safe side. Imagine if
.would have been added to the "Inline type patterns" list.I think we can even also omit sending "Content-Disposition: attachment; ..." if $disposition_attachment is TRUE since Drupal core seems to send it always on requests through system/files. This was at least the case in my tests, however that should be tested with only private_download.module enabled and all other modules disabled to be sure. So we only need to send "Content-Disposition: " if $disposition_attachment is FALSE (to unset it), otherwise we send no header field at all.
IMO, modules should be very careful when sending HTTP header fields and avoid that generally if not really mandatory, since it has a potential to break things. This is also the reason why I would vote for removing the "file headers" possibility. That just opens a potential hole. It is currently even a required field, so it forces the admin to do some potentially dangerous action through a GUI. It seems that Drupal core sends "Content-Transfer-Encoding: binary" and "Cache-Control: private" anyway on requests piped through system/files.