Page MenuHomePhabricator

Remove PhabricatorFile::buildFromFileDataOrHash()
ClosedPublic

Authored by epriestley on Apr 4 2017, 9:32 PM.
Tags
None
Referenced Files
F19260888: D17617.diff
Mon, Dec 22, 4:29 PM
F19054680: D17617.id.diff
Fri, Nov 28, 2:16 PM
F19047721: D17617.id.diff
Thu, Nov 27, 1:17 PM
F19040665: D17617.diff
Wed, Nov 26, 1:07 PM
F19024603: D17617.diff
Mon, Nov 24, 3:44 AM
F18839073: D17617.diff
Oct 27 2025, 2:14 PM
F18835093: D17617.id.diff
Oct 26 2025, 1:19 PM
F18831465: D17617.diff
Oct 25 2025, 1:14 PM
Subscribers

Details

Summary

Ref T12464. This is a very old method which can return an existing file instead of creating a new one, if there's some existing file with the same content.

In the best case this is a bad idea. This being somewhat reasonable predates policies, temporary files, etc. Modern methods like newFromFileData() do this right: they share underlying data in storage, but not the actual File records.

Specifically, this is the case where we get into trouble:

  • I upload a private file with content "X".
  • You somehow generate a file with the same content by, say, viewing a raw diff in Differential.
  • If the diff had the same content, you get my file, but you don't have permission to see it or whatever so everything breaks and is terrible.

Just get rid of this.

Test Plan
  • Generated an SSH key.
  • Viewed a raw diff in Differential.
  • (Did not test Phragment.)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable