Skip to content

fix: harden manifest parse boundaries#922

Open
mldangelo-oai wants to merge 2 commits intomainfrom
mdangelo/codex/manifest-boundary-audit
Open

fix: harden manifest parse boundaries#922
mldangelo-oai wants to merge 2 commits intomainfrom
mdangelo/codex/manifest-boundary-audit

Conversation

@mldangelo-oai
Copy link
Copy Markdown
Contributor

Summary

This PR hardens the manifest scanner boundary uncovered in the repo-wide correctness audit. Parsed top-level manifest arrays were accepted as successful scans but skipped the structured model-name, URL, and weak-hash checks, so array-shaped model configs could hide weak integrity hashes or untrusted URLs. Malformed or scalar manifest roots also completed successfully even though structured analysis never ran.

Root Cause

Manifest parsing used truthiness and dict-only checks as the control-flow gate. Non-empty lists were truthy but ignored because downstream analysis only ran for dict roots. Empty values, malformed files, and scalar roots were collapsed into a soft debug parse attempt rather than an explicit incomplete scan outcome. The parser also advertised TOML and INI extensions without structured parsing support.

Changes

  • Add an explicit parse-failed sentinel so JSON null and other scalar roots are not confused with parse errors.
  • Traverse parsed dict and list roots for model-name policy checks, URL allowlist checks, and weak hash detection.
  • Mark unparseable or unsupported-root manifests as scan_outcome=inconclusive and fail closed unless a recovered WARNING/CRITICAL finding already exists.
  • Add structured TOML and INI parsing for supported manifest extensions, gracefully failing closed if TOML support is unavailable on older Python runtimes.
  • Add regression coverage for top-level list weak hashes, top-level list untrusted URLs, malformed configs, scalar roots, recovered blacklist findings, TOML, and INI manifests.
  • Update the Unreleased changelog.

Validation

  • uv run pytest tests/scanners/test_manifest_scanner.py: 48 passed
  • 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/: 432 source files clean
  • uv run pytest -n auto -m "not slow and not integration" --maxfail=1: 2437 passed, 1100 skipped, 16 warnings
  • uv run ruff check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • uv run ruff format --check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • git diff --check

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 10, 2026

Warning

Rate limit exceeded

@mldangelo-oai has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 18 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 18 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: cd5c7303-0d66-48b6-84d8-ab3d3d2e1321

📥 Commits

Reviewing files that changed from the base of the PR and between f285a05 and bd800cb.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • modelaudit/scanners/manifest_scanner.py
  • tests/scanners/test_manifest_scanner.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mdangelo/codex/manifest-boundary-audit

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mldangelo-oai mldangelo-oai enabled auto-merge (squash) April 10, 2026 20:50
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 10, 2026

Workflow run and artifacts

Performance Benchmarks

Compared 6 shared benchmarks with a regression threshold of 15%.
Status: 1 regressions, 0 improved, 5 stable, 0 new, 0 missing.
Aggregate shared-benchmark median: 732.33ms -> 679.20ms (-7.3%).

Top regressions:

  • tests/benchmarks/test_scan_benchmarks.py::test_detect_file_format_safe_pickle +37.8% (165.3us -> 227.8us, safe_model.pkl, size=49.4 KiB, files=1)
Benchmark Target Size Files Baseline Current Change Status
tests/benchmarks/test_scan_benchmarks.py::test_detect_file_format_safe_pickle safe_model.pkl 49.4 KiB 1 165.3us 227.8us +37.8% regression
tests/benchmarks/test_scan_benchmarks.py::test_scan_duplicate_directory duplicate-corpus 840.0 KiB 81 519.74ms 480.32ms -7.6% stable
tests/benchmarks/test_scan_benchmarks.py::test_scan_safe_pickle safe_model.pkl 49.4 KiB 1 30.19ms 28.02ms -7.2% stable
tests/benchmarks/test_scan_benchmarks.py::test_scan_pytorch_zip state_dict.pt 1.5 MiB 1 36.67ms 34.18ms -6.8% stable
tests/benchmarks/test_scan_benchmarks.py::test_scan_mixed_directory mixed-corpus 1.7 MiB 54 145.52ms 136.39ms -6.3% stable
tests/benchmarks/test_scan_benchmarks.py::test_validate_file_type_pytorch_zip state_dict.pt 1.5 MiB 1 47.4us 49.4us +4.2% stable

Copy link
Copy Markdown

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

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: f32cd81066

ℹ️ 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 on lines 767 to 777
if ext in [".ini", ".cfg"] or (ext == ".config" and not stripped_content.startswith(("{", "["))):
return self._parse_ini_file(content)

# Try JSON format first
if ext in [
".json",
".manifest",
".model",
".metadata",
] or content.strip().startswith(("{", "[")):
] or stripped_content.startswith(("{", "[")):
return json.loads(content)
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 Handle .config INI sections before JSON parsing

Update _parse_file parsing order for .config: the current guard not stripped_content.startswith(("{", "[")) skips _parse_ini_file for valid INI files that begin with [section]. Those files then go through json.loads, fail, and are marked parse-failed/inconclusive, so structured URL/hash checks never run on valid .config INI manifests.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

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

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: bd800cb5f7

ℹ️ 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".


def _parse_ini_file(self, content: str) -> dict[str, Any]:
"""Parse INI-style manifests into nested dictionaries."""
parser = configparser.ConfigParser()
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 Disable interpolation when parsing INI manifests

_parse_ini_file builds a default ConfigParser, which enables % interpolation. Valid INI values like percent-encoded URLs (...%2F...) raise InterpolationSyntaxError, causing _parse_file to return parse-failed. The scan is then marked inconclusive and structured URL/hash checks never run on valid manifests.

Useful? React with 👍 / 👎.

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