Page MenuHomePhabricator

Fully implement granular file permissions and upload defaults
Closed, ResolvedPublic

Description

All files currently have "All Users" permission. This is sometimes unexpected, not really very desirable, and generates reports on HackerOne. Specifically, the expectation is that when you upload a file to an object (like a Conpherence) it should have very narrow default permissions and then an exception punched through them for the object.

This is mostly supported in other applications (especially after T2222) and we should be able to finish implementation now.

Event Timeline

epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added a project: Files.
epriestley added a subscriber: epriestley.

In FilesQuery, we need to drop the "throw on failure" flag from the subquery for linked objects. Currently, if you loadOne() a file and can't see any of the linked objects, we prevent you from seeing the file.

qgil moved this task from Important to Potential blockers on the Wikimedia board.

@epriestley How much is really remaining to do here? Assuming we don't care about audit (we'll only be using maniphest initially) is there any way to make maniphest attachments private without waiting for T4896? Is this something we could contribute patches towards? It looks like this is the last remaining blocker for Wikimedia to go live with Maniphest replacing Bugzilla and RT.

This is probably hard to drive forward without my involvement. We could work around T4896, but I'm very hesitant to do so since we'd either leave a (fairly bad) policy inconsistency in the application (file policy rules would not work as expected in Diffusion/Audit) or be writing code that we'd have to remove during T4896, and which would make T4896 more difficult to complete.

T4896 isn't particularly difficult, but it involves data migrations that are risky to get wrong. If you're feeling especially brave, T2222 is a similar migration which we previously completed. The steps we've had success with so far are roughly:

  • modernize all query APIs;
  • add "double reads": make the old datasource readable into either the old or new datastructures. The new datastructure proxies the old one and still does writes to the old table.
  • convert all reads/writes to go through the new datastructure;
  • do the migration, and remove the proxy layer. This diff is the dangerous one and should be as small as possible
  • clean things up

This needs to happen twice, once for inline comments (must happen first, generally more difficult) and once for top-level transactions (must happen second, generally easier).

You could try to pursue this by following the footsteps of T2222, which went through these steps for Differential. T4896 is easier, but still on the same order of magnitude. I worry that helping someone through this process will take more of my time (and involve much more back-and-forth overhead) than doing it myself, though, and at the end of the day we wouldn't get anything else out of it, since expertise in the system the migration deletes isn't valuable and this is the last nontrivial migration we need to perform. It's also a ton of work that probably doesn't directly benefit WMF.

Short of doing it the right way, you could do something really hacky:

  • In PhabricatorFileDropUploadController, add 'viewPolicy' => $user->getPHID() to the PhabricatorFile::newFromXHRUpload() call.
  • Do this in PhabricatorFileUploadController as well.
  • I can land D8498.

This will basically be as buggy as things are now, but fail closed (with no one able to see files) instead of failing open. For example, all files uploaded to Diffusion will be visible only to the uploader if you make these changes.

T4896 has come along far enough that this is now technically unblocked, but I plan to finish it up before focusing here.