fix: preserve Flax routing across ambiguous prefixes#1379
Conversation
Performance BenchmarksCompared
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac7b8e7132
ℹ️ 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".
…ax-post-merge-review # Conflicts: # modelaudit/utils/file/detection.py # tests/utils/file/test_filetype.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8d54aa3fe
ℹ️ 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".
Address review feedback for bounded Flax routing across skipped suffixes and pickle-shaped overlaps while preserving strict owner precedence.
…ax-post-merge-review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f8115e171
ℹ️ 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".
| (include_unvalidated_pickle and _looks_like_binary_pickle_protocol(prefix[:4])) | ||
| or _has_bounded_binary_pickle_security_signal(pickle_probe_sample) | ||
| or _looks_like_proto0_or_1_pickle( |
There was a problem hiding this comment.
When core.py falls back from a Flax-routed file because flax_msgpack is excluded or the user selected only pickle, it calls detect_flax_msgpack_overlap_routes(path) with the default include_unvalidated_pickle=False. That means a binary pickle prefix whose dangerous opcode is beyond PROTO0_1_MAX_PROBE_BYTES (the new tests cover this for default scans) is not returned here, so scan_file(..., {"scanners": ["pickle"]}) skips the pickle scanner entirely even though the same payload is scanned as a pickle overlap when Flax is enabled. Please keep binary pickle-shaped overlaps available for the scanner-selection fallback, or otherwise run the pickle scanner in that selected-scanner path without surfacing parser-only fallout.
Useful? React with 👍 / 👎.
1 participant
Summary
Review Context
This addresses the unresolved review findings raised after #1280 had already been merged:
mainTwo independent review passes validated the actionable paths. One rejected an initial simplification that would have silently skipped a JSON-looking MessagePack prelude with a later malicious Flax object; this PR retains that case as incomplete coverage instead.
Validation
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/PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest -n auto -m "not slow and not integration" --maxfail=1(6352 passed, 16 skipped)663 passed)