[yoavkatz]

The 4 py/path-injection alerts (HIGH, CWE-22) from #1994 are still open after #1995, and are reported again in #1996: cs-12, cs-13, cs-14, cs-18. CodeQL analysed the merge commit (603f1c5ce, analysis 2093723778) and still found all four.

#1995 was wrong, and overstated its own guarantee

Its commit message claimed the rebuilt name "shares nothing with the raw input." As dataflow that is false:

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

split() and join() propagate taint, so the returned string is still derived from the request. The re.match guard sits in a separate if, which CodeQL does not treat as sanitizing the returned value — and it is right not to: a value rebuilt from the input is only ever as good as the checks applied to it.

The post-merge SARIF shows the flow running straight through the new helper and out the other side. All four sinks route through the join at artifact.py:112:

service/metrics/main.py:69,85   metric_name (FastAPI path parameter)
artifact.py:70,97               identifier -> separate_inside_and_outside...
artifact.py:108,112             segments -> SEP.join(segments)
service/metrics/main.py:85,107  safe_metric_name
operators.py:1193,1212          artifact_identifier
artifact.py:612,628             artifact_rep
artifact.py:303-306             open(path)            <- cs-12, cs-13, cs-14
utils.py:221-222                open(path)            <- cs-18

The fix

Stop deriving the value from the request at all. sanitize_catalog_identifier() now resolves the requested name against the names the local catalogs publish and returns the catalog's own string, so what reaches a loader originates in trusted data:

canonical = _lookup_catalog_name(name)
if canonical is None:
    raise IllegalCatalogNameError(identifier)
return canonical, args

The returned object is the one held in the catalog index, not the caller's — asserted by identity in the tests, which is what distinguishes this from #1995. The syntactic checks are kept as a cheap pre-filter, but they are no longer what the guarantee rests on.

Supporting changes:

• _catalog_name_index() builds {name: name} from catalog.ls(), cached via lru_cache_decorator and keyed on a generation counter. Lookups are ~1µs with no filesystem walk per request (2000 in 2.2ms); the walk itself is ~35ms for 4923 entries.
• add_to_catalog() calls invalidate_catalog_name_index(), so an artifact saved at runtime stays resolvable in the same process.
• IllegalCatalogNameError deliberately does not distinguish "malformed" from "not published", so the error cannot be used to probe which entries exist.

Deliberate behaviour change — worth a reviewer's attention

The metrics service now rejects a well-formed name that the catalogs do not publish, where before it would attempt to load it. A name published only by the remote GithubCatalog is also rejected, because ls() walks local catalogs only.

This was chosen over consulting the remote catalog, which would add an HTTP request per lookup and would once again vouch for a name rebuilt from the request. 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. Documented on _catalog_name_index() and pinned by a test, so a future change here is a decision rather than an accident.

Also corrects the inaccurate "shares nothing with the raw input" rationale left in the sanitizer's docstring by #1995.

Verification

The arbitrary-file read, the planted-artifact load, traversal, Windows paths, URLs, null bytes and malformed brackets are all refused with 400, with no file content in the error. metrics.accuracy, metrics.rouge, metrics.f1_macro and metrics.f1_macro[ignore_case=False] all still resolve and the override applies.

23 tests in tests/library/test_path_injection_service.py (up from 20). The new ones assert the returned name is the catalog's string object and not the caller's, that unpublished names are rejected, and that the index matches the local walk.

tests/library: 802 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 tree and come from optional dependencies missing locally. ruff 0.1.6 (the pinned pre-commit version) check and format clean.

Note on CI: performance, helm, preparation (0) and coverage/coveralls are red on main for unrelated reasons (a dataset 429, a datasets API drift in HELM, a SQL-metric assertion, and a Coveralls gate that fails on rising coverage), so expect those here too.

Reported by: IBM OSPO security scanning (CodeQL default setup)

Closes #1996

🤖 Generated with Claude Code