Skip to content

fix: enforce MLflow artifact backend allowlist - #1564

Merged
mldangelo-oai merged 5 commits into
mainfrom
mdangelo/codex/fix-mlflow-uri-allowlist
Jun 9, 2026
Merged

mldangelo-oai merged 5 commits into
mainfrom
mdangelo/codex/fix-mlflow-uri-allowlist

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • require every concrete non-local MLflow artifact target to match MODELAUDIT_MLFLOW_ALLOWED_ARTIFACT_URIS
  • preserve current-main private staging-root identity, containment, symlink, special-file, hardlink, and entry-budget validation
  • bind downloads to the exact repositories validated during preflight, including run and logged-model overlays
  • securely compose hardened local artifacts with allowlisted remote overlays
  • validate remote artifact listings before destination creation and use guarded per-file downloads for standard MLflow repositories
  • fail closed on malformed/ambiguous URIs, unsafe artifact paths, unknown custom-scheme authority changes, remote file authorities, UNC/device roots, and drive-relative paths
  • skip a missing optional run overlay only when structured repository metadata shows it has no files; all other target failures remain fatal

Security review

  • closes plain, encoded, repeated-separator, and artifact-listing traversal bypasses
  • prevents delegated download results and multi-target destinations from escaping ModelAudit staging
  • prevents one target from planting a link that redirects a later target outside staging
  • scans the composed staging root whenever multiple delegated targets contribute
  • keeps scoped run and logged-model overlays in the same scanned tree
  • avoids MLflow recursive-download listing swaps by downloading the validated file plan directly for standard repositories
  • preserves raw authorities for unknown/custom URI schemes while normalizing known network schemes
  • accepts valid bracketed IPv6 loopback file URIs and rejects malformed unbracketed forms
  • preserves legitimate MLflow 3 logged-model scans with absent run-side overlays without parsing exception strings

Validation

  • Head: f56567b4374e95cdce48773e24b00ff8daec66a2
  • Base merged: 21956abfe97ec8312bb4f6312b5da99ec0fa640e
  • Exact published tree: 4f6a61017b696f533136eec1bb197bcc3b5e9d2b
  • tests/integrations/test_mlflow_integration.py: 179 passed
  • MLflow CLI slice: 12 passed, 244 deselected
  • scoped mypy across the changed implementation/tests: clean
  • Ruff format/check and git diff --check: clean
  • full local suite intentionally not run; CI owns broader coverage
  • all 12 existing review threads resolved

@github-actions

github-actions Bot commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Workflow run and artifacts

Performance Benchmarks

Compared 12 shared benchmarks with a regression threshold of 15%.
Status: 0 regressions, 0 improved, 12 stable, 0 new, 0 missing.
Aggregate shared-benchmark median: 1.412s -> 1.428s (+1.2%).

Workload Benchmark Target Size Files Baseline Current Change Status
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_raw] nested_raw 78 B 1 499.9us 462.7us -7.4% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_hex] nested_hex 130 B 1 528.0us 493.7us -6.5% stable
mixed-model-repository tests/benchmarks/test_scan_benchmarks.py::test_scan_release_candidate_repository release-candidate 547.3 KiB 32 473.63ms 500.43ms +5.7% stable
warm-cache-rescan tests/benchmarks/test_scan_benchmarks.py::test_scan_warm_cached_repository_rescan release-candidate 547.3 KiB 32 96.23ms 91.60ms -4.8% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_base64] nested_base64 98 B 1 489.1us 469.9us -3.9% stable
padded-multi-stream-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_padded_multi_stream_upload multi_stream_padded 4.1 KiB 1 540.1us 523.0us -3.2% stable
direct-malicious-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_direct_malicious_upload malicious_reduce 52 B 1 445.9us 438.7us -1.6% stable
duplicate-heavy-registry tests/benchmarks/test_scan_benchmarks.py::test_scan_duplicate_registry_snapshot registry-snapshot 915.2 KiB 13 399.33ms 395.99ms -0.8% stable
clean-training-checkpoint tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_clean_training_checkpoint safe_large 278.2 KiB 1 111.38ms 110.61ms -0.7% stable
chunked-upload-stream tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_chunked_upload_stream chunked_stream 278.2 KiB 1 114.20ms 113.57ms -0.6% stable
suspicious-pickle-intake tests/benchmarks/test_scan_benchmarks.py::test_scan_suspicious_pickle_intake suspicious-intake 183.8 KiB 4 141.79ms 141.11ms -0.5% stable
single-checkpoint-preflight tests/benchmarks/test_scan_benchmarks.py::test_scan_single_checkpoint_before_load single_checkpoint.pkl 183.0 KiB 1 72.60ms 72.31ms -0.4% stable

