From f821ac1eaa9b97bfd2536253629901a8bdf46d15 Mon Sep 17 00:00:00 2001 From: Yoav Katz Date: Mon, 2 Nov 2026 12:05:49 +0200 Subject: [PATCH] Security: Fix py/path-injection in metrics service (cs-12..cs-16) Remediates the 5 open py/path-injection CodeQL alerts (HIGH, CWE-22) reported in the IBM OSPO security notification (#1991): alerts cs-12, cs-13, cs-14, cs-15 and cs-16. CodeQL reports five alerts but traced a single taint flow for all of them. The source is the metric_name URL path parameter of the metrics service: service/metrics/main.py:65 metric_name (FastAPI path parameter) service/metrics/main.py:82 ArtifactFetcherMixin.get_artifact(metric_name) operators.py:1190-1192 fetch_artifact(artifact_identifier) artifact.py:546,562 fetch_artifact(artifact_rep) artifact.py:237-240 open(path) <- cs-12, cs-13, cs-14 artifact.py:284-285 Artifact.load -> artifacts_json_cache utils.py:203-204,221-226 open(path) <- cs-15, cs-16 The underlying cause is a mismatch of expectations. fetch_artifact() is documented to accept either a catalog name or a local file path, which is correct for library callers. The service then passed a value straight off the request URL into it, so a caller could name any file on the server instead of a catalog entry. This was confirmed to be exploitable, not merely a taint-tracking artifact: - fetch_artifact() loaded an artifact JSON file from an absolute path entirely outside the catalog. - load_json() put the full contents of whatever it opened into its exception message, so pointing the request at a non-JSON file disclosed that file verbatim. A planted file came back in the error, contents and all. The endpoint requires a token, so this is not pre-auth, but it still let any token holder read arbitrary files readable by the service account, which is a clear privilege escalation over computing a metric. Fixed in two places, so neither half depends on the other: 1. service/metrics/main.py -- require a catalog name. metric_name is validated with the existing is_name_legal_for_catalog() before it reaches the loader, and a bad value returns HTTP 400 rather than being opened. That validator permits only [\w.], so path separators, "..", absolute paths and drive letters are all rejected. The name is first split with separate_inside_and_outside_square_brackets(), so legitimate overrides such as metrics.f1_macro[average=macro] keep working. The check sits before the function's try block, so the 400 reaches the existing HTTPException handler instead of being turned into a 500 by the broad except. 2. utils.py -- stop load_json() echoing file contents. The JSONDecodeError handler no longer reads the file back into the error message (CWE-209). This is what turned a path into a file-read primitive, and removing it also protects any other caller that passes an untrusted path. The JSON decoder's own message is kept, so the error still names the file and the failing line and column, which is what is actually needed to debug a malformed catalog entry. No test or caller depended on the old "with file content:" wording. fetch_artifact()'s support for loading an artifact from a file path is deliberately left intact: it is a documented feature used by library callers, and restricting it would be a breaking change. The untrusted input is blocked at the service boundary, where the expectation is narrower, rather than by removing a capability from the library. Tests: tests/library/test_path_injection_service.py. Four of them fail against the unpatched code -- the two disclosure tests, plus two asserting the endpoint wires the validator in before get_artifact() and answers 400, since the endpoint itself cannot be called in environments without the optional fastapi dependency. The rest pin behaviour that must not regress: catalog names with and without overrides, loading an artifact from a file path, and load_json() on valid JSON. tests/library: 788 passed. The three 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. Reported by: IBM OSPO security scanning (CodeQL default setup) Closes: #1991 Co-Authored-By: Claude Opus 5 Signed-off-by: Yoav Katz --- src/unitxt/service/metrics/main.py | 20 +- src/unitxt/utils.py | 12 +- tests/library/test_path_injection_service.py | 183 +++++++++++++++++++ 3 files changed, 209 insertions(+), 6 deletions(-) create mode 100644 tests/library/test_path_injection_service.py diff --git a/src/unitxt/service/metrics/main.py b/src/unitxt/service/metrics/main.py index 059573e424..ec29b55228 100644 --- a/src/unitxt/service/metrics/main.py +++ b/src/unitxt/service/metrics/main.py @@ -11,7 +11,11 @@ from fastapi.exceptions import HTTPException from starlette.responses import JSONResponse -from ...artifact import Artifact +from ...artifact import ( + Artifact, + is_name_legal_for_catalog, + separate_inside_and_outside_square_brackets, +) from ...operator import MultiStreamOperator from ...operators import ArtifactFetcherMixin from ...stream import MultiStream @@ -65,6 +69,20 @@ def compute( metric_name: str, request: dict, token: dict = Depends(verify_token) ) -> dict: 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 + # 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): + 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.", + ) try: logging.info(f"Request from [{token['sub']}]") logging.info(f"Computing metric '{metric_name}'.") diff --git a/src/unitxt/utils.py b/src/unitxt/utils.py index 2bfd7b1522..5226bcb3db 100644 --- a/src/unitxt/utils.py +++ b/src/unitxt/utils.py @@ -223,11 +223,13 @@ def load_json(path): try: return json.load(f) except json.decoder.JSONDecodeError as e: - with open(path) as f: - file_content = "\n".join(f.readlines()) - raise RuntimeError( - f"Failed to decode json file at '{path}' with file content:\n{file_content}" - ) from e + # Security: the file contents are deliberately NOT included here. + # This function is reachable with a caller-supplied path, so echoing + # the body of whatever was opened turns a malformed-file error into + # an information disclosure primitive (CWE-209). The decoder's own + # message still reports the line and column that failed, which is + # what is actually needed to fix a broken catalog file. + raise RuntimeError(f"Failed to decode json file at '{path}': {e}") from e def save_to_file(path, data): diff --git a/tests/library/test_path_injection_service.py b/tests/library/test_path_injection_service.py new file mode 100644 index 0000000000..7fca994361 --- /dev/null +++ b/tests/library/test_path_injection_service.py @@ -0,0 +1,183 @@ +"""Regression tests for CodeQL py/path-injection alerts cs-12 .. cs-16 (#1991). + +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. + +The remediation has two halves, tested here: + +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 library's own support for loading an artifact from a file path is a +documented feature and is deliberately left working. +""" + +import json +import os +import tempfile +import unittest + +from unitxt.artifact import ( + Artifact, + fetch_artifact, + is_name_legal_for_catalog, + separate_inside_and_outside_square_brackets, +) +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", + ): + with self.subTest(name=name): + self.assertFalse(_service_guard_accepts(name)) + + def test_rejects_windows_style_paths(self): + for name in (r"C:\windows\win.ini", r"..\..\secret.txt"): + with self.subTest(name=name): + self.assertFalse(_service_guard_accepts(name)) + + def test_rejects_a_real_artifact_file_path(self): + """Even a genuine artifact file must not be loadable by path remotely.""" + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "metric.json") + with open(path, "w") as handle: + json.dump({"__type__": "accuracy"}, handle) + + self.assertTrue( + Artifact.is_artifact_file(path), + "sanity: the file is a loadable artifact, so only the guard stops it", + ) + self.assertFalse(_service_guard_accepts(path)) + + 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)) + + 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]")) + + +class TestServiceEndpointAppliesTheGuard(unittest.TestCase): + """The guard above is only meaningful if ``compute`` actually performs it. + + 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. + """ + + def _compute_source(self): + service_main = os.path.join( + os.path.dirname( + os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + ), + "src", + "unitxt", + "service", + "metrics", + "main.py", + ) + with open(service_main) as handle: + source = handle.read() + start = source.index("def compute(") + return source[start : source.index("\n@app.", start)] + + def test_compute_validates_the_name_before_loading_the_artifact(self): + body = self._compute_source() + self.assertIn("is_name_legal_for_catalog", body) + self.assertIn("HTTPException", body) + self.assertLess( + body.index("is_name_legal_for_catalog"), + body.index("get_artifact"), + "the name must be validated before it reaches the artifact loader", + ) + + def test_compute_rejects_with_a_client_error(self): + self.assertIn("status_code=400", self._compute_source()) + + +class TestLoadJsonDoesNotLeakFileContents(unittest.TestCase): + def test_error_omits_file_contents(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "secret.env") + # Stands in for whatever the file happens to contain; it only needs + # to be a distinctive, non-JSON string that must not be echoed back. + canary = "CANARY-must-not-appear-in-the-error-message" + with open(path, "w") as handle: + handle.write(canary) + + with self.assertRaises(RuntimeError) as ctx: + load_json(path) + + message = str(ctx.exception) + self.assertNotIn(canary, message) + + def test_error_still_identifies_the_file_and_position(self): + """The message must stay useful for debugging a broken catalog file.""" + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "broken.json") + with open(path, "w") as handle: + handle.write('{"a": 1,,}') + + with self.assertRaises(RuntimeError) as ctx: + load_json(path) + + message = str(ctx.exception) + self.assertIn("broken.json", message) + self.assertIn("line", message) + + +class TestArtifactFilePathLoadingStillWorks(unittest.TestCase): + """The library feature CodeQL's sink sits on must not regress.""" + + def test_fetch_artifact_still_loads_from_a_file_path(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "my_metric.json") + with open(path, "w") as handle: + json.dump({"__type__": "accuracy"}, handle) + + artifact, catalog = fetch_artifact(path) + self.assertEqual(type(artifact).__name__, "Accuracy") + self.assertIsNone(catalog) + + def test_fetch_artifact_still_loads_from_a_catalog_name(self): + artifact, catalog = fetch_artifact("metrics.accuracy") + self.assertIsNotNone(artifact) + self.assertIsNotNone(catalog) + + def test_load_json_still_parses_valid_json(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "ok.json") + with open(path, "w") as handle: + json.dump({"__type__": "accuracy", "n": 1}, handle) + + self.assertEqual(load_json(path), {"__type__": "accuracy", "n": 1}) + + +if __name__ == "__main__": + unittest.main()