Allow : in URL template segments (#7046)

Co-authored-by: Amaury Chamayou <amaury@xargs.fr>

Author: eddyashton

CHANGELOG.md

@@ -13,6 +13,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 - Nodes will now avoid re-parsing `.committed` files in the main directory if they have established a later commit point in the `read_only` directories. This should significantly reduce start-up time for nodes with large existing ledgers.
 
+### Changed
+
+- Allow `:` within regex matched templated URL components again, while still terminating matched segments correctly (#7046).
+
 ### Dependencies
 
 - Updated didx509cpp to 0.11.0 (#7050).

src/endpoints/endpoint_registry.cpp

@@ -131,42 +131,30 @@ namespace ccf::endpoints
 
     PathTemplateSpec spec;
 
-    const std::string allowed_delimiters = "/:";
-
     std::string regex_s(uri);
     template_start = regex_s.find_first_of('{');
+    size_t template_end = 0;
     while (template_start != std::string::npos)
     {
-      if (template_start != 0)
-      {
-        const auto prev_char = regex_s[template_start - 1];
-        if (allowed_delimiters.find(prev_char) == std::string::npos)
-        {
-          throw std::logic_error(fmt::format(
-            "Invalid templated path - illegal character ({}) preceding "
-            "template: {}",
-            prev_char,
-            uri));
-        }
-      }
-
-      const auto template_end = regex_s.find_first_of('}', template_start);
+      template_end = regex_s.find_first_of('}', template_start);
       if (template_end == std::string::npos)
       {
         throw std::logic_error(fmt::format(
           "Invalid templated path - missing closing curly bracket: {}", uri));
       }
 
-      if (template_end + 1 != regex_s.size())
+      // Default regex is "([^/]+)", aka "match everything until the next /"
+      std::string regex_terminator = "/";
+
+      if (template_end < regex_s.size() - 1)
       {
-        const auto next_char = regex_s[template_end + 1];
-        if (allowed_delimiters.find(next_char) == std::string::npos)
+        const auto terminator_candidate = regex_s[template_end + 1];
+        if (terminator_candidate != '/')
         {
-          throw std::logic_error(fmt::format(
-            "Invalid templated path - illegal character ({}) following "
-            "template: {}",
-            next_char,
-            uri));
+          // If there's some other character literal following the template,
+          // treat that as a terminator as well.
+          // eg: "/{foo}:bar" => "/(^[/:]):bar"
+          regex_terminator += terminator_candidate;
         }
       }
 
@@ -175,7 +163,8 @@ namespace ccf::endpoints
       regex_s.replace(
         template_start,
         template_end - template_start + 1,
-        fmt::format("([^{}]+)", allowed_delimiters));
+        fmt::format("([^{}]+)", regex_terminator));
+
       template_start = regex_s.find_first_of('{', template_start + 1);
     }
 

src/endpoints/test/endpoint_registry.cpp

@@ -31,6 +31,34 @@ std::optional<PathTemplateSpec> require_parsed_components(
   return spec;
 }
 
+using ExpectedMatchTestCase = std::map<std::string, std::vector<std::string>>;
+
+void require_template_parsing(
+  const std::string& url_template,
+  const ExpectedMatchTestCase& matched = {},
+  const std::vector<std::string>& unmatched = {})
+{
+  std::optional<PathTemplateSpec> spec;
+  REQUIRE_NOTHROW(spec = PathTemplateSpec::parse(url_template));
+
+  for (const auto& [path, elements] : matched)
+  {
+    std::smatch match;
+    REQUIRE(std::regex_match(path, match, spec->template_regex));
+    REQUIRE(match.size() == elements.size() + 1);
+    for (size_t i = 1; i < match.size(); ++i)
+    {
+      REQUIRE(match[i].str() == elements[i - 1]);
+    }
+  }
+
+  for (const auto& path : unmatched)
+  {
+    std::smatch match;
+    REQUIRE_FALSE(std::regex_match(path, match, spec->template_regex));
+  }
+}
+
 TEST_CASE("URL template parsing")
 {
   ccf::logger::config::default_init();
@@ -54,9 +82,9 @@ TEST_CASE("URL template parsing")
     REQUIRE(match[2].str() == "spain");
 
     path = prefix + "/alice:jump/spain";
-    REQUIRE_FALSE(std::regex_match(path, match, parsed->template_regex));
-    path = prefix + "/alice/spain:jump";
-    REQUIRE_FALSE(std::regex_match(path, match, parsed->template_regex));
+    REQUIRE(std::regex_match(path, match, parsed->template_regex));
+    REQUIRE(match[1].str() == "alice:jump");
+    REQUIRE(match[2].str() == "spain");
 
     require_parsed_components(prefix + "/{name}:do", {"name"});
     require_parsed_components(prefix + "/{name}:do/world", {"name"});
@@ -84,15 +112,27 @@ TEST_CASE("URL template parsing")
     REQUIRE(match[3].str() == "spain");
   }
 
-  REQUIRE_THROWS(PathTemplateSpec::parse("/foo{id}"));
-  REQUIRE_THROWS(PathTemplateSpec::parse("/foo{id}bar"));
-  REQUIRE_THROWS(PathTemplateSpec::parse("/{id}bar"));
-  REQUIRE_THROWS(PathTemplateSpec::parse("/{id}-{name}"));
-  REQUIRE_THROWS(PathTemplateSpec::parse("/id{id}"));
-  REQUIRE_THROWS(PathTemplateSpec::parse("/foo{id}:"));
-  REQUIRE_THROWS(PathTemplateSpec::parse("/foo{id}/bar"));
-  REQUIRE_THROWS(PathTemplateSpec::parse("/foo/{id}bar"));
-  REQUIRE_THROWS(PathTemplateSpec::parse("/foo/id{id}:bar"));
+  require_template_parsing(
+    "/foo{id}",
+    {{"/foo1", {"1"}}, {"/foobar", {"bar"}}},
+    {"/foo", "/foo/1", "/foo/bar"});
+  require_template_parsing(
+    "/foo{id}bar",
+    {{"/foo1bar", {"1"}}, {"/foofazbar", {"faz"}}},
+    {"/foobar", "/foobazbar", "/foo/bar", "/foo/baz/bar"});
+  require_template_parsing(
+    "/{id}bar",
+    {{"/1bar", {"1"}}, {"/foobar", {"foo"}}},
+    {"/bar", "/bazbar", "/foo/bar", "foo/bar"});
+  require_template_parsing(
+    "/{id}-{name}",
+    {{"/foo-bar", {"foo", "bar"}}, {"/1-2", {"1", "2"}}},
+    {"/foobar", "/foo/-bar", "/foo-/bar", "/foo/-/bar"});
+  require_template_parsing("/id{id}");
+  require_template_parsing("/foo{id}:");
+  require_template_parsing("/foo{id}/bar");
+  require_template_parsing("/foo/{id}bar");
+  require_template_parsing("/foo/id{id}:bar");
 
   REQUIRE_THROWS(PathTemplateSpec::parse("/{id}/{id}"));
   REQUIRE_THROWS(PathTemplateSpec::parse("/foo/{id}/{id}"));