Paths

Table of Contentst

Differential D25462

Fix view policy inheritance on image transforms
ClosedPublic
Actions

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

Details

Reviewers

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

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

Diff Detail

Repository

Branch

security/profile-transform

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 912
Build 912: arc lint + arc unit

Event Timeline

• l2dy created this revision.Nov 10 2023, 15:10

Owners added a reviewer: O1: Blessed Committers.Nov 10 2023, 15:10

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptNov 10 2023, 15:10

• l2dy requested review of this revision.Nov 10 2023, 15:10

Harbormaster completed remote builds in B901: Diff 1467.Nov 10 2023, 15:10

Fix lint warnings

Harbormaster completed remote builds in B902: Diff 1468.Nov 10 2023, 15:13

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 :)

Mention upstream commit

Harbormaster completed remote builds in B903: Diff 1469.Nov 10 2023, 15:56

@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 subscribed.Nov 11 2023, 00:03

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)”?

• l2dy added inline comments.Nov 11 2023, 00:27

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".

• l2dy added inline comments.Nov 11 2023, 00:34

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).

• l2dy marked 2 inline comments as done.Nov 11 2023, 03:57

• l2dy mentioned this in Q83: Pull security fixes from Mozilla's fork (Answer 108).Nov 11 2023, 08:25

valerio.bozzolan edited the summary of this revision. (Show Details)Nov 11 2023, 22:09

Update commit message

Harbormaster completed remote builds in B912: Diff 1480.Nov 11 2023, 23:07

Closed by commit rP005fea5a14f4: Fix view policy inheritance on image transforms. · Explain WhyNov 11 2023, 23:10

This revision was automatically updated to reflect the committed changes.

• l2dy added a commit: rP005fea5a14f4: Fix view policy inheritance on image transforms.

Revision Contents
Changeset List

Path

Size

Packages

src/

applications/

files/

transform/

PhabricatorFileImageTransform.php

6 lines

O1: Blessed Committers

PhabricatorFileThumbnailTransform.php

2 lines

O1: Blessed Committers

Diff 1480

src/applications/files/transform/PhabricatorFileImageTransform.php

Loading...

src/applications/files/transform/PhabricatorFileThumbnailTransform.php

Loading...