@mldangelo-oai
mldangelo-oai marked this pull request as ready for review June 8, 2026 21:14

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 824b057962

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread modelaudit/integrations/mlflow.py
Comment thread modelaudit/integrations/mlflow.py Outdated
Comment thread modelaudit/integrations/mlflow.py Outdated

mldangelo-oai commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor Author

@codex Please publish the already-validated local repair for all six unresolved review threads on this PR. The exact current-main local head is 41e3829b (original PR head 824b0579, prior thread fix f98d8dbd, additional separator fix 6e75a848, then merge current main at f856f9c3).

Required behavior:

  • Validate percent escapes, decode remote paths once, reject ambiguous double encoding, NULs, backslashes, and ./.. segments before segment-aware prefix matching.
  • Preserve repeated path separators instead of collapsing them: s3://trusted-bucket/models//safe/... and %2fsafe/... must not match s3://trusted-bucket/models/safe. This closes an additional object-key allowlist bypass found during follow-up review.
  • Normalize omitted HTTP/HTTPS ports to 80/443, while rejecting malformed explicit ports, URI userinfo, and malformed bracketed hosts.
  • Unwrap transparent model repositories and enforce RunsArtifactRepository trust against the resolved backend repo.artifact_uri, not the runs:/ wrapper.
  • Keep the focused regressions in tests/integrations/test_mlflow_integration.py, including direct and end-to-end traversal and repeated-separator cases.

Post-merge local validation: 66 passed for tests/integrations/test_mlflow_integration.py; the targeted CLI trust-refusal test passed; scoped Ruff check/format, mypy, and git diff --check are clean. The shell GitHub credential still rejects direct push. Do not weaken the fail-closed cases.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 824b057962

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread modelaudit/integrations/mlflow.py Outdated
Comment thread modelaudit/integrations/mlflow.py
Comment thread modelaudit/integrations/mlflow.py Outdated
@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

Testing

  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/integrations/test_mlflow_integration.py -q — 62 passed, 1 skipped.
  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/test_cli.py::test_scan_mlflow_uri_trust_refusal_is_not_recorded_as_completed -q
  • ✅ uv run ruff format modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ uv run ruff check --fix modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ uv run mypy modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ git diff --check
  • ❌ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest -n auto -m "not slow and not integration" --maxfail=1 — I terminated the full xdist lane after it progressed slowly to ~22% with one early failure marker but no failure details emitted; targeted MLflow and CLI regressions passed.

Committed changes as 019517e and recorded the follow-up PR metadata.

View task →

@mldangelo-oai mldangelo-oai changed the title fix: require mlflow artifact uri allowlist fix: enforce MLflow artifact backend allowlist Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Published remote head 99f3ff84 with all review fixes and additional false-positive/false-negative hardening.

Key follow-ups beyond the original threads:

  • validates both run and logged-model artifact backends
  • downloads through the exact repository objects approved during preflight, eliminating repository and logged-model re-resolution TOCTOU
  • preserves raw object-store key separators instead of normalizing to a different key
  • rejects logical wrapper URIs, percent-encoded remote paths, malformed/missing authorities, and incomplete overlay discovery
  • fails closed for mixed local/remote overlays until the hardened local copier can be composed safely

