Improve area/iteration path validation (#3489)

* Add more comprehensive checks and better error messages to area/iteration path validation

* Join invalid chars with space instead of comma

* Make tree path validation more testable

* Add error code for invalid ADO project in config

* Write unit tests for tree path validation

* Format tree path unit tests

* Merge escape character and control character checks and clarify error message

Author: kananb

src/ApiService/ApiService/OneFuzzTypes/Enums.cs

@@ -49,6 +49,7 @@ public enum ErrorCode {
     ADO_VALIDATION_MISSING_PAT_SCOPES = 492,
     ADO_WORKITEM_PROCESSING_DISABLED = 494,
     ADO_VALIDATION_INVALID_PATH = 495,
+    ADO_VALIDATION_INVALID_PROJECT = 496,
     // NB: if you update this enum, also update enums.py
 }
 

src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs

@@ -89,30 +89,97 @@ private static bool IsTransient(Exception e) {
         return errorCodes.Any(errorStr.Contains);
     }
 
-    private static async Async.Task<OneFuzzResultVoid> ValidatePath(string project, string path, TreeStructureGroup structureGroup, WorkItemTrackingHttpClient client) {
-        var pathType = (structureGroup == TreeStructureGroup.Areas) ? "Area" : "Iteration";
-        var pathParts = path.Split('\\');
-        if (!string.Equals(pathParts[0], project, StringComparison.OrdinalIgnoreCase)) {
+    public static OneFuzzResultVoid ValidateTreePath(IEnumerable<string> path, WorkItemClassificationNode? root) {
+        if (root is null) {
+            return OneFuzzResultVoid.Error(ErrorCode.ADO_VALIDATION_INVALID_PROJECT, new string[] {
+                $"Path \"{string.Join('\\', path)}\" is invalid. The specified ADO project doesn't exist.",
+                "Double check the 'project' field in your ADO config.",
+            });
+        }
+
+        string treeNodeTypeName;
+        switch (root.StructureType) {
+            case TreeNodeStructureType.Area:
+                treeNodeTypeName = "Area";
+                break;
+            case TreeNodeStructureType.Iteration:
+                treeNodeTypeName = "Iteration";
+                break;
+            default:
+                return OneFuzzResultVoid.Error(ErrorCode.ADO_VALIDATION_INVALID_PATH, new string[] {
+                    $"Path root \"{root.Name}\" is an unsupported type. Expected Area or Iteration but got {root.StructureType}.",
+                });
+        }
+
+        // Validate path based on
+        // https://learn.microsoft.com/en-us/azure/devops/organizations/settings/about-areas-iterations?view=azure-devops#naming-restrictions
+        var maxNodeLength = 255;
+        var maxDepth = 13;
+        // Invalid characters from the link above plus the escape sequences (since they have backslashes and produce confusingly formatted errors if not caught here)
+        var invalidChars = new char[] { '/', ':', '*', '?', '"', '<', '>', '|', ';', '#', '$', '*', '{', '}', ',', '+', '=', '[', ']' };
+
+        // Ensure that none of the path parts are too long
+        var erroneous = path.FirstOrDefault(part => part.Length > maxNodeLength);
+        if (erroneous != null) {
+            return OneFuzzResultVoid.Error(ErrorCode.ADO_VALIDATION_INVALID_PATH, new string[] {
+                $"{treeNodeTypeName} Path \"{string.Join('\\', path)}\" is invalid. \"{erroneous}\" is too long. It must be less than {maxNodeLength} characters.",
+                "Learn more about naming restrictions here: https://learn.microsoft.com/en-us/azure/devops/organizations/settings/about-areas-iterations?view=azure-devops#naming-restrictions"
+            });
+        }
+
+        // Ensure that none of the path parts contain invalid characters
+        erroneous = path.FirstOrDefault(part => invalidChars.Any(part.Contains));
+        if (erroneous != null) {
             return OneFuzzResultVoid.Error(ErrorCode.ADO_VALIDATION_INVALID_PATH, new string[] {
-                $"Path \"{path}\" is invalid. It must start with the project name, \"{project}\".",
-                $"Example: \"{project}\\{path}\".",
+                $"{treeNodeTypeName} Path \"{string.Join('\\', path)}\" is invalid. \"{erroneous}\" contains an invalid character ({string.Join(" ", invalidChars)}).",
+                "Make sure that the path is separated by backslashes (\\) and not forward slashes (/).",
+                "Learn more about naming restrictions here: https://learn.microsoft.com/en-us/azure/devops/organizations/settings/about-areas-iterations?view=azure-devops#naming-restrictions"
             });
         }
 
-        var current = await client.GetClassificationNodeAsync(project, structureGroup, depth: pathParts.Length - 1);
-        if (current == null) {
+        // Ensure no unicode control characters
+        erroneous = path.FirstOrDefault(part => part.Any(ch => char.IsControl(ch)));
+        if (erroneous != null) {
             return OneFuzzResultVoid.Error(ErrorCode.ADO_VALIDATION_INVALID_PATH, new string[] {
-                $"{pathType} Path \"{path}\" is invalid. \"{project}\" is not a valid project.",
+                // More about control codes and their range here: https://en.wikipedia.org/wiki/Unicode_control_characters
+                $"{treeNodeTypeName} Path \"{string.Join('\\', path)}\" is invalid. \"{erroneous}\" contains a unicode control character (\\u0000 - \\u001F or \\u007F - \\u009F).",
+                "Make sure that you're path doesn't contain any escape characters (\\0 \\a \\b \\f \\n \\r \\t \\v).",
+                "Learn more about naming restrictions here: https://learn.microsoft.com/en-us/azure/devops/organizations/settings/about-areas-iterations?view=azure-devops#naming-restrictions"
             });
         }
 
-        foreach (var part in pathParts.Skip(1)) {
+        // Ensure that there aren't too many path parts
+        if (path.Count() > maxDepth) {
+            return OneFuzzResultVoid.Error(ErrorCode.ADO_VALIDATION_INVALID_PATH, new string[] {
+                $"{treeNodeTypeName} Path \"{string.Join('\\', path)}\" is invalid. It must be less than {maxDepth} levels deep.",
+                "Learn more about naming restrictions here: https://learn.microsoft.com/en-us/azure/devops/organizations/settings/about-areas-iterations?view=azure-devops#naming-restrictions"
+            });
+        }
+
+
+        // Path should always start with the project name ADO expects an absolute path
+        if (!string.Equals(path.First(), root.Name, StringComparison.OrdinalIgnoreCase)) {
+            return OneFuzzResultVoid.Error(ErrorCode.ADO_VALIDATION_INVALID_PATH, new string[] {
+                $"{treeNodeTypeName} Path \"{string.Join('\\', path)}\" is invalid. It must start with the project name, \"{root.Name}\".",
+                $"Example: \"{root.Name}\\{path}\".",
+            });
+        }
+
+        // Validate that each part of the path is a valid child of the previous part
+        var current = root;
+        foreach (var part in path.Skip(1)) {
             var child = current.Children?.FirstOrDefault(x => string.Equals(x.Name, part, StringComparison.OrdinalIgnoreCase));
             if (child == null) {
-                return OneFuzzResultVoid.Error(ErrorCode.ADO_VALIDATION_INVALID_PATH, new string[] {
-                    $"{pathType} Path \"{path}\" is invalid. \"{part}\" is not a valid child of \"{current.Name}\".",
-                    $"Valid children of \"{current.Name}\" are: [{string.Join(',', current.Children?.Select(x => $"\"{x.Name}\"") ?? new List<string>())}].",
-                });
+                if (current.Children is null || !current.Children.Any()) {
+                    return OneFuzzResultVoid.Error(ErrorCode.ADO_VALIDATION_INVALID_PATH, new string[] {
+                        $"{treeNodeTypeName} Path \"{string.Join('\\', path)}\" is invalid. \"{current.Name}\" has no children.",
+                    });
+                } else {
+                    return OneFuzzResultVoid.Error(ErrorCode.ADO_VALIDATION_INVALID_PATH, new string[] {
+                        $"{treeNodeTypeName} Path \"{string.Join('\\', path)}\" is invalid. \"{part}\" is not a valid child of \"{current.Name}\".",
+                        $"Valid children of \"{current.Name}\" are: [{string.Join(',', current.Children?.Select(x => $"\"{x.Name}\"") ?? new List<string>())}].",
+                    });
+                }
             }
 
             current = child;
