Page MenuHomePhabricator

Migrate Project image to modular transactions
ClosedPublic

Authored by amckinley on May 18 2017, 8:06 PM.
Tags
None
Referenced Files
F13480892: D17954.diff
Fri, Jul 19, 9:24 AM
F13462656: D17954.diff
Mon, Jul 15, 2:19 PM
F13431836: D17954.diff
Wed, Jul 10, 9:15 AM
F13408774: D17954.diff
Sat, Jul 6, 5:57 AM
F13394056: D17954.id43184.diff
Wed, Jul 3, 4:43 AM
F13387407: D17954.diff
Mon, Jul 1, 7:09 AM
F13372618: D17954.id43187.diff
Fri, Jun 28, 12:34 PM
F13363764: D17954.diff
Wed, Jun 26, 4:48 AM
Subscribers

Details

Summary

I'm not sure you can actually remove a project's image (maybe via the API?), but I kept the code for rendering the relevant title/feed anyway.

Test Plan

Unit tests + adding/changing project pictures.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

src/applications/project/storage/PhabricatorProjectTransaction.php
48

I'm pretty sure this isn't needed anymore, but I could be wrong.

I think you're right that there's currently no legitimate way to remove the image.

This looks good, but while we're here, let's add file validation -- I think it's currently impossible to hit this transaction with the API, but it's likely to become possible sooner or later, and there's some possibility that we'll open up security holes if we aren't careful (the attack is: edit a form, add a file PHID which you don't have permission to see as the project's image, submit the form, now you can see the file because it became "attached" to the project). PhabricatorMacroFileTransaction has validation you can reference. We don't need to do the "must have a file" bit, but should make sure the file is valid, that the viewer can see the file, and that the file is a viewable image.

An indirect attack this also defuses is:

  • Knowing that privileged user Alice routinely looks at project X;
  • set the profile picture of project X to an image containing nuclear launch codes which you don't have permission to see;
  • using your spyglass, observe Alice's screen;
  • by surprise, she reveals the nuclear launch codes to you during routine operations. Because of her surprise, she doesn't have time to wrap he arms around her monitor like she usually does when looking at nuclear launch codes.

This is obscure nonsense, but the "make sure the viewer can see the file" check prevents anything in this vein from happening.

src/applications/project/storage/PhabricatorProjectTransaction.php
48

Should be good to nuke.

src/applications/project/xaction/PhabricatorProjectImageTransaction.php
73–74

Probably impossible to hit, but we should return array() if there's no image (instead of implicitly returning null).

This revision now requires changes to proceed.May 18 2017, 8:48 PM

Any suggestions on how to test the file validation? I went and edited PhabricatorProjectEditPictureController to hardcode the PHID of a restricted image into setNewValue(). That's throwing a "You Shall Not Pass" permissions errors, but it's not coming from my validateTransactions() code, so it looks like something is kicking me out before we get far enough through applyTransactions() to trigger my validation code.

Hmm, show me the code? I have no clue what's doing the extra validation offhand.

It's possible that extractFilePHIDs() runs before validation, in which case the extra validation might effectively be untestable today, but I think writing a test is still worthwhile to make sure we don't break the validation after changes like T11035 (which may reorder some of the transaction phases), even if it doesn't actually test new validation code.

Hmm, show me the code? I have no clue what's doing the extra validation offhand.

Here's my janky patch: https://secure.phabricator.com/P2052 (Substitute that PHID for one that exists but is restricted on your instance). Then edit a project picture (I was using the drag and drop, but the quick icon thing should work too).

It's possible that extractFilePHIDs() runs before validation, in which case the extra validation might effectively be untestable today, but I think writing a test is still worthwhile to make sure we don't break the validation after changes like T11035 (which may reorder some of the transaction phases), even if it doesn't actually test new validation code.

For giggles, I tried commenting-out extractFilePHIDs() but that didn't change what I was seeing.

Perfect, thanks. Let me see if I can sort out what's happening...

Oh, from the Project slugs thing, I get this under PHP7:

>>> UNRECOVERABLE FATAL ERROR <<<

&#039;break&#039; not in the &#039;loop&#039; or &#039;switch&#039; context

/Users/epriestley/dev/core/lib/phabricator/src/applications/project/xaction/PhabricatorProjectSlugsTransaction.php:64


┻━┻ ︵ ¯\_(ツ)_/¯ ︵ ┻━┻

That's because a break; got left around at the bottom of PhabricatorProjectSlugsTransaction->getTitle().