Validation: 76 MLflow integration tests, 3 affected CLI tests, scoped Ruff/mypy, Prettier, and diff check all pass. Resolving the six threads now; CI can continue asynchronously.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 99f3ff8462

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread modelaudit/integrations/mlflow.py Outdated

Copy link
Copy Markdown
Contributor Author

Addressed the P1 fail-closed review finding in published head a65c59829134457a0d232adc168d2d9889e64259.

Every delegated run/logged-model artifact target is now mandatory: the first exception, non-string path, None, or empty path aborts immediately, so a later successful target cannot mask an incomplete download. Added ordering regressions for failures in the first and later targets plus missing/empty returned paths.

Focused validation:

  • MLflow integration: 80 passed
  • CLI -k mlflow: 10 passed, 216 deselected
  • scoped Ruff, format, mypy, and diff checks: clean

The inline thread is resolved. CI is running on the new head; continuing the PR queue per the audit workflow.

Copy link
Copy Markdown
Contributor Author

Addressed the incomplete-scan review finding in a65c59829134457a0d232adc168d2d9889e64259.

  • Every delegated MLflow artifact target is now mandatory: an exception, None, an empty path, or a non-string path fails closed immediately.
  • A later target is no longer attempted after an earlier target fails, preventing a later success from masking an incomplete scan.
  • Added regressions for first-target failure, later-target failure, and missing returned paths.

Focused validation:

  • tests/integrations/test_mlflow_integration.py: 80 passed
  • tests/test_cli.py -k mlflow: 10 passed, 216 deselected
  • scoped Ruff, format check, mypy, and diff checks: clean

Published tree: 2002e70a9908fbfcc2352f7700b0a61f3cc53205.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a65c598291

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread modelaudit/integrations/mlflow.py
Comment thread modelaudit/integrations/mlflow.py Outdated
@mldangelo-oai
mldangelo-oai force-pushed the mdangelo/codex/fix-mlflow-uri-allowlist branch 2 times, most recently from d034060 to 2dbdcac Compare June 9, 2026 06:13

Copy link
Copy Markdown
Contributor Author

Addressed both new review threads in 2dbdcacc. Trust evaluation now joins each validated repository URI with its delegated artifact_path, so exact object/subpath allowlists work while sibling objects remain untrusted. Mixed local-run/remote-model overlays are also supported without bypassing the hardened local copy path; the reverse remote-then-local order explicitly fails closed to preserve overlay precedence. Merged-current-main MLflow validation: 138 integration tests and 11 CLI tests passed; exact tree c9aee8635dae240bb10bb7cc56990fffe5be6990.

Copy link
Copy Markdown
Contributor Author

Addressed both remaining review findings in live head 2dbdcacc9affbb662b04f84f0f88424b918a8f26 (exact tree c9aee8635dae240bb10bb7cc56990fffe5be6990). Delegated artifact paths are included in URI trust checks, so exact object/subpath entries work without broadening the allowlist. Local-first mixed overlays now use ModelAudit's hardened local copier while only remote targets require allowlisting; remote-first/local-later ordering still fails closed to preserve overlay precedence. Validation: 138 MLflow integration tests and 11 MLflow CLI tests passed; scoped Ruff, format, mypy, and diff checks are clean.

@mldangelo-oai
mldangelo-oai force-pushed the mdangelo/codex/fix-mlflow-uri-allowlist branch from 2dbdcac to ea4841a Compare June 9, 2026 09:05

Copy link
Copy Markdown
Contributor Author

Critical audit follow-up published at ea4841ad5feff418b64edc78ecb831d496728373.

