Page MenuHomePhabricator

Add a CanCDN flag to uploaded files
ClosedPublic

Authored by 20after4 on Aug 6 2014, 9:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 12, 5:24 PM
Unknown Object (File)
Thu, Dec 12, 11:34 AM
Unknown Object (File)
Fri, Dec 6, 8:23 PM
Unknown Object (File)
Fri, Dec 6, 3:45 AM
Unknown Object (File)
Sat, Nov 30, 8:33 PM
Unknown Object (File)
Wed, Nov 27, 3:50 PM
Unknown Object (File)
Wed, Nov 27, 2:14 PM
Unknown Object (File)
Mon, Nov 25, 12:59 PM

Details

Summary

CanCDN flag indicates that a file can be served + cached
via anonymous content distribution networks.

Once D10054 lands, any files that lack the CanCDN flag
will require a one-time-use token and headers will
prohibit cache to protect sensitive files from
unauthorized access.

This diff separates the CanCDN changes from the code that
enforces these restrictions in D10054 so that the changes
can be tested and refined independently.

Test Plan

Work in progress

Diff Detail

Repository
rP Phabricator
Branch
T5685.cancdn
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 2066
Build 2067: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

20after4 retitled this revision from to Add a CanCDN flag to uploaded files.
20after4 updated this object.
20after4 edited the test plan for this revision. (Show Details)
20after4 added a reviewer: epriestley.
20after4 added subscribers: aklapper, qgil, chasemp.

I will update this diff shortly to pass this flag from all of the places where we create files which should be public (canCDN) by default.

Here are the places I was able to identify that need the canCDN flag:

  • add_macro.php, CLI creation of new image macros.
  • All calls in PhabricatorImageTransformer: the rule this will implement is "image thumbnails can always CDN", which I think is reasonable. We do not permit users to choose their thumbnail dimensions arbitrarily, so this can never show you the source file by generating a thumbnail with source dimensions unless the source is very small.
  • PhabricatorPeopleProfilePictureController, the Gravatar profile image write and the explicit upload write.
  • PhabricatorMacroAudioController, the audio write.
  • PhabricatorMacroEditController, both the from-file and from-url writes.
  • PhabricatorProjectEditPictureController, the explicit upload write.
  • PhabricatorAuthProvider, the profile image write.
  • PhabricatorFileComposeController, the write of generated project profile images.

And possibly:

  • FileUploadConduitAPIMethod should maybe expose this (and maybe viewPolicy) but we can wait for use cases.

I'm omitting:

  • On consideration, let's leave Pholio off the canCDN list for now, since conceivably it could have sensitive stuff in it and the number of actual image requests is very small and it's reasonable to special-case it later and somewhat complicated to get the flag set.
  • I'm leaving the write in PhabricatorRemarkupGraphvizBlockInterpreter off for similar reasons.

In general, I've compiled this list by looking for calls to PhabricatorFile::newFromFileData(), PhabricatorFile::newFromPHPUpload(), PhabricatorFile::newFromFileDownload(), PhabricatorFile::newFromXHRUpload(), or PhabricatorFile::buildFromFileDataOrHash() and then calling out the ones that generate a reasonably-public, commonly used file.

(For completeness, @rush898 is tackling the Conduit method in D10164.)

20after4 edited edge metadata.

Set canCDN => true from various file creation callsites.
This includes just the ones @epriestley suggested,
there may be more that should be addressed.

Moved conduit upload api changes into D10164 so I've amended
this revision to exclude FileUploadConduitAPIMethod.php

epriestley edited edge metadata.

One tiny nit.

src/applications/files/storage/PhabricatorFile.php
867

For consistency within the codebase, omit the = true default.

This revision now requires changes to proceed.Aug 7 2014, 4:39 PM
20after4 edited edge metadata.

nit picked...

epriestley edited edge metadata.
This revision is now accepted and ready to land.Aug 8 2014, 1:56 AM
epriestley updated this revision to Diff 24511.

Closed by commit rP12aaa942acc5 (authored by @20after4, committed by @epriestley).