From e44bbc0caf4636839e535edcc3329b0016aa8f6e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 11 May 2026 10:58:02 -0400 Subject: [PATCH] fix(template-folder): bound cycle-walk to defend against malformed graphs --- .../Services/TemplateFolderService.cs | 6 +++++- .../Services/TemplateFolderServiceTests.cs | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/ScadaLink.TemplateEngine/Services/TemplateFolderService.cs b/src/ScadaLink.TemplateEngine/Services/TemplateFolderService.cs index 3eec81b..5f23938 100644 --- a/src/ScadaLink.TemplateEngine/Services/TemplateFolderService.cs +++ b/src/ScadaLink.TemplateEngine/Services/TemplateFolderService.cs @@ -86,13 +86,17 @@ public class TemplateFolderService return Result.Failure($"Target folder with ID {newParentId.Value} not found."); var all = await _repository.GetAllFoldersAsync(cancellationToken); - // Walk up from newParentId — if we encounter folderId, the move would create a cycle. var byId = all.ToDictionary(f => f.Id); var cursor = newParentId; + // Walk up from newParentId — if we encounter folderId, the move would create a cycle. + // Bound iterations by byId.Count to defensively terminate on a malformed graph. + var iterations = 0; while (cursor.HasValue) { if (cursor.Value == folderId) return Result.Failure("Cannot move a folder under one of its descendants (cycle)."); + if (++iterations > byId.Count) + return Result.Failure("Folder hierarchy contains a cycle; cannot determine ancestry."); cursor = byId.TryGetValue(cursor.Value, out var node) ? node.ParentFolderId : null; } diff --git a/tests/ScadaLink.TemplateEngine.Tests/Services/TemplateFolderServiceTests.cs b/tests/ScadaLink.TemplateEngine.Tests/Services/TemplateFolderServiceTests.cs index 72abb67..cf2f87a 100644 --- a/tests/ScadaLink.TemplateEngine.Tests/Services/TemplateFolderServiceTests.cs +++ b/tests/ScadaLink.TemplateEngine.Tests/Services/TemplateFolderServiceTests.cs @@ -167,4 +167,23 @@ public class TemplateFolderServiceTests Assert.True(result.IsSuccess); Assert.Null(result.Value.ParentFolderId); } + + [Fact] + public async Task MoveFolder_PreExistingCycleInGraph_ReturnsFailure_DoesNotInfiniteLoop() + { + // Manufactured malformed graph: X.parent=Y, Y.parent=X. We move Z under X. + // The ancestor walk would loop forever without a guard. + var x = new TemplateFolder("X") { Id = 1, ParentFolderId = 2 }; + var y = new TemplateFolder("Y") { Id = 2, ParentFolderId = 1 }; + var z = new TemplateFolder("Z") { Id = 3, ParentFolderId = null }; + _repoMock.Setup(r => r.GetFolderByIdAsync(3, It.IsAny())).ReturnsAsync(z); + _repoMock.Setup(r => r.GetFolderByIdAsync(1, It.IsAny())).ReturnsAsync(x); + _repoMock.Setup(r => r.GetAllFoldersAsync(It.IsAny())) + .ReturnsAsync(new List { x, y, z }); + + var result = await _sut.MoveFolderAsync(3, 1, "admin"); + + Assert.True(result.IsFailure); + Assert.Contains("cycle", result.Error, StringComparison.OrdinalIgnoreCase); + } }