Page MenuHomePhorge

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

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:

Revisions and Commits

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.