[yoavkatz]

Remediates the 4 open py/path-injection CodeQL alerts (HIGH, CWE-22) from the IBM OSPO security notification (#1994): cs-12, cs-13, cs-14 (artifact.py:238,240) and cs-18 (utils.py:222).

Why the alerts were still open

The previous fix (#1993) blocked the attack at runtime, but 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):   # checks requested_name
    raise HTTPException(status_code=400, ...)
...
metric = ArtifactFetcherMixin.get_artifact(metric_name)   # passes the 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 that 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/cs-16 at the same sink: the flow survived, only its line numbers moved.

The fix

Makes the sanitization load-bearing 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.

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. 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.

Two things worth a reviewer's attention

• Re-serializing the overrides into the name would have been a bug. The parser returns typed values, so [n=[1,2]] and nested artifact references do not round-trip through a string. That is why overrides are passed separately rather than rebuilt into the identifier.
• Fixes a latent 500. Malformed brackets (metrics.accuracy[unclosed) previously raised a ValueError that reached the endpoint's broad except and became a 500; it is now a clean 400.

Verification

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/library/test_path_injection_service.py, extended to 20 tests. Three 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 this regression 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.

Lint: ruff 0.1.6 (the version pinned in .pre-commit-config.yaml) check and format, codespell, and the relative-imports hook all pass. Note that a newer local ruff reformats ~250 lines of unrelated code in operators.py/artifact.py, so the pinned version is the one to check against.

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

Closes #1994

🤖 Generated with Claude Code