Security: Pass the validated name onward in metrics service (#cs-12, #cs-13, #cs-14, #cs-18)

src/unitxt/artifact.py

@@ -46,6 +46,72 @@ def verify_legal_catalog_name(name):
     ), f'Artifict name ("{name}") should be alphanumeric. Use "." for nesting (e.g. myfolder.my_artifact)'
 
 
+class IllegalCatalogNameError(ValueError):
+    """Raised when an artifact reference is not a legal catalog name.
+
+    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.
+    """
+
+    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 "'
+            + 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."
+        )
+
+
+def sanitize_catalog_identifier(
+    identifier: str,
+) -> Tuple[str, Optional[Dict[str, Any]]]:
+    """Validates an artifact reference as a catalog name, returning its parts.
+
+    Intended for references that arrive from an untrusted source (a request URL,
+    for example). :func:`fetch_artifact` deliberately accepts filesystem paths as
+    well as catalog names, which is right for library callers but not for a value
+    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 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.
+    """
+    if not isinstance(identifier, str):
+        raise IllegalCatalogNameError(identifier)
+
+    try:
+        name, args = separate_inside_and_outside_square_brackets(identifier)
+    except ValueError as e:
+        # Malformed brackets. Reported as an illegal name so the caller can
+        # answer with a client error instead of an unhandled failure.
+        raise IllegalCatalogNameError(identifier) from e
+
+    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):
+        raise IllegalCatalogNameError(identifier)
+
+    return constants.catalog_hierarchy_sep.join(segments), args
+
+
 def dict_diff_string(dict1, dict2, max_diff=200):
     keys_in_both = dict1.keys() & dict2.keys()
     added = {k: dict2[k] for k in dict2.keys() - dict1.keys()}

src/unitxt/operators.py

@@ -91,6 +91,7 @@
     LRUCache,
     deep_copy,
     flatten_dict,
+    json_dump,
     recursive_copy,
     recursive_shallow_copy,
     shallow_copy,
@@ -1187,11 +1188,31 @@ class ArtifactFetcherMixin:
     _artifacts_cache = LRUCache(max_size=1000)
 
     @classmethod
-    def get_artifact(cls, artifact_identifier: str) -> Artifact:
-        if str(artifact_identifier) not in cls._artifacts_cache:
-            artifact, catalog = fetch_artifact(artifact_identifier)
-            cls._artifacts_cache[str(artifact_identifier)] = artifact
-        return shallow_copy(cls._artifacts_cache[str(artifact_identifier)])
+    def get_artifact(
+        cls,
+        artifact_identifier: str,
+        overwrite_kwargs: Optional[Dict[str, Any]] = None,
+    ) -> Artifact:
+        """Fetches an artifact, caching it by identifier.
+
+        :param artifact_identifier: an Artifact, a catalog name, or a path --
+            whatever :func:`fetch_artifact` accepts.
+        :param overwrite_kwargs: arguments to override on the fetched artifact.
+            Lets a caller that has already parsed ``name[key=value]`` overrides
+            pass them separately, instead of re-serializing them into the
+            identifier.
+        """
+        cache_key = str(artifact_identifier)
+        if overwrite_kwargs:
+            # Keep overridden artifacts from colliding with the plain one, or
+            # with each other, in the cache.
+            cache_key += f"[{json_dump(overwrite_kwargs)}]"
+        if cache_key not in cls._artifacts_cache:
+            artifact, _catalog = fetch_artifact(
+                artifact_identifier, overwrite_kwargs=overwrite_kwargs
+            )
+            cls._artifacts_cache[cache_key] = artifact
+        return shallow_copy(cls._artifacts_cache[cache_key])
 
 
 class ApplyOperatorsField(InstanceOperator):

src/unitxt/service/metrics/main.py

@@ -13,8 +13,8 @@
 
 from ...artifact import (
     Artifact,
-    is_name_legal_for_catalog,
-    separate_inside_and_outside_square_brackets,
+    IllegalCatalogNameError,
+    sanitize_catalog_identifier,
 )
 from ...operator import MultiStreamOperator
 from ...operators import ArtifactFetcherMixin
@@ -71,21 +71,27 @@ def compute(
     t0 = time.perf_counter()
     # Security: metric_name arrives from the request URL, and the artifact loader
     # it is passed to also accepts filesystem paths -- so without this check a
-    # caller could name any file on the server and have it opened, with its
-    # contents surfacing in the error message (CWE-22, path injection).
-    # Only catalog names are a legitimate value here, so require one: the
-    # validator permits [\w.] plus optional [...] overrides, and therefore no
+    # caller could name any file on the server and have it opened (CWE-22, path
+    # injection). Only catalog names are a legitimate value here, so require one:
+    # the validator permits [\w.] plus optional [...] overrides, and therefore no
     # path separators, "..", drive letters or absolute paths.
-    requested_name, _ = separate_inside_and_outside_square_brackets(metric_name)
-    if not is_name_legal_for_catalog(requested_name):
+    #
+    # Everything below uses safe_metric_name, the name returned by the sanitizer,
+    # and never metric_name itself. Passing on the value that was actually
+    # validated is what makes the check load-bearing: a check on one variable
+    # does not make a different variable safe. The raw value is kept out of the
+    # log lines too, so it cannot be used to forge log entries.
+    try:
+        safe_metric_name, metric_overrides = sanitize_catalog_identifier(metric_name)
+    except IllegalCatalogNameError as e:
         raise HTTPException(
             status_code=400,
             detail="Invalid metric name. A metric must be referenced by its "
             "catalog name (alphanumeric, '.' for nesting), not by a file path.",
-        )
+        ) from e
     try:
         logging.info(f"Request from [{token['sub']}]")
-        logging.info(f"Computing metric '{metric_name}'.")
+        logging.info(f"Computing metric '{safe_metric_name}'.")
         logging.info(
             f"MetricRequest contains {len(request['instance_inputs'])} input instances"
         )
@@ -97,7 +103,9 @@ def compute(
             logging.info("Acquired compute_lock, starting computation .. ")
             start_infer_time = datetime.datetime.now()
             # obtain the metric to compute
-            metric: Artifact = ArtifactFetcherMixin.get_artifact(metric_name)
+            metric: Artifact = ArtifactFetcherMixin.get_artifact(
+                safe_metric_name, overwrite_kwargs=metric_overrides
+            )
             metric: MultiStreamOperator = cast(MultiStreamOperator, metric)
             # Confidence interval is currently always off for remote metrics.
             # metric.
@@ -113,7 +121,7 @@ def compute(
         infer_time = datetime.datetime.now() - start_infer_time
         wait_time = start_infer_time - start_time
         logging.info(
-            f"Computed {len(metric_results)} metric '{metric_name}' results, "
+            f"Computed {len(metric_results)} metric '{safe_metric_name}' results, "
             f"took: {infer_time!s}, waited: {wait_time!s}')"
         )
 
@@ -126,7 +134,7 @@ def compute(
     finally:
         t1 = time.perf_counter()
         logging.info(
-            f"Request for metric '{metric_name}' handled in [{t1 - t0:.2f}] secs."
+            f"Request for metric '{safe_metric_name}' handled in [{t1 - t0:.2f}] secs."
         )