Page MenuHomePhabricator

Improve file behaviors around POST requests and downloads
Closed, ResolvedPublic

Description

See T4340.

Currently, we can not specify a narrow form-action 'self' Content-Security-Policy because we sometimes create forms which point to the CDN domain ("alternate file domain"). Specific examples include:

  • "Download" button in Files.
  • "Download" buttons in "Export Data", "Harbormaster Logs", and "Generate SSH Key".
  • Maybe some stuff on the file lightbox thing?

We don't want to serve file content requests from the main domain because an attacker can do this:

  • Upload an evil .html file.
  • Create an HTML document with <iframe src="..." /> pointing at that file somewhere.
  • Convince a victim to load the HTML document.
  • The file loads as HTML, runs scripts on the primary domain, and can start cookie smashing and making Ajax requests or whatever.

(Or maybe they upload an evil SWF or applet and use <object />? I'm less sure if these attacks have ever been possible.)

In theory, headers like Content-Type: hey-buddy-this-is-definitely-not-html-so-don't-try-to-execute-it, Content-Disposition: attachment, X-Frame-Options: Deny, and now frame-ancestors 'none' should prevent this. However, they have a dismal track record of actually working across browser and devices: older IE used to content sniff and decide that "text" documents were probably really HTML and should be executed (unless you X-Content-Type-Options: nosniff, which we do), and older iPads would simply ignore Content-Disposition: attachment completely so you didn't need the <iframe /> at all.

So, on top of the four existing layers we already have, we currently additionally require POST requests if the file is hosted on the primary domain, since these are hard to make with <iframe ...> or <object ...> tags.

We could reasonably drop this requirement and just say: we have four layers of protections against this already; browsers probably won't make this mistake too many more times; anyone who cares should follow the setup warning and configure an alternate file domain; and this may not be effective anyway if the other layers of protection fail (e.g., you could just have Javascript POST to the file on older iPads).

Actually getting the POST to work is also a super inconvenient mess, since it means we need to render UI elements which are not forms as forms. This "Download File" link is really a sneaky super-secret form:

Screen Shot 2018-02-28 at 2.45.08 PM.png (223×458 px, 16 KB)

The "Harbormaster Logs" and "Export Data" workflows each have an extra step so we can get a form out of things.

Separately, there's THIS mess (the UI element -- not the PDF):

Every time I interact with this thing I'm surprised by the behavior. I'm not going to bother trying to click it since I think it will destroy all my work; I'll document the issues below.

Tentatively, I'm going to do this:

  • Add a separate route for downloading files like /file/download/....
  • When this is served from the main domain, it will render a standalone dialog with a download button that POSTs to the same page.
  • This dialog won't appear when the route is served from the CDN domain.
  • Every application that wants to do file stuff can then just Location redirect or link normally without worrying about rendering <form> tags that look like links or popping up 17 interstitial confirm dialogs.

Event Timeline

epriestley created this task.

Okay, here's another one of these:

  • "Download" is a form, so you can't command-click it.
  • The whole thing is a <div href="..." /> (huh?) so you can't command-click it to open it in a new window.
  • When you click it for a non-image file, you get this weird interstitial that you can leave comments on if you click an additional button, which uses janky animations and AJAX. This feature is pretty half-baked and I've never seen anyone actually use it. It's possibly a net negative in its current form.
  • There is no way to actually show the text file in the browser! ARHGRH

These changes are all deployed here, now. The embed element only got touched lightly but is at least slightly better. See T4340 for further adventures in Content-Security-Policy.