(I have never seen this error before.)

themoreyouknow

amckinley edited edge metadata.
  • add validation of project image viewability, remove accidental dangling break statement

I think there are a couple of subtle things going wrong here:

If I hard-code an image I can't see, the file attaches successfully, then redirects to /project/view/<id>/. Then, that page throws a policy error while trying to load the project because I can't see the project's profile image. This error is arising from the PhabricatorFileQuery issued by PhabricatorProjectQuery->didFilterPage(). I got a stack trace by just try/catch'ing all of PhabricatorProjectViewController->handleRequest() and phlog(...)'ing the exception:

[Thu May 18 14:49:38.784002 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496] [2017-05-18 16:49:38] EXCEPTION: (PhabricatorPolicyException) [You Shall Not Pass: Restricted File] (Can View) You do not have permission to view this object. // By default, no one can take this action. The user who uploaded a file can always view and edit it. Files attached to objects are visible to users who can view those objects. Thumbnails are visible only to users who can view the original file. at [<phabricator>/src/applications/policy/filter/PhabricatorPolicyFilter.php:640]
[Thu May 18 14:49:38.784630 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496] arcanist(head=experimental, ref.master=3c4735795a29, ref.experimental=dc65bfbe5434), corgi(head=master, ref.master=c732aa7b3fef), instances(head=master, ref.master=74b660f19aa0), ledger(head=master, ref.master=4da4a24b8779), libcore(), phabricator(head=master, ref.master=90c562fe572e, custom=1), phutil(head=master, ref.master=a900d7b63e95), secure(head=master, ref.master=fa7bd57d338a), services(head=image1, ref.master=99e27a5eed75, ref.image1=bad018b269c9)
[Thu May 18 14:49:38.784641 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #0 <#2> PhabricatorPolicyFilter::rejectObject(PhabricatorFile, string, string) called at [<phabricator>/src/applications/policy/filter/PhabricatorPolicyFilter.php:543]
[Thu May 18 14:49:38.784644 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #1 <#2> PhabricatorPolicyFilter::checkCapability(PhabricatorFile, string) called at [<phabricator>/src/applications/policy/filter/PhabricatorPolicyFilter.php:250]
[Thu May 18 14:49:38.784647 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #2 <#2> PhabricatorPolicyFilter::apply(array) called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:258]
[Thu May 18 14:49:38.784649 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #3 <#2> PhabricatorPolicyAwareQuery::execute() called at [<phabricator>/src/applications/project/query/PhabricatorProjectQuery.php:370]
[Thu May 18 14:49:38.784652 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #4 <#2> PhabricatorProjectQuery::didFilterPage(array) called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:273]
[Thu May 18 14:49:38.784654 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #5 <#2> PhabricatorPolicyAwareQuery::execute() called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:168]
[Thu May 18 14:49:38.784658 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #6 <#2> PhabricatorPolicyAwareQuery::executeOne() called at [<phabricator>/src/applications/project/controller/PhabricatorProjectController.php:51]
[Thu May 18 14:49:38.784661 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #7 <#2> PhabricatorProjectController::loadProject() called at [<phabricator>/src/applications/project/controller/PhabricatorProjectViewController.php:15]
[Thu May 18 14:49:38.784663 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #8 phlog(PhabricatorPolicyException) called at [<phabricator>/src/applications/project/controller/PhabricatorProjectViewController.php:37]
[Thu May 18 14:49:38.784666 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #9 PhabricatorProjectViewController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:268]
[Thu May 18 14:49:38.784668 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #10 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:177]
[Thu May 18 14:49:38.784671 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #11 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:17]

(Possibly, in development mode, we should make silent exceptions go to the log anyway, although I think this sort of thing is fairly rare.)

You can verify that the edit actually went through by looking at the project row in the database -- I hard-coded PHID-FILE-xhvuatzxmc73hcmmz3we, and SELECT * FROM phabricator_project.project WHERE id = 38 now shows that project 38 has that as the profileImagePHID.

This shouldn't be allowed, and validation logic should be able to prevent it.


Second, we're actually missing some logic to attach the file to the project. Repro for this should be:

  • As user A, upload a file which only you can see.
  • Set it as the profile for public project B.
  • Other users won't be able to see the project any more.

The fix for this is to call $file->attachToObject($project->getPHID()) when the file is set as the profile picture. This lets anyone who can see the project see the file.

You can double-check this on /F123 on the "Attached" tab.

But: note that we also generate a thumbnail. The thumbnail does get attached. So if you set F123 as the profile image, then load the page, we'll generate F124: a thumbnail of F123 which is properly attached to the project. In practice, this means it's very difficult to actually hit any issues because of this bug, since you'd normally never actually go look at the original F123.

PhabricatorMacroFileTransaction->applyExternalEffects() has some reference logic for doing attachToObject() properly. Possibly, we should move that up the stack somewhere and call the same (or similar) logic from both places since I think both transactions are doing roughly the same thing.

Validation looks good, just bring over the attachToObject() stuff and I think we're all set.

validation log

"log" is the hip way the kids are saying "logic" these days

If I hard-code an image I can't see, the file attaches successfully, then redirects to /project/view/<id>/. Then, that page throws a policy error while trying to load the project because I can't see the project's profile image.

Oh, that's interesting. Are you sure that's how it works? When I attempt to edit the project image to an unviewable image, I get redirected to a page like this: http://local.phacility.com/project/picture/1/

That page throws a permissions error, but if I examine the DB, there's no row for the transaction object, the original PHID is still listed on the projects table, and refreshing the project page (http://local.phacility.com/project/profile/1/) shows the original, viewable project image. Did you also arc patch my migration? I wonder if the modular transactions framework does something clever that prevents the transaction from going through.

  • add attachToObject logic to prevent subtle permissions issues with project images

That's how it seems to be working for me, at least. Here's what I did:

  • I have the earlier version of this change patched locally, prior to validateTransactions().
$ git show
commit 90c562fe572e50a77140fdb5badd892deed24245
Author: Austin McKinley <austin@phacility.com>
Date:   Thu May 18 14:24:47 2017 -0700

    Migrate Project image to modular transactions
  • I set the Files default view permission to "No One":

Screen Shot 2017-05-18 at 3.27.37 PM.png (1×1 px, 205 KB)

  • As user @dog, I uploaded a file by drag-and-dropping it onto the home page. Note permissions are "No One":

Screen Shot 2017-05-18 at 3.28.36 PM.png (1×1 px, 612 KB)

  • I grabbed the PHID of the file from the database:
mysql> SELECT phid FROM local_file.file WHERE id = 29055;
+--------------------------------+
| phid                           |
+--------------------------------+
| PHID-FILE-dokbmtmdwwr655muykvr |
+--------------------------------+
1 row in set (0.00 sec)
  • I hard-coded that PHID into PhabricatorProjectEditPictureController:

Screen Shot 2017-05-18 at 3.30.09 PM.png (180×597 px, 49 KB)

  • As user @hector, I went to the picture edit UI for project ID 123, picked a file with "Choose File", and clicked Upload Picture:

Screen Shot 2017-05-18 at 3.30.41 PM.png (1×1 px, 138 KB)

  • Then I end up on /project/profile/123/ (I might have mistranscribed this as /view/123/ earlier, I think?) with that page raising the policy error:

Screen Shot 2017-05-18 at 3.32.29 PM.png (1×1 px, 136 KB)

  • ...and the project has the no-permissions PHID in the database:
mysql> SELECT * FROM local_project.project WHERE id = 123\G
*************************** 1. row ***************************
                id: 123
              name: Elite Papercraft
              phid: PHID-PROJ-qmzlmz2cek67xw4mjimt
        authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
       dateCreated: 1454551863
      dateModified: 1495146684
            status: 0
        viewPolicy: users
        editPolicy: obj.project.members
        joinPolicy: users
isMembershipLocked: 0
  profileImagePHID: PHID-FILE-dokbmtmdwwr655muykvr
              icon: project
             color: blue
           mailKey: l6ca7j6queygcxz2aiay
       primarySlug: elite_papercraft
 parentProjectPHID: NULL
      hasWorkboard: 1
     hasMilestones: 1
    hasSubprojects: 0
   milestoneNumber: NULL
       projectPath: xDOK
      projectDepth: 0
    projectPathKey: xDOK
        properties: []
1 row in set (0.00 sec)

Oh, I get the /project/picture/xxx/ error (which is correct) with the validation -- maybe you were just testing post-validation and I was testing pre-validation? My comments have been a little out of sequence here since I had to take a quick IRL meeting about whether our pup is adorable or not.

With the current version, I'm no longer able to find a way to break any of this locally.

This revision is now accepted and ready to land.May 18 2017, 10:38 PM
This revision was automatically updated to reflect the committed changes.