Page MenuHomePhorge

Fix view policy inheritance on image transforms
ClosedPublic

Authored by l2dy on Nov 10 2023, 15:10.

Details

Reviewers
speck
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
Restricted Maniphest Task
Commits
rP005fea5a14f4: Fix view policy inheritance on image transforms
Summary

Inherit viewPolicy from original image in image transforms and warn about Profile transform making transformed images public. Details:

https://hackerone.com/reports/1984060

https://github.com/mozilla-conduit/phabricator/commit/8358b435a99435a95e0dffbbb92f71dc1961bc7b

Closes T15663

Test Plan
  1. Click View Transforms on an image file with restrictive view policy.
  2. See (Image will be Public) warning on Profile transform.
  3. Click on Workcard transform.
  4. Go back to View Transforms page and visit the Workcard transformed file.
  5. Check if its view policy matches the original file.

Diff Detail

Repository
rP Phorge
Branch
security/profile-transform
Lint
Lint Warnings
Unit
Tests Passed
Build Status
Buildable 901
Build 901: arc lint + arc unit

Event Timeline

Thanks

Maybe we can mention the upstream commit, and we can amend to set their author information.

I'm trying to understand but I was a bit confused by both changes. I mean, if the first change allows transforms to inherit permissions, why they are still published?

Sorry if this is a stupid question. Need to try :)

@valerio.bozzolan Upstream removed code that enforces $always_visible when $file->getIsProfileImage() is true in PhabricatorFileQuery.php, but I'm afraid that this change may break other things, so I did not apply it here, but on the other hand kept the description of "Image will be Public".

speck added inline comments.
src/applications/files/transform/PhabricatorFileImageTransform.php
153

The order here is important and we might want to call it out in a comment because it might not be clear when reading the code. The behavior of merging arrays this way is that left-most array’s entries are retained vs right-array. So if a property is defined with getFileProperties() but has a different value from inherit or defaults it will still take precedence.

src/applications/files/transform/PhabricatorFileThumbnailTransform.php
61

What do you think of “Public Profile Image (dimensions)”?

src/applications/files/transform/PhabricatorFileImageTransform.php
153

The pattern of + $defaults is quite common in Phorge, and I don't see a comment elsewhere.

src/applications/files/transform/PhabricatorFileThumbnailTransform.php
61

I don't think "Public Profile Image" conveys the idea unambiguously. It could be read as an image transform to "Public Profile" dimensions. Also, other options don't contain the word "Image".

src/applications/files/transform/PhabricatorFileThumbnailTransform.php
61

An image transform that makes an image public irrecoverably is counter-intuitive. I want to capture attention with the extra parentheses and avoid ambiguity as much as possible.

Awesome thanks for adding details and clarification

This revision is now accepted and ready to land.Nov 11 2023, 02:25

Before landing this change, I would like to know the right way to credit both the original author and my modifications with Arcanist (Q85).