Prevent template circular references (#15907)

* Added a simpel circular reference check to templates

* Upgraded detecting to all levels

* Rename method and add comment to clarify why we have 2 similar methods

* Apply suggestions from code review

Co-authored-by: Nikolaj Geisle <[email protected]>

---------

Co-authored-by: Sven Geusens <[email protected]>
Co-authored-by: Nikolaj Geisle <[email protected]>

Author: Migaroez

src/Umbraco.Cms.Api.Management/Controllers/Template/TemplateControllerBase.cs

@@ -30,6 +30,10 @@ protected IActionResult TemplateOperationStatusResult(TemplateOperationStatus st
                 .WithTitle("Duplicate alias")
                 .WithDetail("A template with that alias already exists.")
                 .Build()),
+            TemplateOperationStatus.CircularMasterTemplateReference => BadRequest(problemDetailsBuilder
+                .WithTitle("Invalid master template")
+                .WithDetail("The master template referenced in the template leads to a circular reference.")
+                .Build()),
             _ => StatusCode(StatusCodes.Status500InternalServerError, problemDetailsBuilder
                 .WithTitle("Unknown template operation status.")
                 .Build()),

src/Umbraco.Core/Services/OperationStatus/TemplateOperationStatus.cs

@@ -7,5 +7,6 @@ public enum TemplateOperationStatus
     InvalidAlias,
     DuplicateAlias,
     TemplateNotFound,
-    MasterTemplateNotFound
+    MasterTemplateNotFound,
+    CircularMasterTemplateReference
 }

src/Umbraco.Core/Services/TemplateService.cs

@@ -242,6 +242,14 @@ private async Task<Attempt<ITemplate, TemplateOperationStatus>> SaveAsync(ITempl
                 return Attempt.FailWithStatus(TemplateOperationStatus.MasterTemplateNotFound, template);
             }
 
+            // detect circular references
+            if (masterTemplateAlias is not null
+                && masterTemplate is not null
+                && await HasCircularReference(masterTemplateAlias, template, masterTemplate))
+            {
+                return Attempt.FailWithStatus(TemplateOperationStatus.CircularMasterTemplateReference, template);
+            }
+
             await SetMasterTemplateAsync(template, masterTemplate, userKey);
 
             EventMessages eventMessages = EventMessagesFactory.Get();
@@ -413,4 +421,42 @@ private void Audit(AuditType type, int userId, int objectId, string? entityType)
 
     private static bool IsValidAlias(string alias)
         => alias.IsNullOrWhiteSpace() == false && alias.Length <= 255;
+
+    private async Task<bool> HasCircularReference(string parsedMasterTemplateAlias, ITemplate template, ITemplate masterTemplate)
+    {
+        // quick check without extra DB calls as we already have both templates
+        if (parsedMasterTemplateAlias.IsNullOrWhiteSpace() is false
+            && masterTemplate.MasterTemplateAlias is not null
+            && masterTemplate.MasterTemplateAlias.Equals(template.Alias))
+        {
+            return true;
+        }
+
+        var processedTemplates = new List<ITemplate> { template, masterTemplate };
+        return await HasRecursiveCircularReference(processedTemplates, masterTemplate.MasterTemplateAlias);
+    }
+
+    private async Task<bool> HasRecursiveCircularReference(List<ITemplate> referencedTemplates, string? masterTemplateAlias)
+    {
+        if (masterTemplateAlias is null)
+        {
+            return false;
+        }
+
+        if (referencedTemplates.Any(template => template.Alias.Equals(masterTemplateAlias)))
+        {
+            return true;
+        }
+
+        ITemplate? masterTemplate = await GetAsync(masterTemplateAlias);
+        if (masterTemplate is null)
+        {
+            // this should not happen unless somebody manipulated the data by hand as this function is only called between persisted items
+            return false;
+        }
+
+        referencedTemplates.Add(masterTemplate);
+
+        return await HasRecursiveCircularReference(referencedTemplates, masterTemplate.MasterTemplateAlias);
+    }
 }