Skip to content

Commit

Permalink
Prevent embedded remarkup content from cycling when it contains embed…
Browse files Browse the repository at this point in the history
…ded self-references

Summary: Ref T13678. When remarkup content embeds other remarkup content, detect and degrade if the references have nesting depth greater than 1. This is a coarse cycle detector, since rendering shallow (but technically non-cycling) trees doesn't seem valuable.

Test Plan: Created various objects with self-references, saw everything degrade properly (after one level of embedding) when embedded in itself and in other contexts. See attached screenshot.

Maniphest Tasks: T13678

Differential Revision: https://secure.phabricator.com/D21810
  • Loading branch information
epriestley committed May 9, 2022
1 parent a640a4a commit 01253d5
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 0 deletions.
20 changes: 20 additions & 0 deletions src/infrastructure/markup/PhabricatorMarkupEngine.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ final class PhabricatorMarkupEngine extends Phobject {
private $engineCaches = array();
private $auxiliaryConfig = array();

private static $engineStack = array();


/* -( Markup Pipeline )---------------------------------------------------- */

Expand Down Expand Up @@ -103,6 +105,24 @@ public function addObject(PhabricatorMarkupInterface $object, $field) {
* @task markup
*/
public function process() {
self::$engineStack[] = $this;

try {
$result = $this->execute();
} finally {
array_pop(self::$engineStack);
}

return $result;
}

public static function isRenderingEmbeddedContent() {
// See T13678. This prevents cycles when rendering embedded content that
// itself has remarkup fields.
return (count(self::$engineStack) > 1);
}

private function execute() {
$keys = array();
foreach ($this->objects as $key => $info) {
if (!isset($info['markup'])) {
Expand Down
12 changes: 12 additions & 0 deletions src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,25 @@ protected function renderObjectEmbedForAnyMedia(
return $this->renderObjectTagForMail($name, $href, $handle);
}

// See T13678. If we're already rendering embedded content, render a
// default reference instead to avoid cycles.
if (PhabricatorMarkupEngine::isRenderingEmbeddedContent()) {
return $this->renderDefaultObjectEmbed($object, $handle);
}

return $this->renderObjectEmbed($object, $handle, $options);
}

protected function renderObjectEmbed(
$object,
PhabricatorObjectHandle $handle,
$options) {
return $this->renderDefaultObjectEmbed($object, $handle);
}

final protected function renderDefaultObjectEmbed(
$object,
PhabricatorObjectHandle $handle) {

$name = $handle->getFullName();
$href = $handle->getURI();
Expand Down

0 comments on commit 01253d5

Please sign in to comment.