[yoavkatz]

Summary

Remediates the 5 open py/path-injection CodeQL alerts (HIGH, CWE-22) from the IBM OSPO security notification in #1991 — alerts cs-12, cs-13, cs-14, cs-15, cs-16.

CodeQL reports five alerts but traced one 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 cause is a mismatch of expectations. fetch_artifact() is documented to accept either a catalog name or a local file path, which is right for library callers. The service 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 is exploitable, not just a taint-tracking artifact

Both were verified against the unpatched code:

• 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 let any token holder read arbitrary files readable by the service account — a clear privilege escalation over computing a metric.

The fix

Two independent halves, so neither relies 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; a bad value returns HTTP 400 instead of 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 like metrics.f1_macro[average=macro] keep working.

The check sits before the function's try, so the 400 reaches the existing HTTPException handler rather than being converted to 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). That echo is what turned a path into a file-read primitive, and removing it also protects any other caller passing an untrusted path. The decoder's own message is kept, so errors still name the file and the failing line/column — what you actually need to debug a malformed catalog entry. No test or caller depended on the old "with file content:" wording.

What is deliberately not changed

fetch_artifact()'s support for loading an artifact from a file path stays intact. It's 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 library capability.

Testing

tests/library/test_path_injection_service.py (13 tests). Four fail against the unpatched code: the two disclosure tests, plus two asserting the endpoint wires the validator in before get_artifact() and answers 400 — checked at the source level because the endpoint can't be called where the optional fastapi dependency is absent.

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. 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 and come from optional dependencies missing in the local environment.

Closes #1991

🤖 Generated with Claude Code

[yoavkatz]

CI status analysis

All checks relevant to this change pass:

Check	Result
Analyze (python) — CodeQL	✅ pass
CodeQL	✅ pass
unittests	✅ pass
quality (lint/format)	✅ pass
consistency, docs, eager, DCO	✅ pass
preparation shards 1–6	✅ pass

There are six red results. None of them is attributable to this change:

1. performance — urllib.error.HTTPError: HTTP Error 429: Too Many Requests from a dataset host. It fails inside the step named "Run performance on main branch", i.e. while profiling main, before this branch's code is exercised at all.
2. helm — TypeError: as_dataset() got an unexpected keyword argument 'verification_mode'. This is the known upstream datasets incompatibility already described in the commit message of Security: Fix CodeQL alerts for ReDoS, path injection, and workflow permissions #1984.
3. preparation (7), (8), (9) — cancelled, not failed. All three ran 30m19s–30m52s against the workflow's timeout-minutes: 30, so they were killed by the timeout rather than failing an assertion (--log-failed is empty for all three).
4. preparation (0) — a SQL metric mismatch: sqlglot_optimized_equivalence returned 0.0 where 0.38 was expected. Every other key in that score dict matches exactly; this is an environment-dependent SQL optimizer result.
5. coverage/coveralls — reported as fail while its own message reads "Coverage increased (+0.03%) to 78.346%", and coverage/coveralls/patch passes ("No coverable files changed"). A threshold-configuration quirk, not a regression.

Why these are independent of this diff

Two checks, rather than inference from the failure text alone:

• The metrics service is not reachable from any failing job. src/unitxt/service/metrics/main.py is imported nowhere outside this PR's own test, and that test only reads the file's source as text. No catalog-preparation, performance or helm job can touch it.
• load_json is byte-for-byte equivalent on real input. It is the only other shared code touched here. Running it over all 4,923 files under src/unitxt/catalog/ on this branch and on clean main gives an identical SHA-256 digest (062877e9…) with zero errors on both. The change only alters the error-message text for malformed JSON — a path that never executes in CI.

Caveat on method

A true apples-to-apples comparison was not available: these workflows are PR-triggered (no recent main runs to compare), and the other open PRs predate CI being re-enabled, so they carry no checks. The conclusion above rests on the failure signatures plus the code-reachability and digest checks, not on a green control run.

The performance 429 and the preparation timeouts match the upstream rate-limiting that #1984 also had to work around, so a re-run would likely clear several of these.

🤖 Generated with Claude Code

[yoavkatz]

Correction and re-run results

I said above that a re-run "would likely clear several of these." That was right for one job and wrong for preparation (0) — correcting it here.

preparation (0) is reproducible, not flaky. It failed identically on both runs: sqlglot_optimized_equivalence returns 0 where 1 is expected, on exactly instances 1, 4 and 7, with every other key in the score dict matching. Two identical runs make this deterministic, so it needs a decision rather than another retry.

Root cause is a library-version dependency, not this PR. sqlglot_optimized_equivalence (src/unitxt/text2sql_utils.py:597) compares queries through sqlglot.optimizer.optimize, whose output varies between sqlglot releases, and the expected values stored in prepare/metrics/text2sql_accuracy.py were produced under a different version. Calling that function directly on this branch and on clean main gives identical results, so the diff here does not influence it.

performance failed again on the same urllib.error.HTTPError: HTTP Error 429: Too Many Requests, again inside the "Run performance on main branch" step — upstream dataset-host rate limiting, hit while profiling main.

preparation (9) failed differently the second time, in 44s instead of timing out: the "Install internal repo via pip (SSH)" step (MLT/LLMEvalKit from github.ibm.com) failed, and Run Tests was skipped entirely, so no test code ran. It is also not in the required-checks list.

Where that leaves the merge

preparation (0) and preparation (7) are both required contexts. (0) is red for a pre-existing, version-dependent reason unrelated to this change, so it will not go green on a retry — clearing it needs either a sqlglot pin / refreshed expected values (separate change), or an explicit decision to merge past it.

🤖 Generated with Claude Code