The review found an additional non-local fallback bypass: MLflow classifies remote file://host/share repositories as LocalArtifactRepository. On Windows, resolving that path can initiate SMB access and leak credentials before the artifact allowlist is evaluated; on POSIX, MLflow may silently discard the hostile authority. The branch now rejects remote file authorities and UNC/device roots before path lookup or resolution. It also normalizes SFTP's default port so equivalent allowlist entries do not create a benign false positive.

Validation: the complete associated MLflow/CLI slice passes (155 passed, 216 deselected), scoped Ruff/format/mypy and diff checks are clean, and all 9 review threads are resolved. CI is intentionally left running while the audit moves to the next randomly selected PR.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ea4841ad5f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread modelaudit/integrations/mlflow.py Outdated
Comment thread modelaudit/integrations/mlflow.py Outdated
@mldangelo-oai
mldangelo-oai marked this pull request as draft June 9, 2026 09:53
@mldangelo-oai
mldangelo-oai requested a review from mldangelo June 9, 2026 10:18
@mldangelo-oai
mldangelo-oai marked this pull request as ready for review June 9, 2026 10:19
@mldangelo-oai
mldangelo-oai enabled auto-merge (squash) June 9, 2026 10:19
@mldangelo-oai
mldangelo-oai marked this pull request as draft June 9, 2026 10:19
auto-merge was automatically disabled June 9, 2026 10:19

Pull request was converted to draft

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0ba1688c3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread modelaudit/integrations/mlflow.py Outdated

Copy link
Copy Markdown
Contributor Author

Critical audit complete and published in f56567b4374e95cdce48773e24b00ff8daec66a2 (exact tree 4f6a61017b696f533136eec1bb197bcc3b5e9d2b).

Additional issues found and fixed during current-main integration:

  • preserved main's staging-root identity and full tree validation for delegated downloads
  • blocked multi-target symlink destination escapes before a later backend can write
  • fixed scoped run/logged-model overlays being split across scanned and unscanned trees
  • rejected unsafe remote artifact listings before destination creation
  • bypassed MLflow's recursive downloader for standard repositories by downloading the validated per-file plan
  • preserved raw authority semantics for unknown/custom schemes
  • fixed bracketed IPv6 loopback file URI handling
  • skipped optional missing run overlays from structured listing state instead of fragile exception-string parsing

Focused validation: 179 MLflow integration tests, 12 MLflow CLI tests, scoped mypy, Ruff, and diff checks all clean. The full local suite was intentionally left to CI per review instructions. All existing inline threads remain resolved.

@mldangelo-oai
mldangelo-oai marked this pull request as ready for review June 9, 2026 16:14
@mldangelo-oai
mldangelo-oai merged commit 74ebeae into main Jun 9, 2026
23 of 27 checks passed
@mldangelo-oai
mldangelo-oai deleted the mdangelo/codex/fix-mlflow-uri-allowlist branch June 9, 2026 16:15

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f56567b437

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

not validated_path
or posixpath.isabs(validated_path)
or ntpath.isabs(validated_path)
or ntpath.splitdrive(validated_path)[0]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject Windows ADS paths before delegated downloads

On Windows, this only rejects drive-qualified paths, so an artifact path like model.pkl:payload still reaches download_artifacts() for an allowlisted non-standard repository. Such a downloader can write an NTFS alternate data stream under staging; the later directory validation/walk will not enumerate that stream, so the scan can pass without inspecting the downloaded payload. Reject : in delegated artifact paths on Windows before delegating the download.

Useful? React with 👍 / 👎.

Comment on lines +2089 to +2090
if target.optional_when_missing and not artifact_paths:
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fail closed on empty mandatory MLflow listings

If a mandatory allowlisted standard MLflow target lists zero files, this falls through and _download_validated_mlflow_files() returns an empty staging dir, so an absent or transiently empty listing is scanned as clean without downloading artifacts. Fail closed unless optional. guidance

Useful? React with 👍 / 👎.

@github-actions github-actions Bot mentioned this pull request Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant