InsecureRemarkupGraphvizBlockInterpreter.php

ActivePublic

Actions

I understand this is not supported, thought I would document this scenario I came across.

There is a situation where this will not render for users:

1. Group/Project "Employees"
2. Group/Project "Outside Collaborators"
3. Edit Default Policies for Files application
   1. Set Default View Policy to "Employees"
   2. This means by default files uploaded by "Employees" are not accessible to "Outside Collaborators", but files attached/uploaded to tasks or files uploaded by "Outside Collaborators" can be accessible to "Outside Collaborators"
4. Post a dot graphviz on a Task which is accessible by "Outside Collaborators".
5. "Outside Collaborators" do not have permission to view the generated graphviz

PhabricatorEmbedFileRemarkupRule has the "correct" solution for this problem, via a chain of permissions grants tied together with KEY_EMBED_FILE_PHIDS. This will grant the file the same permissions as the relevant object (e.g., the task or whatever).

A hack would be to add 'viewPolicy' => 'users' or similar, at the cost of leaking secret internal graphs leak to anyone with access to the install if they go fishing around in Files.

There is a problem I have stumbled upon.

It seems that the visual preview creates a new file (and not only a temporary one) each time the graph changes as the user edits in the remarkup window. This is quickly spamming permanent files out which are not referenced at all.

I understand that this feature is not supported anymore, but is there a way to avoid this? It's a great feature for a trusted user base and we would like to use it.

Thanks in advance

@thuffir, I can't come up with an easy fix for that.

@epriestley Thanks for the prompt answer. Isn't there any way to mark these preview files as temporary, or any maintenance task that I could run to remove all graphviz.png files that are not referenced from anywhere?

Sorry -- I'm happy to walk you through the technical details in greater depth at consulting rates (see Consulting) but want to draw a hard line that "not supported" really means "not supported" on this.

@epriestley I see. Thanks anyways. Nevertheless I am hoping that some hints are available at buddy rates :)

I can easily set those files to temporary by setting the ttl field but that ruins the cached display after they get deleted and until the markup cache gets purged.
Can I access some information from the PhutilRemarkupBlockInterpreter Class if the page has been rendered in preview mode or not? I have seen D11832 doing something similar, but I am not sure if I can access any of the PhabricatorMarkupOneOff information from the Block Interpreter class.

For everyone who would like to use this feature (Taking of course the above mentioned security risks into account).

This is my partial (and not very elegant) solution for file spamming I have mentioned in P1853#4928.

--- a/InsecureRemarkupGraphvizBlockInterpreter.php      2016-02-24 08:47:54.517183033 +0000
+++ b/InsecureRemarkupGraphvizBlockInterpreter.php      2016-02-24 08:51:02.784873997 +0000
@@ -41,11 +41,14 @@
           $stderr));
     }

-    $file = PhabricatorFile::buildFromFileDataOrHash(
+    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+    $file = PhabricatorFile::newFromFileData(
       $stdout,
       array(
         'name' => 'graphviz.png',
+        'ttl' => time() + (60 * 60 * 24 * 31),
       ));
+    unset($unguarded);

     if ($this->getEngine()->isTextMode()) {
       return '<'.$file->getBestURI().'>';

The line

'ttl' => time() + (60 * 60 * 24 * 31),

will create a temporary file instead of a permanent one. Adjust the ttl parameter to your cache.markup setting, and make the file last a bit longer (e.g. 31 days vs 30 days with default settings). The proper solution here would be to get the parameter cache.markup from the configuration, but my limited php and phabricator knowledge stands in my way...

Now another problem is, that if the markup gets rendered again after the markup cache has been expired but before the temp file gets deleted, you will get a broken page after the temp file gets actually deleted because buildFromFileDataOrHash() links to an existing file if the content has not been changed. Changing buildFromFileDataOrHash() to newFromFileData() will create an new file (with new ttl value) each time the markup gets rendered.

beginScopedUnguardedWrites() is needed by newFromFileData() (see PhabricatorFile::buildFromFileDataOrHash()).

I hope this helps.

A small update on this topic.
The diff I have submitted above in P1853#4950 is not working anymore since rP45b386596e62.
The corrected patch would be:

--- a/InsecureRemarkupGraphvizBlockInterpreter.php      2016-02-24 08:47:54.517183033 +0000
+++ b/InsecureRemarkupGraphvizBlockInterpreter.php      2016-02-24 08:51:02.784873997 +0000
@@ -41,11 +41,14 @@
           $stderr));
     }

-    $file = PhabricatorFile::buildFromFileDataOrHash(
+    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+    $file = PhabricatorFile::newFromFileData(
       $stdout,
       array(
         'name' => 'graphviz.png',
+        'ttl.relative' => (60 * 60 * 24 * 31),
       ));
+    unset($unguarded);

     if ($this->getEngine()->isTextMode()) {
       return '<'.$file->getBestURI().'>';

As the parameter ttl has been splitted up into ttl.relative and ttl.absolute.