From 01253d533bbca72e179b1efb64d97e4d91c988e4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 9 May 2022 11:18:13 -0700
Subject: [PATCH] Prevent embedded remarkup content from cycling when it
 contains embedded 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
---
 .../markup/PhabricatorMarkupEngine.php        | 20 +++++++++++++++++++
 .../rule/PhabricatorObjectRemarkupRule.php    | 12 +++++++++++
 2 files changed, 32 insertions(+)

diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php
index 26851abd5f..d897314245 100644
--- a/src/infrastructure/markup/PhabricatorMarkupEngine.php
+++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php
@@ -46,6 +46,8 @@ final class PhabricatorMarkupEngine extends Phobject {
   private $engineCaches = array();
   private $auxiliaryConfig = array();
 
+  private static $engineStack = array();
+
 
 /* -(  Markup Pipeline  )---------------------------------------------------- */
 
@@ -103,6 +105,24 @@ final class PhabricatorMarkupEngine extends Phobject {
    * @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'])) {
diff --git a/src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php b/src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php
index 7fb997b2c5..5ab033d6b1 100644
--- a/src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php
+++ b/src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php
@@ -126,6 +126,12 @@ abstract class PhabricatorObjectRemarkupRule extends PhutilRemarkupRule {
       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);
   }
 
@@ -133,6 +139,12 @@ abstract class PhabricatorObjectRemarkupRule extends PhutilRemarkupRule {
     $object,
     PhabricatorObjectHandle $handle,
     $options) {
+    return $this->renderDefaultObjectEmbed($object, $handle);
+  }
+
+  final protected function renderDefaultObjectEmbed(
+    $object,
+    PhabricatorObjectHandle $handle) {
 
     $name = $handle->getFullName();
     $href = $handle->getURI();