@@ -195,14 +262,19 @@ await policy.ExecuteAsync(async () => {
 
         try {
             // Validate AreaPath and IterationPath exist
+            // This also validates that the config.Project exists
             if (config.AdoFields.TryGetValue("System.AreaPath", out var areaPathString)) {
-                var validateAreaPath = await ValidatePath(config.Project, areaPathString, TreeStructureGroup.Areas, witClient);
+                var path = areaPathString.Split('\\');
+                var root = await witClient.GetClassificationNodeAsync(config.Project, TreeStructureGroup.Areas, depth: path.Length - 1);
+                var validateAreaPath = ValidateTreePath(path, root);
                 if (!validateAreaPath.IsOk) {
                     return validateAreaPath;
                 }
             }
             if (config.AdoFields.TryGetValue("System.IterationPath", out var iterationPathString)) {
-                var validateIterationPath = await ValidatePath(config.Project, iterationPathString, TreeStructureGroup.Iterations, witClient);
+                var path = iterationPathString.Split('\\');
+                var root = await witClient.GetClassificationNodeAsync(config.Project, TreeStructureGroup.Iterations, depth: path.Length - 1);
+                var validateIterationPath = ValidateTreePath(path, root);
                 if (!validateIterationPath.IsOk) {
                     return validateIterationPath;
                 }

src/ApiService/Tests/TreePathTests.cs

@@ -0,0 +1,148 @@
+using System.Collections.Generic;
+using System.Linq;
+using Microsoft.OneFuzz.Service;
+using Microsoft.TeamFoundation.WorkItemTracking.WebApi.Models;
+using Xunit;
+
+namespace Tests;
+
+// This might be a good candidate for property based testing
+// https://fscheck.github.io/FsCheck//QuickStart.html
+public class TreePathTests {
+    private static IEnumerable<string> SplitPath(string path) {
+        return path.Split('\\');
+    }
+
+    private static WorkItemClassificationNode MockTreeNode(IEnumerable<string> path, TreeNodeStructureType structureType) {
+        var root = new WorkItemClassificationNode() {
+            Name = path.First(),
+            StructureType = structureType
+        };
+
+        var current = root;
+        foreach (var segment in path.Skip(1)) {
+            var child = new WorkItemClassificationNode {
+                Name = segment
+            };
+            current.Children = new[] { child };
+            current = child;
+        }
+
+        return root;
+    }
+
+
+    [Fact]
+    public void TestValidPath() {
+        var path = SplitPath(@"project\foo\bar\baz");
+        var root = MockTreeNode(path, TreeNodeStructureType.Area);
+
+        var result = Ado.ValidateTreePath(path, root);
+
+        Assert.True(result.IsOk);
+    }
+
+    [Fact]
+    public void TestNullTreeNode() { // A null tree node indicates an invalid ADO project was used in the query
+        var path = SplitPath(@"project\foo\bar\baz");
+
+        var result = Ado.ValidateTreePath(path, null);
+
+        Assert.False(result.IsOk);
+        Assert.Equal(ErrorCode.ADO_VALIDATION_INVALID_PROJECT, result.ErrorV!.Code);
+        Assert.Contains("ADO project doesn't exist", result.ErrorV!.Errors![0]);
+    }
+
+    [Fact]
+    public void TestPathPartTooLong() {
+        var path = SplitPath(@"project\foo\barbazquxquuxcorgegraultgarplywaldofredplughxyzzythudbarbazquxquuxcorgegraultgarplywaldofredplughxyzzythudbarbazquxquuxcorgegraultgarplywaldofredplughxyzzythudbarbazquxquuxcorgegraultgarplywaldofredplughxyzzythudbarbazquxquuxcorgegraultgarplywaldofredplughxyzzythud\baz");
+        var root = MockTreeNode(path, TreeNodeStructureType.Iteration);
+
+        var result = Ado.ValidateTreePath(path, root);
+
+        Assert.False(result.IsOk);
+        Assert.Equal(ErrorCode.ADO_VALIDATION_INVALID_PATH, result.ErrorV!.Code);
+        Assert.Contains("too long", result.ErrorV!.Errors![0]);
+    }
+
+    [Theory]
+    [InlineData("project/foo/bar/baz")]
+    [InlineData("project\\foo:\\bar\\baz")]
+    public void TestPathContainsInvalidChar(string invalidPath) {
+        var path = SplitPath(invalidPath);
+        var treePath = SplitPath(@"project\foo\bar\baz");
+        var root = MockTreeNode(treePath, TreeNodeStructureType.Area);
+
+        var result = Ado.ValidateTreePath(path, root);
+
+        Assert.False(result.IsOk);
+        Assert.Equal(ErrorCode.ADO_VALIDATION_INVALID_PATH, result.ErrorV!.Code);
+        Assert.Contains("invalid character", result.ErrorV!.Errors![0]);
+    }
+
+    [Theory]
+    [InlineData("project\\foo\\ba\u0005r\\baz")]
+    [InlineData("project\\\nfoo\\bar\\baz")]
+    public void TestPathContainsUnicodeControlChar(string invalidPath) {
+        var path = SplitPath(invalidPath);
+        var treePath = SplitPath(@"project\foo\bar\baz");
+        var root = MockTreeNode(treePath, TreeNodeStructureType.Area);
+
+        var result = Ado.ValidateTreePath(path, root);
+
+        Assert.False(result.IsOk);
+        Assert.Equal(ErrorCode.ADO_VALIDATION_INVALID_PATH, result.ErrorV!.Code);
+        Assert.Contains("unicode control character", result.ErrorV!.Errors![0]);
+    }
+
+    [Fact]
+    public void TestPathTooDeep() {
+        var path = SplitPath(@"project\foo\bar\baz\qux\quux\corge\grault\garply\waldo\fred\plugh\xyzzy\thud");
+        var root = MockTreeNode(path, TreeNodeStructureType.Area);
+
+        var result = Ado.ValidateTreePath(path, root);
+
+        Assert.False(result.IsOk);
+        Assert.Equal(ErrorCode.ADO_VALIDATION_INVALID_PATH, result.ErrorV!.Code);
+        Assert.Contains("levels deep", result.ErrorV!.Errors![0]);
+    }
+
+    [Fact]
+    public void TestPathWithoutProjectName() {
+        var path = SplitPath(@"foo\bar\baz");
+        var treePath = SplitPath(@"project\foo\bar\baz");
+        var root = MockTreeNode(treePath, TreeNodeStructureType.Iteration);
+
+        var result = Ado.ValidateTreePath(path, root);
+
+        Assert.False(result.IsOk);
+        Assert.Equal(ErrorCode.ADO_VALIDATION_INVALID_PATH, result.ErrorV!.Code);
+        Assert.Contains("start with the project name", result.ErrorV!.Errors![0]);
+    }
+
+    [Fact]
+    public void TestPathWithInvalidChild() {
+        var path = SplitPath(@"project\foo\baz");
+        var treePath = SplitPath(@"project\foo\bar");
+        var root = MockTreeNode(treePath, TreeNodeStructureType.Iteration);
+
+        var result = Ado.ValidateTreePath(path, root);
+
+        Assert.False(result.IsOk);
+        Assert.Equal(ErrorCode.ADO_VALIDATION_INVALID_PATH, result.ErrorV!.Code);
+        Assert.Contains("not a valid child", result.ErrorV!.Errors![0]);
+    }
+
+    [Fact]
+    public void TestPathWithExtraChild() {
+        var path = SplitPath(@"project\foo\bar\baz");
+        var treePath = SplitPath(@"project\foo\bar");
+        var root = MockTreeNode(treePath, TreeNodeStructureType.Iteration);
+
+        var result = Ado.ValidateTreePath(path, root);
+
+        Assert.False(result.IsOk);
+        Assert.Equal(ErrorCode.ADO_VALIDATION_INVALID_PATH, result.ErrorV!.Code);
+        Assert.Contains("has no children", result.ErrorV!.Errors![0]);
+    }
+}

src/pytypes/onefuzztypes/enums.py

@@ -303,6 +303,7 @@ class ErrorCode(Enum):
     ADO_VALIDATION_UNEXPECTED_ERROR = 491
     ADO_VALIDATION_MISSING_PAT_SCOPES = 492
     ADO_VALIDATION_INVALID_PATH = 495
+    ADO_VALIDATION_INVALID_PROJECT = 496
     # NB: if you update this enum, also update Enums.cs