This isn't quite done but I wanted to make sure I'm on the right track with the one time token stuff and my logic in the data controller.

It doesn't handle redirect loops yet and I'm not quite sure where the canCDN flag should be set, but other than that I think this should be close to correct.

For no domain being set, I still want things to work -- a lot of installs don't have a setting here yet, and new installs won't either. We can do a normal login check on these files -- just make sure they aren't cacheable.

I think we just add a first if ($alt_domain === null) branch, so the overall logic looks like this:

if (no alt domain configured) {
  load the file with permissions checks;
  return the file data without cache headers;
} else if (we are on the primary domain) {
  load the file with permissions checks;
  if the user can see it, generate a token;
  redirect to the alt domain with the token;
} else if (a token exists) {
  consume the token, if it is valid;
  load the file, bypassing permission checks;
  return the file data without cache headers;
}

Some of that might be able to be simplified, but I think that's roughly how the behaviors should work.

For the migration, I expect it to look something like this:

$table = new PhabricatorFile();
$conn_w = $table->establishConnection('w');
foreach (new LiskMigrationIterator($table) as $file) {
  $id = $file->getID();
  echo "Updating flags for file {$id}...\n";
  $meta = $file->getMetadata();
  if (!idx($meta, 'canCDN')) {

    $meta['canCDN'] = true;

    queryfx(
      $conn_w,
      'UPDATE %T SET metadata = %s WHERE id = %d',
      $table->getTableName(),
      json_encode($meta),
      $id);
  }
}

Migrations are no fun and it would be nice not to have to do it, but I think we're much better off in the long run if the default is the "safe" option.

Still a bit more to do I suspect, and the migration isn't included here...

The logic in PhabricatorFileDataController should be just about right, but I wouldn't mind a second set of eyeballs. I deviated slightly from the pseudocode in @epriestley's review, here's why:

1. I don't consume the token if it's a range request, I don't know if there is a good way to handle this since there could be multiple requests for the same file/token. Do tokens get garbage collected eventually? If so, maybe it's ok to leave it for garbage collection in this case.
2. I'm just returning a 403 when there is a request meeting all of these conditions: using alt domain, without a token, CanCDN is false. I probably should redirect to the main domain to get a token in this case but this is the situation where we need to prevent redirect loops. I'll think a bit more about this one and attempt to handle that properly.

Tokens are automatically GC'd, yeah. Your range behavior seems reasonable. I think this only actually impacts audio for image macros, and the browser(s) in question might only actually make one request (?) for the whole file (??) -- it's been a while. But generally this seems like a reasonable line of attack.

I think if you redirect instead of doing the second 403 this is basically in good shape.

ok: all issues should be addressed in the incoming update....

Cool, this looks correct to me. We also need to set the canCDN flag on new profile pictures, macros, Pholio mocks, and thumbnails -- without that, this will throw them into big redirect messes. Do you want to do that as a separate diff and we can just hold this until that's ready?

Thinking about it, that can actually land first. That is, do one diff which:

• adds support for a canCDN flag to PhabricatorFile::buildFromFileData();
• sets the flag on files created via profile pictures, macros, Pholio mocks, thumbnail transformations, and maybe Remarkup .dot file images -- let me grep around and try to get you an exhaustive list; and
• adds a property on the File detail view (maybe on the "File Info" tab) showing the value of the flag for a given file, so it's easier to test/debug that things are working right.

We can land that, kick the tires on it a bit, and then land this.

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.

@epriestley ok so do you think the migration should be in this diff or the other one?

I think doing it in either place is OK, whatever's easiest.

If you rebase this (since some of it happened in D10166), I think we're good to go.