fix(scanner): report unreadable model files as errors, not clean LOW-risk artifacts (#131) - #115
Conversation
…risk artifacts (#131) `LOW` is an assertion: AIsbom opened the artifact and found nothing dangerous. For a file no parser could read, the honest answer is different — and collapsing the two meant a truncated download passed a CI gate as a clean model. Five inspectors already recorded their own parse failure in `meta["error"]` and nothing in the codebase ever read it. Two corruption shapes never raise at all (a text `.pt`, a two-byte pickle), and a git-LFS pointer set that key to the empty string, so a truthiness check would still have missed it. Each inspector now reports whether its parse actually succeeded. Unreadable files land in `results["errors"]` with a closed-set subtype (LfsPointer, EmptyFile, TruncatedStream, UnrecognizedFormat), print in their own "Could not read" section, and exit 1 — which is what the README's exit-code table has always documented for "a file failed to parse". The component stays in the SBOM so an auditor sees the file was present, but carries no framework label and no risk verdict, marked with `aisbom:unreadable` properties. Dropping the label is what stops `aisbom:format` claiming 200 random bytes are SafeTensors. `aisbom score` refuses to grade such a scan via the existing #114 gate. Also fixed, as the same class of problem: - The three `UNKNOWN (...)` verdicts (GGUF invalid header, Keras unrecognized container, unparsable ONNX) were honest but scored 0 in `_risk_score` — below LOW — so those files exited 0 and rated safer than a clean model. - Any text file in `.pt`/`.pth`/`.bin` was classified as a Python path config, so an HTML error page saved over a checkpoint scored LOW. That classification is now `.pth`-only and validated against the format's actual spec. - A SafeTensors header longer than the file is rejected before the read, replacing an internal OverflowError with a message naming the problem. A real-tree probe drove one design decision: requiring the pickle opcode walk to reach STOP flagged 75 valid files — every `.pkl` in joblib's own test corpus — because joblib appends array data as raw bytes after STOP. The rule is "nothing past the protocol header disassembled" instead, and that file shape is now a regression test. The probe over 35,283 artifacts (141 real `.pth` files, a node_modules tree, an HF cache, a generated valid-model corpus) reports zero false positives. BREAKING: a tree containing unreadable files now exits 1 where it exited 0. Called out at the top of the changelog entry and in a README upgrade note.
![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: 9440625d53
ℹ️ 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 not parsed and local_path is not None: | ||
| self._mark_unreadable( |
There was a problem hiding this comment.
For HTTP/Hugging Face scans, source is a RemoteStream, so local_path is always None and this condition skips the new error path. For example, a remote .safetensors containing random or truncated bytes returns with framework="SafeTensors", risk_level="LOW", and only an internal meta["error"]; nothing is appended to results["errors"], so the CLI still exits 0 and reports the remote artifact as clean. The remote dispatch also bypasses _inspect_claimed, so it needs equivalent unreadable recording using the artifact name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a9c85c9 — correct, and the most important of the three.
Every unreadable recording was guarded on local_path is not None, so hf:// and HTTPS scans got none of the fix. All six inspectors now finalize through one shared _finalize_inspection, identified by local_path or name, so local and remote are reported identically.
The git-LFS subtype survives on the remote path too, which matters more than I first credited: raw.githubusercontent.com serves the pointer text rather than the file for anything LFS-tracked, so a plain HTTPS scan of a GitHub-hosted model hits it directly. The head is re-read only after a parse has already failed, so the extra Range request is never paid by a healthy file.
Covered by test_remote_random_bytes_are_not_reported_clean and test_remote_lfs_pointer_names_the_remedy.
| precheck = self._unreadable_precheck(path) | ||
| if precheck is not None: | ||
| return self._unreadable_stub(path, *precheck) | ||
| return inspector(path) |
There was a problem hiding this comment.
When a claimed local file passes the precheck but its inspector catches an open or parse exception, this wrapper returns the metadata without consulting meta["error"]. The PyTorch, pickle, GGUF, Keras, and ONNX inspectors catch such exceptions internally without appending to self.errors; GGUF/Keras/ONNX also retain their initial LOW verdict, so an unreadable file—such as one that cannot be opened after the precheck—can still produce a clean exit. Check the returned metadata here and route caught failures through _mark_unreadable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a9c85c9 — correct. _finalize_inspection now picks up meta["error"] for every inspector, local and remote.
One detail worth recording: the check is key membership, not truthiness. A git-LFS pointer reaching the SafeTensors parser sets meta["error"] to the empty string, so if meta.get("error") would still have missed that case. Pinned by test_empty_string_error_is_still_caught.
I added a guard your comment did not ask for but which this change makes necessary: a verdict already reached is never overwritten. An inspector can find a CRITICAL payload and then throw — a truncated zip whose first member disassembles to os.system, say — and relabelling that "unreadable" would drop the finding, which is worse than the bug this PR fixes. Only the seeded UNKNOWN/LOW placeholders give way. Pinned by test_caught_error_never_downgrades_a_real_finding.
| # A path entry occupies the whole line, so interior whitespace, | ||
| # markup and unprintable bytes each rule it out. | ||
| if any(ch.isspace() or ch in "<>" or not ch.isprintable() for ch in line): | ||
| return False |
There was a problem hiding this comment.
Python .pth path lines may contain spaces; site.addpackage strips only trailing whitespace before joining the entire line to the site directory. This check therefore rejects valid entries such as /opt/My Models/site-packages or common Windows Program Files paths, causing the scanner to mark an ordinary path-config file unreadable and exit 1. Only control/newline characters should be rejected rather than all whitespace within the path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a9c85c9 — you are right about the spec, and I adopted the finding but not the proposed remedy.
site.addpackage only rstrips a line before joining it, so /opt/My Models/site-packages and the Windows C:\Program Files\... shape are valid entries and were being called corrupt. Both are now regression cases. Notably the 141 real .pth files I probed against contained no spaced path, so the real-tree probe could not have caught this — thanks.
Rejecting only control and newline characters, though, would have undone the fix this validator exists for: totally not a model at all has no control characters, so it would read as a valid path entry and a text file saved over a checkpoint would score LOW again. That is the exact case in the issue.
So a line carrying whitespace must additionally hold a path separator (/ or \). Prose does not have one; a directory path does. < and > stay rejected, which is what catches a saved HTML error page. Both halves are pinned: the spaced paths in test_real_pth_path_config_still_scans_clean, the prose in test_text_that_is_not_a_path_config_is_unreadable.
…rse errors (#131) Addresses all three findings from the Codex review on PR #115. P1 — remote artifacts skipped the fix entirely. Every unreadable recording was guarded on `local_path is not None`, so `hf://` and HTTPS scans — the flagship "verify before you `git clone`" path — kept the old behaviour: a remote `.safetensors` of random bytes came back `framework="SafeTensors"`, `risk_level="LOW"` and exit 0. Each inspector now finalizes through one shared `_finalize_inspection`, identified by `local_path or name`, so local and remote are reported identically. The git-LFS subtype survives on the remote path too, which is a real case rather than a contrived one: `raw.githubusercontent.com` serves the pointer text, not the file, for anything LFS-tracked. The head is re-read only after a parse has already failed, so the extra Range request is never paid by a healthy file. P2 — a caught inspector exception stayed a clean verdict. All five inspectors wrap their body in `except Exception: meta["error"] = ...`, and nothing read that key; GGUF, Keras and ONNX seed `risk_level` to LOW, so a file that threw mid-parse exited 0 as clean. `_finalize_inspection` now picks that up — by key membership, not truthiness, because the LFS stub sets it to the empty string. A verdict already reached is never overwritten: an inspector can find a CRITICAL payload and then throw, and downgrading that to "unreadable" would lose the finding — the one outcome worse than the bug being fixed. Only the seeded UNKNOWN/LOW placeholders give way. Pinned by test. P2 — `.pth` path entries may contain spaces. `site.addpackage` only rstrips a line before joining it, so `/opt/My Models/site-packages` and the Windows `C:\Program Files\...` shape are valid and were being called corrupt. Rejecting only control characters (as suggested) would have let `totally not a model at all` read as a path and undone the fix, so a line carrying whitespace must additionally hold a path separator: prose has none, a directory path does. The 141-file probe corpus contained no spaced path, which is why the real-tree probe could not catch this. Verification: 1498 tests pass (+8), coverage 93.79%. Real-tree probe re-run after the `.pth` change — still zero false positives, 141 `.pth` files kept clean. Scorecard, full-repo scan and `--strict` gates all exit 0.
1 participant
Closes Lab700xOrg/aisbom-ops#131.
The problem
LOWis an assertion: AIsbom opened the artifact and found nothing dangerous. A file that no parser could read got the same verdict — with a confident framework label — and exited0. So a truncated download passed a CI gate as a clean model, andaisbom scoregraded a garbage scan as a complete AIBOM.Reproduced on
main(deb3abc) with six corruption shapes, all graded clean at exit 0:lfs_pointer.safetensors(git-LFS stub)LOW/ SafeTensors,error=''UNKNOWN (Git LFS Pointer)+ exit 1empty.gguf(0 bytes)UNKNOWN (Invalid Header), exit 0UNKNOWN (Empty File)+ exit 1truncated_pickle.pkl(\x80\x05)LOW/ PickleUNKNOWN (Truncated)+ exit 1random.safetensors(200 random bytes)LOW/ SafeTensorsUNKNOWN (Truncated)+ exit 1garbage.pt(plain text)LOW/ "Python Path Config"UNKNOWN (Unrecognized Format)+ exit 1truncated_zip.ptLOW/ "Python Path Config"UNKNOWN (Unrecognized Format)+ exit 1The root cause is small: five inspectors already recorded their own parse failure in
meta["error"], and nothing in the codebase ever read it. Two shapes never raise at all, and the LFS stub set that key to the empty string — so even a truthiness check on it would have missed one.What changed
Each inspector now reports whether its parse actually succeeded. Unreadable files land in
results["errors"]with a closed-set subtype (LfsPointer,EmptyFile,TruncatedStream,UnrecognizedFormat), print in their own Could not read section, and exit1— which is what the README's exit-code table has always documented for "a file failed to parse".The component stays in the SBOM so an auditor can see the file was present, but carries no framework label and no risk verdict, marked with
aisbom:unreadable/aisbom:unreadable_typeproperties. Dropping the label is what stopsaisbom:formatclaiming 200 random bytes are SafeTensors.aisbom scorerefuses to grade such a scan through the existing #114 gate — no change needed there.Three adjacent instances of the same problem, fixed on the same terms:
UNKNOWNverdicts had no consequence. An unparseable GGUF, Keras or ONNX file was labelled honestly, butUNKNOWNscores0in_risk_score— belowLOW— so those scans exited 0 and the file rated safer than a clean model..pt/.pth/.binwas "Python Path Config". An HTML error page saved over a checkpoint scoredLOW..pthgenuinely is also a Python path-config format, so that classification is now.pth-only and validates against the format's actual spec (importstatements and bare paths) rather than accepting anything that decodes as text.OverflowErrorwith a message naming the problem.The real-tree probe changed the design
Per the
probe-real-trees-for-false-positiveslesson, curated fixtures prove nothing about false positives. The probe caught one: my first implementation required the pickle opcode walk to reach aSTOPopcode, and that flagged 75 valid files — every.pklin joblib's own test corpus, across every virtualenv on the machine. joblib appends array data as raw bytes immediately after the pickle's STOP, so the walk dies on that trailing data after 54 perfectly good opcodes.The rule is now "nothing past the protocol header disassembled", which catches the issue's
\x80\x05case and passes joblib's. That file shape is a named regression test.Final probe — zero false positives across 35,297 artifacts:
.pthfiles)node_modules×2Verification
bypass-scorecard --check(9/11, no case below floor),scan .over the whole repo,scan demo_data,--strict,mock_broken.pt --lint,diff. The full-repo scan is the load-bearing one — CI runs it with--no-fail-on-risk, which suppresses exit 2 but not exit 1 from errorsgenerate-test-artifactsstill reports CRITICAL for the malicious fixtures and no unreadable filesSix existing tests changed. Two of them —
test_deep_scanner_flags_legacy_pt_when_not_zipandtest_unreadable_keras_file_records_an_error_not_a_crash— had names promising exactly what the code did not do; they were pinning the bug. The other four pinned the threeUNKNOWNlabels.Breaking change
A tree containing unreadable files now exits
1where it exited0. Anyone whose pipeline currently passes over a corrupt or LFS-stub artifact will start failing. That is the intended signal — those files were never examined — and it is the behaviour the exit-code table already documented. Called out in a blockquote at the top of the changelog entry and in a README upgrade note.Considered landing it behind a flag for one release; decided against, since a security fix nobody enables protects nobody, and the real-tree probe buys the same information a flag's soak period would.
Notes
.pklcontaining plain text still scores LOW, becausetotally not a model at alldoes disassemble into 6 opcodes. Pre-existing, outside the issue's repro set, and a stricter rule risks the protocol-0 pickles the codebase explicitly cares about. Routing to the follow-ups tracker rather than widening scope here.