From 191d14027d3cb874123a45a1be6a9218c383a1dc Mon Sep 17 00:00:00 2001 From: Yoav Katz Date: Wed, 4 Nov 2026 11:59:17 +0200 Subject: [PATCH] Security: Pass the validated name onward in metrics service (#cs-12, #cs-13, #cs-14, #cs-18) Remediates the 4 py/path-injection CodeQL alerts (HIGH, CWE-22) still open after the previous fix, reported in the IBM OSPO security notification (#1994): cs-12, cs-13, cs-14 (src/unitxt/artifact.py:238,240) and cs-18 (src/unitxt/utils.py:222). The earlier fix (#1993) blocked the attack at runtime but left the alerts open, and the alerts were right to stay open. It validated a derived value and then passed the original one on: requested_name, _ = separate_inside_and_outside_square_brackets(metric_name) if not is_name_legal_for_catalog(requested_name): raise HTTPException(status_code=400, ...) ... metric = ArtifactFetcherMixin.get_artifact(metric_name) # <- raw value Checking requested_name says nothing about metric_name, so nothing about the value that actually reached the loader. It happened to be safe only because the two strings agreed on the part that mattered -- a property no check enforced and any later edit could break. CodeQL reported it as unsanitized because, as dataflow, it is. The taint path in the current SARIF is unchanged from before the fix: service/metrics/main.py:69,100 metric_name (FastAPI path parameter) operators.py:1190-1192 fetch_artifact(artifact_identifier) artifact.py:546,562-563 fetch_artifact(artifact_rep) artifact.py:238,240 open(path) <- cs-12, cs-13, cs-14 utils.py:203-204,221-222 open(path) <- cs-18 cs-18 is a fresh alert raised against the fix commit itself, replacing cs-15 and cs-16 at the same sink: the flow survived, only its line numbers moved. The fix makes the sanitization real rather than incidental: 1. artifact.py -- new sanitize_catalog_identifier(). Validates a reference as a catalog name and returns the name it validated, rebuilt from its checked segments, plus the bracketed overrides already parsed. Callers pass on the value that was checked, so no unvalidated substring can reach a loader and the guarantee no longer depends on two variables staying in agreement. Rejections raise IllegalCatalogNameError, a ValueError subclass, so a caller can answer 400 for "not a catalog name" without catching unrelated failures. Malformed brackets ("metrics.accuracy[unclosed") now reject cleanly too; before, the parser's ValueError reached the endpoint's broad except and became a 500. 2. service/metrics/main.py -- use only the sanitized name. compute() passes safe_metric_name to the loader and never metric_name, which is now consumed by the sanitizer and nothing else. The raw value is also kept out of the log lines, so it cannot be used to forge log entries. 3. operators.py -- get_artifact() takes overwrite_kwargs. Since overrides now travel beside the name instead of inside it, they are applied as keyword overrides. Re-serializing them into the identifier was not an option: the parser returns typed values, so "[n=[1,2]]" and nested artifact references do not round-trip through a string. The cache key includes the overrides, so an overridden artifact cannot be served from, or written to, the plain artifact's entry. The parameter is optional and all seven existing callers are unaffected. fetch_artifact()'s support for loading an artifact from a file path is again left intact: it is a documented library feature, and the untrusted input is blocked at the service boundary where the expectation is narrower. Verified: the planted-file read and the arbitrary-file read are both refused with 400, with no file content in the error; legitimate names, and names with overrides, still resolve and the override is applied. Tests: tests/library/test_path_injection_service.py, extended to 20 tests. Three of them fail against the #1993 code, including one that names get_artifact(metric_name) as the raw-value use that kept the alerts open -- that class asserts the endpoint never passes the raw parameter onward, so the regression this commit fixes cannot recur silently. Added coverage for the sanitizer's rejections (absolute paths, traversal, Windows paths, URLs, null bytes, malformed brackets, non-strings), for overrides still applying, and for the cache not being poisoned across overridden and plain fetches. The earlier tests pinning load_json's non-disclosure and file-path loading are kept. tests/library: 799 passed. The 3 failures (test_metrics.py::test_f1_strings, test_utils.py::test_require_errors, test_type_utils.py::test_parse_malformed_string) reproduce identically on a clean main tree and come from optional dependencies missing locally. ruff 0.1.6 (the pinned pre-commit version) check and format, codespell and the relative-imports hook all pass. Reported by: IBM OSPO security scanning (CodeQL default setup) Closes: #1994 Signed-off-by: Yoav Katz --- src/unitxt/artifact.py | 66 ++++++ src/unitxt/operators.py | 31 ++- src/unitxt/service/metrics/main.py | 34 +-- tests/library/test_path_injection_service.py | 208 ++++++++++++++----- 4 files changed, 267 insertions(+), 72 deletions(-) diff --git a/src/unitxt/artifact.py b/src/unitxt/artifact.py index e1ccae320e..94044d4555 100644 --- a/src/unitxt/artifact.py +++ b/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()} diff --git a/src/unitxt/operators.py b/src/unitxt/operators.py index 410b940df1..43385f5eb0 100644 --- a/src/unitxt/operators.py +++ b/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): diff --git a/src/unitxt/service/metrics/main.py b/src/unitxt/service/metrics/main.py index ec29b55228..32568177ba 100644 --- a/src/unitxt/service/metrics/main.py +++ b/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." ) diff --git a/tests/library/test_path_injection_service.py b/tests/library/test_path_injection_service.py index 7fca994361..4e1a9727f0 100644 --- a/tests/library/test_path_injection_service.py +++ b/tests/library/test_path_injection_service.py @@ -1,16 +1,23 @@ -"""Regression tests for CodeQL py/path-injection alerts cs-12 .. cs-16 (#1991). +"""Regression tests for CodeQL py/path-injection alerts cs-12..cs-18 (#1991, #1994). -CodeQL traced one taint flow for all five alerts: the ``metric_name`` URL path -parameter of the metrics service (``src/unitxt/service/metrics/main.py``) reaching -``open()`` in ``Artifact.is_artifact_file`` and ``utils.load_json``, because the -artifact loader accepts filesystem paths as well as catalog names. +CodeQL traced one taint flow for every alert: the ``metric_name`` URL path +parameter of the metrics service (``src/unitxt/service/metrics/main.py``) +reaching ``open()`` in ``Artifact.is_artifact_file`` and ``utils.load_json``, +because the artifact loader accepts filesystem paths as well as catalog names. -The remediation has two halves, tested here: +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. -1. The service rejects anything that is not a catalog name, so the untrusted - value never reaches the loader. -2. ``load_json`` no longer echoes file contents into its error, so a path that - does reach it cannot be used to read a file (CWE-209). +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. +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). The library's own support for loading an artifact from a file path is a documented feature and is deliberately left working. @@ -23,42 +30,52 @@ from unitxt.artifact import ( Artifact, + IllegalCatalogNameError, fetch_artifact, - is_name_legal_for_catalog, - separate_inside_and_outside_square_brackets, + sanitize_catalog_identifier, ) +from unitxt.operators import ArtifactFetcherMixin from unitxt.utils import load_json - -def _service_guard_accepts(metric_name): - """Mirrors the check performed by the service's ``compute`` endpoint. - - The endpoint itself needs fastapi, which is an optional dependency, so the - guard logic is exercised directly here. - """ - requested_name, _ = separate_inside_and_outside_square_brackets(metric_name) - return bool(is_name_legal_for_catalog(requested_name)) - - -class TestServiceMetricNameGuard(unittest.TestCase): - def test_rejects_absolute_paths(self): - for name in ("/etc/passwd", "/tmp/secret.txt", "/var/run/secrets/token"): - with self.subTest(name=name): - self.assertFalse(_service_guard_accepts(name)) - - def test_rejects_traversal(self): - for name in ( - "../../etc/passwd", - "../catalog/thing.json", - "metrics.accuracy/../../etc/passwd", - ): +# Shapes that must never be accepted from an untrusted caller. Each would +# otherwise be opened as a filesystem path by the artifact loader. +UNSAFE_REFERENCES = [ + "/etc/passwd", + "/tmp/secret.txt", + "/var/run/secrets/token", + "../../etc/passwd", + "../catalog/thing.json", + "metrics.accuracy/../../etc/passwd", + r"C:\windows\win.ini", + r"..\..\secret.txt", + "~/.ssh/id_rsa", + "./relative.json", + "/", + "", + "metrics\x00.accuracy", + "http://evil.example/x.json", +] + + +class TestSanitizeCatalogIdentifierRejectsPaths(unittest.TestCase): + def test_rejects_unsafe_references(self): + for name in UNSAFE_REFERENCES: with self.subTest(name=name): - self.assertFalse(_service_guard_accepts(name)) + with self.assertRaises(IllegalCatalogNameError): + sanitize_catalog_identifier(name) - def test_rejects_windows_style_paths(self): - for name in (r"C:\windows\win.ini", r"..\..\secret.txt"): + def test_rejects_malformed_brackets(self): + """Must be a clean rejection, not an unhandled ValueError from the parser.""" + for name in ("metrics.accuracy[unclosed", "a]b[c", "metrics.x]"): with self.subTest(name=name): - self.assertFalse(_service_guard_accepts(name)) + with self.assertRaises(IllegalCatalogNameError): + sanitize_catalog_identifier(name) + + def test_rejects_non_strings(self): + for value in (None, 123, ["metrics.accuracy"], {"a": 1}): + with self.subTest(value=value): + with self.assertRaises(IllegalCatalogNameError): + sanitize_catalog_identifier(value) def test_rejects_a_real_artifact_file_path(self): """Even a genuine artifact file must not be loadable by path remotely.""" @@ -71,27 +88,58 @@ def test_rejects_a_real_artifact_file_path(self): Artifact.is_artifact_file(path), "sanity: the file is a loadable artifact, so only the guard stops it", ) - self.assertFalse(_service_guard_accepts(path)) + with self.assertRaises(IllegalCatalogNameError): + sanitize_catalog_identifier(path) + + def test_error_names_the_offending_value(self): + with self.assertRaises(IllegalCatalogNameError) as ctx: + sanitize_catalog_identifier("/etc/passwd") + self.assertEqual(ctx.exception.name, "/etc/passwd") + + def test_is_a_value_error(self): + """Callers catching ValueError must keep working.""" + self.assertTrue(issubclass(IllegalCatalogNameError, ValueError)) + +class TestSanitizeCatalogIdentifierAcceptsNames(unittest.TestCase): def test_accepts_plain_catalog_names(self): for name in ("metrics.accuracy", "accuracy", "metrics.rouge"): with self.subTest(name=name): - self.assertTrue(_service_guard_accepts(name)) + self.assertEqual(sanitize_catalog_identifier(name), (name, None)) def test_accepts_catalog_names_with_overrides(self): - """Bracketed overrides must keep working; only the name part is checked.""" - self.assertTrue(_service_guard_accepts("metrics.f1_macro[average=macro]")) + """Bracketed overrides must keep working; only the name part is a path risk.""" + name, overrides = sanitize_catalog_identifier( + "metrics.f1_macro[ignore_case=False]" + ) + 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. -class TestServiceEndpointAppliesTheGuard(unittest.TestCase): - """The guard above is only meaningful if ``compute`` actually performs it. + If this ever returns the caller's string unchanged, the returned value is + no safer than the input and the sanitizer is decorative. + """ + 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}) + + +class TestSanitizedNameIsWhatTheServiceUses(unittest.TestCase): + """The sanitizer only helps if ``compute`` passes on its result. The endpoint needs fastapi (an optional dependency), so rather than calling - it, assert that its source wires the validator in before the artifact - loader. This fails if the check is removed or moved after get_artifact. + it, assert its source uses the sanitized name and never the raw parameter. + This fails if the two are edited apart again, which is what kept the alerts + open after the first fix. """ - def _compute_source(self): + @staticmethod + def _compute_source(): service_main = os.path.join( os.path.dirname( os.path.dirname(os.path.dirname(os.path.abspath(__file__))) @@ -107,18 +155,70 @@ def _compute_source(self): start = source.index("def compute(") return source[start : source.index("\n@app.", start)] - def test_compute_validates_the_name_before_loading_the_artifact(self): + def test_compute_sanitizes_before_loading_the_artifact(self): body = self._compute_source() - self.assertIn("is_name_legal_for_catalog", body) - self.assertIn("HTTPException", body) + self.assertIn("sanitize_catalog_identifier", body) self.assertLess( - body.index("is_name_legal_for_catalog"), + body.index("sanitize_catalog_identifier"), body.index("get_artifact"), - "the name must be validated before it reaches the artifact loader", + "the name must be sanitized before it reaches the artifact loader", ) def test_compute_rejects_with_a_client_error(self): - self.assertIn("status_code=400", self._compute_source()) + body = self._compute_source() + self.assertIn("IllegalCatalogNameError", body) + self.assertIn("status_code=400", body) + + def test_compute_never_passes_the_raw_parameter_onward(self): + """``metric_name`` must be consumed by the sanitizer and nothing else.""" + body = self._compute_source() + uses = [ + line.strip() + for line in body.splitlines() + if "metric_name" in line + and "safe_metric_name" not in line + and not line.strip().startswith("#") + ] + # Only the signature and the sanitizer call may mention the raw value. + for line in uses: + with self.subTest(line=line): + self.assertTrue( + "metric_name: str" in line + or "sanitize_catalog_identifier(metric_name)" in line, + f"raw metric_name must not be used here: {line}", + ) + + +class TestGetArtifactOverrides(unittest.TestCase): + """Overrides now travel beside the name, so they must still apply.""" + + def test_overrides_are_applied(self): + name, overrides = sanitize_catalog_identifier( + "metrics.f1_macro[ignore_case=False]" + ) + artifact = ArtifactFetcherMixin.get_artifact(name, overwrite_kwargs=overrides) + self.assertFalse(artifact.ignore_case) + + def test_overrides_do_not_poison_the_cache_for_the_plain_artifact(self): + plain_before = ArtifactFetcherMixin.get_artifact("metrics.f1_macro") + self.assertTrue(plain_before.ignore_case, "sanity: default is True") + + name, overrides = sanitize_catalog_identifier( + "metrics.f1_macro[ignore_case=False]" + ) + overridden = ArtifactFetcherMixin.get_artifact(name, overwrite_kwargs=overrides) + self.assertFalse(overridden.ignore_case) + + plain_after = ArtifactFetcherMixin.get_artifact("metrics.f1_macro") + self.assertTrue( + plain_after.ignore_case, + "an overridden fetch must not be served from, or written to, " + "the cache entry of the plain artifact", + ) + + def test_without_overrides_behaves_as_before(self): + artifact = ArtifactFetcherMixin.get_artifact("metrics.accuracy") + self.assertIsNotNone(artifact) class TestLoadJsonDoesNotLeakFileContents(unittest.TestCase):