Page MenuHomePhorge

"Upload file" in remarkup text fields should attach by default
Open, HighPublic

Description

Our local Phabricator install was recently impacted by the upstream 2022 Week 21 (Late May) update, which mitigated a permission escalation issue with file references.

The security guidance task mentions some limitations in passing:

Some reasonable cases where Phabricator should be able to determine that an attachment is safe (e.g., using the "Upload File" dialog, and some unmodernized interfaces in various applicatinos) are not yet automatically identified as safe and must be manually attached. See T13682 for some discussion of future work.

It appears that some workflows do indeed work as they did before, i.e. they both upload the file and attach it to the parent object, granting implicit visibility of the file to anyone that can see the parent object. From experiments on our local install, I've assembled the following truth table:

WhereUpload to files application and reference {Fxxxx}"Upload File" in text boxPaste into text boxDrag & drop into text box
Task descriptionYesNoYesYes
Task commentYesNoYesYes
Revision summary from arcYesN/AN/AN/A
Revision test plan from arcYesN/AN/AN/A
Revision summary from webYesNoYesYes
Revision test plan from webYesNoYesYes
Revision commentYesNoYesYes
NOTE: The Upload to files application and reference {Fxxxx} used the defaults at https://phabricator.internal.encircleapp.com/file/upload/ where Visible To was set to All Users.

In particular, the "Upload File" in text box workflow seems to be glaringly broken. I have no idea if upstream plans to fix this as a charity or whether this'll have to be fixed through the community (and maybe contributed back upstream if epriestley accepts).


Similar discussions downstream:

Event Timeline

kwisatz subscribed.

@avivey what would be required to merge @Dylsss 's patch into phorge stable?
(This thing has been annoying our team for the past few weeks)

Auto file attachments are also not working when editing a comment.

@kwisatz I'll try to review the diff this week, and that will probably get to master/stable soon...

For completeness: The security breaches mentioned in the security guidance task are all about exposing an existing file that the attacker has new view access to; This flow is only relevant to explicitly uploading a new file, so it doesn't have the same security implications.

valerio.bozzolan triaged this task as Normal priority.EditedApr 20 2023, 13:55

I start triaging this to Normal priority since this problem creates situations that don't happen with many proprietary weird alternatives like Trello, Asana etc., where their Drag & Drop "just works" since their file are always visible to all members and so it's considered as "working" for their end-users (well, thanks to the fact that Trello etc. have a terrible permission management system, thousand years back from Phorge, and they really have no concept of "single file permission", and so they win easy in these confrontations where the user expects something basic and it doesn't happen as default by some of our "attach to something" corner cases).

Another use case: Drag & Drop on a new Object description, even if the Object is not already created, should attach by default to it.

I had communicated these upstream (almost exactly a year ago~) and some helpful information was provided

@speck wrote:

Ref: https://secure.phabricator.com/T13683

Per that description filing these notices here.

A user ran into a case where pasting an image from system clipboard into a text field (specifically an inline comment) would not attach the file to the object as expected. I have not yet tested this (did not realize images in clipboard could be pasted into fields).

Additionally the phorge project has had a few related issues reported

  1. Deleting files leaves leftover "unknown object" references (on diff revisions)
  2. Deleting a file object via ./bin/remove destroy results in an exception due to a missing edge

If you prefer I can post this on https://secure.phabricator.com/T13682 instead


epriestley wrote:

Thanks -- I think these are known elsewhere (the inline comment thing is just "inline comment text never attaches files", and probably not specific to pasting images), I just haven't had a chance to collect them in https://secure.phabricator.com/T13683 and fix them yet since the amount of software development I'm doing these days rounds down to zero. My plan is:

  1. Deleting a file should delete all corresponding PhabricatorFileAttachment objects.
  2. When an object is destroyed, attached edges should be destroyed even if their edge type is unknown, perhaps with a console warning.
  3. Inline comments should attach files.

...but no clue when I'll get there.


(1) is probably simple and straightforward -- a patch using PhabricatorFileAttachmentQuery, following the PhabricatorQueryIterator pattern in DifferentialRevision->destroyObjectPermanently(), seems likely reasonable to me.

(2) may be tricky since I think the code raising the error is far-removed from the knowledge that a deletion is occurring. Alternate but less-pure remedies are "restore stub edge type definitions for the removed edges" or "migrate to destroy all those edges", but I think those are a fairly big step down from "don't fail when destroying unknown edge types".

(3) is probably moderately complicated, I think the patch has several pieces and probably doesn't closely resemble other file attachment patches.


epriestley wrote:

...perhaps with a console warning.

While reviewing this, one note mostly for my own sake: although the current console behavior says "exception", it does continue and actually destroy the object (but not the outdated edge).


I noted the rest of this in T13682.

This feature is not complete also for Conpherence AFAIK

avivey raised the priority of this task from Normal to High.Apr 11 2024, 10:41

Minor clarification.

I've seen this phrase in a unit test:

// Reference the file in a comment. This should not affect the file
// policy.

https://we.phorge.it/source/phorge/browse/master/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php;f75c7ce7664b1baa23463a3f3fa62cbb671687d2$196-197

I'm unsure about the purpose of this.