Security: Resolve metric names against the catalog (#cs-12, #cs-13, #cs-14, #cs-18)

src/unitxt/artifact.py

@@ -27,6 +27,7 @@
 from .utils import (
     artifacts_json_cache,
     json_dump,
+    lru_cache_decorator,
     save_to_file,
     shallow_copy,
 )
@@ -47,25 +48,71 @@ def verify_legal_catalog_name(name):
 
 
 class IllegalCatalogNameError(ValueError):
-    """Raised when an artifact reference is not a legal catalog name.
+    """Raised when an artifact reference is not an available catalog entry.
 
     Used by :func:`sanitize_catalog_identifier` so callers that accept an
-    artifact reference from an untrusted source can tell "this is not a catalog
-    name" apart from any other failure, and answer accordingly.
+    artifact reference from an untrusted source can tell "this is not a usable
+    catalog name" apart from any other failure, and answer accordingly. Covers
+    both a malformed name and a well-formed one the catalogs do not publish;
+    the two are deliberately not distinguished, so the error cannot be used to
+    probe which names exist.
     """
 
     def __init__(self, name):
         self.name = name
         super().__init__(
-            f'Artifact reference ("{name}") is not a legal catalog name. A catalog '
-            'name is alphanumeric, uses "'
+            f'Artifact reference ("{name}") is not an available catalog entry. A '
+            'catalog name is alphanumeric, uses "'
             + constants.catalog_hierarchy_sep
-            + '" for nesting (e.g. metrics.accuracy), and may carry overrides in '
-            "square brackets (e.g. metrics.f1_macro[average=macro]). File paths "
-            "are not accepted."
+            + '" for nesting (e.g. metrics.accuracy), may carry overrides in '
+            "square brackets (e.g. metrics.f1_macro[average=macro]), and must "
+            "name an entry the catalogs publish. File paths are not accepted."
         )
 
 
+@lru_cache_decorator(max_size=1)
+def _catalog_name_index(generation: int) -> Dict[str, str]:
+    """Maps each catalog name to the catalog's own copy of that string.
+
+    Keyed on a generation counter so that registering a new artifact
+    invalidates it; see :func:`invalidate_catalog_name_index`. Values are the
+    strings produced by walking the catalogs, so a lookup hands back catalog
+    data instead of anything derived from a caller's input.
+
+    Only *local* catalogs are indexed, because ``ls`` walks the filesystem. A
+    name published solely by a remote catalog is therefore not resolvable here,
+    and a caller that only accepts indexed names (the metrics service) will
+    reject it. That is deliberate: consulting a remote catalog would mean an
+    HTTP request per lookup, and the name it vouched for would once again be
+    one rebuilt from the request rather than catalog data. The remote catalog
+    mirrors this repository at the release tag, so the practical gap is a name
+    newer than the installed release, or one in an operator's own remote
+    catalog.
+    """
+    from .catalog import ls
+
+    return {name: name for name in ls(to_file=os.devnull)}
+
+
+_catalog_name_generation = 0
+
+
+def invalidate_catalog_name_index():
+    """Forces the catalog name index to be rebuilt on next use.
+
+    Called when an artifact is added to a catalog at runtime, so a name saved
+    during the process's lifetime is still resolvable.
+    """
+    global _catalog_name_generation
+    _catalog_name_generation += 1
+
+
+def _lookup_catalog_name(name: str) -> Optional[str]:
+    """Returns the catalog's own string for ``name``, or None if unknown."""
+    index = _catalog_name_index(_catalog_name_generation)
+    return index.get(name)
+
+
 def sanitize_catalog_identifier(
     identifier: str,
 ) -> Tuple[str, Optional[Dict[str, Any]]]:
@@ -77,18 +124,22 @@ def sanitize_catalog_identifier(
     an outside caller controls -- there, a path would let that caller name any
     file on the host (CWE-22).
 
-    Returning the validated name rather than the original string is what makes
-    this safe to rely on: the caller passes on the value that was actually
-    checked, so no unvalidated substring can reach a loader, and the
-    sanitization is visible to static analysis rather than only to a reader.
+    The name is resolved against the names the catalogs publish, and what comes
+    back is the catalog's own string, not a value built from the caller's. That
+    distinction is the whole point: a name rebuilt from its validated parts is
+    still derived from the request, so it is only ever as good as the checks
+    applied to it. Returning catalog data instead means a caller can pass the
+    result to a loader without re-validating it.
+
     The bracketed overrides are returned already parsed, to be applied as
     keyword overrides instead of being re-serialized into the name.
 
     :param identifier: the artifact reference to validate.
-    :return: a tuple ``(name, overrides)``, where ``name`` is the validated
-        catalog name and ``overrides`` is the parsed contents of the square
-        brackets, or ``None`` if there were none.
-    :raises IllegalCatalogNameError: if ``identifier`` is not a legal catalog name.
+    :return: a tuple ``(name, overrides)``, where ``name`` is the catalog's own
+        copy of the resolved name and ``overrides`` is the parsed contents of
+        the square brackets, or ``None`` if there were none.
+    :raises IllegalCatalogNameError: if ``identifier`` is not a legal catalog
+        name, or names nothing the catalogs publish.
     """
     if not isinstance(identifier, str):
         raise IllegalCatalogNameError(identifier)
@@ -103,13 +154,19 @@ def sanitize_catalog_identifier(
     if not is_name_legal_for_catalog(name):
         raise IllegalCatalogNameError(identifier)
 
-    # Rebuilt from the validated segments, so the returned name is derived from
-    # what the check above accepted and shares nothing with the raw input.
-    segments = name.split(constants.catalog_hierarchy_sep)
-    if not all(is_name_legal_for_catalog(segment) for segment in segments):
+    # Resolve the request against the names the catalogs actually publish, and
+    # return the catalog's own string rather than anything built from the
+    # caller's. Rebuilding the name from its validated parts is not enough: the
+    # result is still a value derived from the request, so it cannot be any
+    # safer than the checks applied to it, and both static analysis and a
+    # reviewer are right to treat it as untrusted. Looking the name up replaces
+    # it with a string that originates in the catalog listing, which is what
+    # lets everything downstream open it without re-validating.
+    canonical = _lookup_catalog_name(name)
+    if canonical is None:
         raise IllegalCatalogNameError(identifier)
 
-    return constants.catalog_hierarchy_sep.join(segments), args
+    return canonical, args
 
 
 def dict_diff_string(dict1, dict2, max_diff=200):

src/unitxt/catalog.py

@@ -12,6 +12,7 @@
     ArtifactLink,
     Catalogs,
     get_catalog_name_and_args,
+    invalidate_catalog_name_index,
     reset_artifacts_json_cache,
     verify_legal_catalog_name,
 )
@@ -135,6 +136,9 @@ def add_to_catalog(
         catalog = LocalCatalog(location=catalog_path)
     verify_legal_catalog_name(name)
     catalog.save_artifact(artifact, name, overwrite=overwrite, verbose=verbose)
+    # The name index is built by walking the catalogs, so a newly saved
+    # artifact must invalidate it to stay resolvable in this process.
+    invalidate_catalog_name_index()
 
 
 def add_link_to_catalog(

tests/library/test_path_injection_service.py

@@ -5,16 +5,22 @@
 reaching ``open()`` in ``Artifact.is_artifact_file`` and ``utils.load_json``,
 because the artifact loader accepts filesystem paths as well as catalog names.
 
-A first fix (#1993) added a validity check to the endpoint, but kept passing the
-original ``metric_name`` to the loader. The check was effective at runtime, yet
-the value that was validated and the value that was used were different
-variables, so the alerts stayed open -- rightly, since that shape only stays
-correct as long as nobody edits the two lines apart.
-
-The remediation tested here:
-
-1. ``sanitize_catalog_identifier`` validates a reference and returns the name it
-   validated, so callers pass on the checked value rather than the raw one.
+Two earlier attempts did not clear the alerts, and both were wrong for the same
+underlying reason -- the value handed to the loader was still derived from the
+request:
+
+- #1993 validated a derived value and then passed the original ``metric_name``
+  on, so the check said nothing about what actually reached the loader.
+- #1995 returned a name rebuilt from the validated parts. That reads safer, but
+  ``split``/``join`` still derive the result from the input, so it was no safer
+  than the checks applied to it and the taint path survived intact.
+
+The remediation tested here resolves the request against the catalog and returns
+the catalog's own string, so the value passed on originates in trusted data
+rather than in the request:
+
+1. ``sanitize_catalog_identifier`` looks the name up and returns the catalog's
+   copy of it, rejecting anything the catalogs do not publish.
 2. The service uses only that returned name, and answers 400 otherwise.
 3. ``load_json`` does not echo file contents into its error, so a path that does
    reach it cannot be used to read a file (CWE-209).
@@ -28,9 +34,11 @@
 import tempfile
 import unittest
 
+from unitxt import artifact as artifact_module
 from unitxt.artifact import (
     Artifact,
     IllegalCatalogNameError,
+    _catalog_name_index,
     fetch_artifact,
     sanitize_catalog_identifier,
 )
@@ -103,7 +111,9 @@ def test_is_a_value_error(self):
 
 class TestSanitizeCatalogIdentifierAcceptsNames(unittest.TestCase):
     def test_accepts_plain_catalog_names(self):
-        for name in ("metrics.accuracy", "accuracy", "metrics.rouge"):
+        # Must be names the catalogs actually publish; a bare "accuracy" is not
+        # one (it does not resolve on an unmodified tree either).
+        for name in ("metrics.accuracy", "metrics.rouge", "metrics.f1_macro"):
             with self.subTest(name=name):
                 self.assertEqual(sanitize_catalog_identifier(name), (name, None))
 
@@ -115,19 +125,66 @@ def test_accepts_catalog_names_with_overrides(self):
         self.assertEqual(name, "metrics.f1_macro")
         self.assertEqual(overrides, {"ignore_case": False})
 
-    def test_returns_the_validated_name_not_the_input(self):
-        """The point of the fix: what comes back is derived from what was checked.
+    def test_returns_the_catalogs_own_string_not_the_callers(self):
+        """The point of the fix: the returned value comes from the catalog.
 
-        If this ever returns the caller's string unchanged, the returned value is
-        no safer than the input and the sanitizer is decorative.
+        Rebuilding the name from the caller's input (an earlier attempt) left a
+        value still derived from the request. Resolving it against the catalog
+        replaces it with a string that originates in the catalog listing, which
+        is what lets callers pass it on without re-validating. Asserting
+        identity, not equality, is what distinguishes the two.
         """
+        requested = "".join(["metrics", ".", "accuracy"])
+        returned, _ = sanitize_catalog_identifier(requested)
+
+        self.assertEqual(returned, requested)
+        self.assertIsNot(
+            returned,
+            requested,
+            "the returned name must not be the caller's own string object",
+        )
+        index = _catalog_name_index(artifact_module._catalog_name_generation)
+        self.assertIs(
+            returned,
+            index["metrics.accuracy"],
+            "the returned name must be the catalog's own string object",
+        )
+
+    def test_strips_overrides_from_the_returned_name(self):
         returned, overrides = sanitize_catalog_identifier(
             "metrics.f1_macro[ignore_case=False]"
         )
         self.assertNotIn("[", returned)
         self.assertEqual(returned, "metrics.f1_macro")
         self.assertEqual(overrides, {"ignore_case": False})
 
+    def test_only_local_catalog_names_are_indexed(self):
+        """Documents a deliberate limitation, so a change here is a decision.
+
+        The index is built by walking local catalogs, so a name published only
+        by a remote catalog is not resolvable and the service rejects it. See
+        the note on ``_catalog_name_index``.
+        """
+        index = _catalog_name_index(artifact_module._catalog_name_generation)
+        from unitxt.catalog import get_local_catalogs_paths, ls
+
+        self.assertEqual(
+            set(index), set(ls(to_file=os.devnull)), "index must match the local walk"
+        )
+        self.assertTrue(
+            get_local_catalogs_paths(), "sanity: there is at least one local catalog"
+        )
+
+    def test_rejects_names_the_catalogs_do_not_publish(self):
+        """A syntactically legal name that does not exist must not reach a loader."""
+        for name in (
+            "metrics.no_such_metric_zzz",
+            "not_a_real_namespace.thing",
+        ):
+            with self.subTest(name=name):
+                with self.assertRaises(IllegalCatalogNameError):
+                    sanitize_catalog_identifier(name)
+
 
 class TestSanitizedNameIsWhatTheServiceUses(unittest.TestCase):
     """The sanitizer only helps if ``compute`` passes on its result.