From 9440625d53a0978345d06825b2d28271ccb2743e Mon Sep 17 00:00:00 2001 From: Ajoy L Date: Thu, 17 Sep 2026 22:25:25 -0500 Subject: [PATCH 1/2] fix(scanner): report unreadable model files as errors, not clean LOW-risk artifacts (#131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- CHANGELOG.md | 22 +++ README.md | 16 ++ aisbom/cli.py | 33 +++- aisbom/properties.py | 12 ++ aisbom/safety.py | 30 +++ aisbom/scanner.py | 291 ++++++++++++++++++++++++++-- tests/test_coverage_fix.py | 5 +- tests/test_gguf_template.py | 8 +- tests/test_keras.py | 18 +- tests/test_onnx.py | 12 +- tests/test_scanner_cli.py | 11 +- tests/test_unreadable_artifacts.py | 293 +++++++++++++++++++++++++++++ 12 files changed, 726 insertions(+), 25 deletions(-) create mode 100644 tests/test_unreadable_artifacts.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 95dd482..e261301 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,27 @@ # Changelog +## 1.7.0 — unreleased + +> **Unreadable model files now exit `1` instead of `0`.** A file that no parser could read — a git-LFS pointer stub, an empty file, a truncated stream, a file in no recognised format — was reported as `Risk: LOW` with a confident framework label and passed as a clean scan. It is now reported as an error and exits `1`. **If your tree contains such files, a pipeline that passes today will start failing.** That is the intended signal: those files were never examined, and `--fail-on-risk` was reading `LOW` on an artifact nothing had opened. The exit-code table in the README has always documented exit `1` for "a file failed to parse". + +### Fixed + +- **"Could not read it" is no longer reported as "read it, it's clean".** `LOW` asserts that AIsbom opened an artifact and found nothing dangerous; for a file no parser could read, the honest answer is different. Five inspectors already recorded their own parse failure internally and nothing ever consulted it, so a truncated zip, a two-byte pickle, 200 bytes of random data named `.safetensors`, and a plain-text `.pt` all graded `LOW` at exit `0`. + + Such files now land in the scan's error list, print in their own **Could not read** section, and exit `1`. They still appear in the SBOM — an auditor needs to see the file was present — but carry no framework label and no risk verdict, marked with `aisbom:unreadable` and `aisbom:unreadable_type` properties. A format label is applied only when that format's parser actually succeeded, so random bytes are no longer described as SafeTensors. `aisbom score` consequently refuses to grade a scan containing one. + + A **git-LFS pointer** gets its own message naming the remedy (`git lfs pull`), since that is a configuration problem rather than a corrupt artifact and is the most common unreadable `.safetensors` in practice. + +- **`UNKNOWN` verdicts have a consequence.** An unparseable GGUF, Keras or ONNX file was already labelled honestly, but `UNKNOWN` scores below `LOW` in the risk ranking, so those scans exited `0` and the file rated *safer* than a clean model. They now exit `1` like every other unreadable file. + +- **A text `.pt` or `.bin` is no longer classified as a Python path config.** `.pth` is legitimately also a Python path-configuration format, and any text file in those three extensions inherited that classification — so an HTML error page saved over a checkpoint scored `LOW`. The classification is now `.pth`-only and validates against that format's actual spec (`import` statements and bare paths), verified against every `.pth` file in a real virtualenv. + +- **A SafeTensors header longer than the file is rejected before it is read.** Random bytes decode to an astronomical declared header length; that is now bounded by the bytes actually present, which replaces an internal `OverflowError` with a message saying what is wrong with the file. + +### Changed + +- **Telemetry reports unreadable files.** `cli_scan` gains `unreadable_count` and `unreadable_types` (a closed set: `LfsPointer`, `EmptyFile`, `TruncatedStream`, `UnrecognizedFormat`), so a wave of failed downloads is distinguishable from a wave of clones missing `git lfs pull`. Paths are never sent. `AISBOM_NO_TELEMETRY=1` still turns telemetry off. + ## 1.6.0 — 2026-09-14 > **`--vex` now contacts a third-party service.** When a `--vex` scan finds exact `requirements.txt` pins, it sends each pinned package name and version to the public OSV API at `api.osv.dev`. Nothing else is sent. Pass `--no-osv` or set `AISBOM_NO_OSV=1` to turn this off. The GitHub Action runs `--vex` whenever its `token` input is set, so those runs make the lookup too. diff --git a/README.md b/README.md index 4154400..8579ab1 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,22 @@ xattr -d com.apple.quarantine aisbom-macos-* `--no-fail-on-risk` governs risk findings only. An unusable target still exits `1`, so a typo'd path in CI fails loudly instead of passing as a clean scan. +### Files that could not be read + +`LOW` is an assertion: AIsbom opened the artifact and found nothing dangerous. A file that no parser could read gets a different answer, reported in its own **Could not read** section and exiting `1`: + +``` +⚠️ Could not read: + - models/model.safetensors: this is a git-LFS pointer file, not the model + itself — run `git lfs pull` to fetch the real artifact, then re-scan + - models/head.pkl: declared a pickle by its extension, but nothing past the + protocol header could be disassembled — truncated or not a pickle at all +``` + +The cases are a git-LFS pointer stub (the usual one — a clone without `git lfs pull`), an empty file, a truncated stream, and a file in no recognised format. Such an artifact still appears in the SBOM, so an auditor can see it was present, but it carries **no framework label and no risk verdict** — a format label is only applied when that format's parser actually succeeded. In CycloneDX it is marked with `aisbom:unreadable` and `aisbom:unreadable_type` properties, and `aisbom score` refuses to grade a scan containing one. + +> **Upgrading:** before v1.7.0 these files were reported as `Risk: LOW` with a confident framework label and exited `0`, so a truncated download could pass a CI gate as a clean model. If your tree contains unreadable files, those scans now exit `1`. That is the behaviour the exit-code table above always documented; the files were never examined. + ### Scan a Hugging Face model ```bash diff --git a/aisbom/cli.py b/aisbom/cli.py index c609614..a491a88 100644 --- a/aisbom/cli.py +++ b/aisbom/cli.py @@ -833,6 +833,10 @@ def _risk_score(label: str) -> int: # (context) below. Intentional; see the slice notes. fetch_failures = [e for e in results['errors'] if e.get('fetch_failure')] target_errors = [e for e in results['errors'] if e.get('target_error')] + # Files a model extension claimed that no parser could read (#131). Split + # out here, ahead of telemetry, so the count and subtypes ride into + # `cli_scan` alongside the target-error split. + unreadable = [e for e in results['errors'] if e.get('unreadable')] # Loop detection (#99) works at scan granularity ("N runs in a row"): the # first failure's fingerprint represents the invocation — a fetch failure # if there is one, else a target error (#126: a cron job on a typo'd path @@ -896,6 +900,14 @@ def _risk_score(label: str) -> int: # split that tells "scanned nothing" from "scanned a corrupt model". "parse_error_count": str(len(results.get("errors", []))), "target_error_count": str(len(target_errors)), + # #131, following the #126 split. The count answers "how much of this + # tree could we not read", and the closed-set subtypes say why — which + # is what tells a wave of failed downloads from a wave of clones + # missing `git lfs pull`. Both are bounded cardinality by construction. + "unreadable_count": str(len(unreadable)), + "unreadable_types": ",".join(sorted( + {e["unreadable_type"] for e in unreadable if e.get("unreadable_type")} + )), "strict_mode": "true" if strict else "false", } telemetry_threads.append( @@ -997,11 +1009,28 @@ def _risk_score(label: str) -> int: if target_errors and not fetch_failures: _maybe_print_loop_warning(loop_count, first_payload["http_status"]) - # Parse errors only — fetch failures and target errors already printed - # their own message to stderr and don't fit the "Could not parse" framing. + # Files carrying a model extension that no parser could read (#131). Their + # own section, because "could not parse" describes a parser that ran and + # failed, while these mostly never got that far — an empty file, a git-LFS + # stub, a stream that stops mid-way. The distinction is the actionable + # part: the LFS case is a `git lfs pull` away from scanning fine. + if unreadable: + console.print("\n[bold red]⚠️ Could not read:[/bold red]") + for err in unreadable: + console.print(f" - [yellow]{err['file']}[/yellow]: {err['error']}") + console.print( + "\n[dim]These files were not examined, so they carry no risk " + "verdict. A scan that cannot read an artifact is not a scan that " + "found it clean.[/dim]" + ) + + # Parse errors only — fetch failures, target errors and unreadable files + # already printed their own message and don't fit the "Could not parse" + # framing. parse_errors = [ e for e in results['errors'] if not e.get('fetch_failure') and not e.get('target_error') + and not e.get('unreadable') ] if parse_errors: console.print("\n[bold red]⚠️ Errors Encountered:[/bold red]") diff --git a/aisbom/properties.py b/aisbom/properties.py index 5e3dd97..9713419 100644 --- a/aisbom/properties.py +++ b/aisbom/properties.py @@ -66,6 +66,18 @@ def build_component_properties(art: Dict[str, Any]) -> List[Tuple[str, str]]: if legal: props.append(("aisbom:legal", str(legal))) + # A file no parser could read (#131). Emitted before the format branch + # because such an artifact deliberately carries no `framework` — the label + # is an assertion that that format's parser succeeded — so it returns below + # without any `aisbom:format`. The marker is what keeps the component + # honest rather than merely silent: a consumer sees the file was present + # and was not examined, instead of reading an absent risk as a clean one. + if art.get("unreadable"): + props.append(("aisbom:unreadable", "true")) + unreadable_type = art.get("unreadable_type") + if unreadable_type: + props.append(("aisbom:unreadable_type", str(unreadable_type))) + fmt = _format_for(art) if fmt is None: return props diff --git a/aisbom/safety.py b/aisbom/safety.py index 06664df..ca11ff5 100644 --- a/aisbom/safety.py +++ b/aisbom/safety.py @@ -1151,6 +1151,36 @@ def looks_like_pickle_stream(data: bytes) -> bool: return False +def pickle_content_opcode_count(data: bytes) -> int: + """How many opcodes past the protocol header ``data`` disassembles into. + + Answers a narrower question than `looks_like_pickle_stream`: not "is this a + complete pickle" but "was there anything here to look at at all". Zero means + the disassembler got nothing beyond a bare `PROTO` byte — the file is + truncated or is not a pickle — which is what separates "could not read it" + from "read it, found nothing dangerous" (#131). + + Reaching STOP is deliberately *not* required, because a real and complete + pickle need not be the whole file. joblib writes its arrays as raw bytes + directly after the pickle's STOP, so the opcode walk over a valid + `.pkl` from joblib's own test corpus dies on that trailing data after 54 + content opcodes. Requiring STOP called 75 such files unreadable. + + Disassembly only. The stream is never unpickled. + """ + count = 0 + try: + for opcode, _arg, _pos in pickletools.genops(io.BytesIO(data)): + if opcode.name == "PROTO": + # The header says which protocol follows; it is not content. + continue + count += 1 + except Exception: + # A walk that dies partway still examined everything it counted. + pass + return count + + class _NullWriter: """Sink for `pickletools.dis`, which validates by writing a listing.""" diff --git a/aisbom/scanner.py b/aisbom/scanner.py index 8a579a6..eb6b5dc 100644 --- a/aisbom/scanner.py +++ b/aisbom/scanner.py @@ -16,6 +16,7 @@ head_looks_like_pickle, jinja_threats_are_critical, looks_like_pickle_stream, + pickle_content_opcode_count, onnx_domain_is_custom, scan_jinja_template, scan_keras_config, @@ -40,6 +41,12 @@ # scanned nothing and exited 0. PICKLE_VARIANT_EXTENSIONS = {'.pkl', '.pickle', '.joblib', '.dill', '.npy', '.npz'} +# The subset whose format *is* a pickle stream. One of these that does not +# disassemble through to a STOP opcode is truncated or mislabelled, not a clean +# artifact (#131) — where a `.npy`/`.npz` of ordinary numbers legitimately +# carries no pickle at all and stays LOW. +PICKLE_STREAM_EXTENSIONS = {'.pkl', '.pickle', '.joblib', '.dill'} + # Local reads get the same budget as the other formats; a remote read pays one # HTTP Range request per call and gets the tighter one. PICKLE_VARIANT_MAX_SCAN_BYTES = 16 * 1024 * 1024 @@ -76,6 +83,44 @@ "NotAFileOrDirectory", }) +# Why a file carrying a model extension could not be read (#131). Same closed +# -set discipline as TARGET_ERROR_TYPES and for the same reason: the subtype +# rides into `cli_error` telemetry, so it is chosen at the call site and never +# derived from the path or the exception text. +# +# The distinction these draw is the point of the slice. `LOW` asserts that a +# file was inspected and nothing dangerous was found; for a file no parser ever +# read, the honest answer is "could not read this". Collapsing the two let a +# truncated download pass a CI gate as a clean model. +UNREADABLE_TYPES = frozenset({ + # A git-LFS pointer stub, not the model: the common real-world unparseable + # `.safetensors`, left behind by a clone without `git lfs pull`. Called out + # separately because it is a configuration problem with a specific remedy, + # and "could not parse" would send the reader hunting for corruption. + "LfsPointer", + "EmptyFile", + # Opened and recognized, but the data ran out: a pickle that never reaches + # STOP, a SafeTensors header shorter than its declared length. + "TruncatedStream", + # Nothing claimed it — no container magic, no parseable header, and not a + # valid `.pth` path config either. + "UnrecognizedFormat", +}) + +# Risk labels for unreadable files. `UNKNOWN (...)` follows the vocabulary the +# GGUF/Keras/ONNX inspectors already used for "couldn't read it"; the exit code +# comes from the errors list, not from this label. +_UNREADABLE_RISK_LABELS = { + "LfsPointer": "UNKNOWN (Git LFS Pointer)", + "EmptyFile": "UNKNOWN (Empty File)", + "TruncatedStream": "UNKNOWN (Truncated)", + "UnrecognizedFormat": "UNKNOWN (Unrecognized Format)", +} + +# First line of a git-LFS pointer file. The spec fixes this as the first line of +# every pointer, so matching the prefix is exact rather than heuristic. +LFS_POINTER_MAGIC = b"version https://git-lfs.github.com/spec/v1" + # --- ONNX protobuf field numbers --- # ONNX has no magic bytes — a .onnx file is a bare serialized ModelProto — so # these field numbers are the schema. Confirmed against models serialized by the @@ -372,17 +417,17 @@ def _inspect_local_file(self, full_path: Path) -> bool: ext = full_path.suffix.lower() if ext in PYTORCH_EXTENSIONS: - self.artifacts.append(self._inspect_pytorch(full_path)) + self.artifacts.append(self._inspect_claimed(full_path, self._inspect_pytorch)) elif ext == SAFETENSORS_EXTENSION: - self.artifacts.append(self._inspect_safetensors(full_path)) + self.artifacts.append(self._inspect_claimed(full_path, self._inspect_safetensors)) elif ext == GGUF_EXTENSION: - self.artifacts.append(self._inspect_gguf(full_path)) + self.artifacts.append(self._inspect_claimed(full_path, self._inspect_gguf)) elif ext in KERAS_EXTENSIONS: - self.artifacts.append(self._inspect_keras(full_path)) + self.artifacts.append(self._inspect_claimed(full_path, self._inspect_keras)) elif ext == ONNX_EXTENSION: - self.artifacts.append(self._inspect_onnx(full_path)) + self.artifacts.append(self._inspect_claimed(full_path, self._inspect_onnx)) elif ext in PICKLE_VARIANT_EXTENSIONS: - self.artifacts.append(self._inspect_pickle_variant(full_path)) + self.artifacts.append(self._inspect_claimed(full_path, self._inspect_pickle_variant)) elif full_path.name == REQUIREMENTS_FILENAME: self._parse_requirements(full_path) elif self._sniff_is_pickle(full_path): @@ -467,6 +512,92 @@ def _record_target_error(self, target: str, message: str, error_type: str) -> No "target_error_type": error_type, }) + def _mark_unreadable( + self, meta: Dict[str, Any], path: Path | str, unreadable_type: str, + message: str, + ) -> Dict[str, Any]: + """Record a file no parser could read, and strip its format claims. + + Two effects, deliberately paired. The error lands in results['errors'] + so the CLI's `errors → exit 1` path fires and `score` refuses to grade + the scan (#114's gate keys off the same list). And `meta` loses its + format label and risk verdict: both were assertions about a file that + was never successfully parsed, and dropping `framework` is what stops + `aisbom:format` claiming 200 random bytes are SafeTensors. + + The component itself stays in the SBOM, carrying the marker instead of + a verdict — an auditor still needs to see that the file was there and + was not examined. `scan_incomplete` (#113) set the precedent. + """ + if unreadable_type not in UNREADABLE_TYPES: + raise ValueError(f"unknown unreadable type: {unreadable_type!r}") + self.errors.append({ + "file": str(path), + "error": message, + "unreadable": True, + "unreadable_type": unreadable_type, + }) + meta["framework"] = None + meta["risk_level"] = _UNREADABLE_RISK_LABELS[unreadable_type] + meta["unreadable"] = True + meta["unreadable_type"] = unreadable_type + return meta + + def _inspect_claimed(self, path: Path, inspector) -> Dict[str, Any]: + """Run ``inspector`` on a file a model extension claimed, unless the + file is disqualified before any parser is worth running. + + Wrapping the dispatch rather than each inspector keeps the precheck in + one place: every format gets the empty-file and LFS-stub verdicts on + the same terms, and a format added later inherits them. + """ + precheck = self._unreadable_precheck(path) + if precheck is not None: + return self._unreadable_stub(path, *precheck) + return inspector(path) + + def _unreadable_stub( + self, path: Path, unreadable_type: str, message: str, + ) -> Dict[str, Any]: + """A component for a file rejected before any parser ran.""" + return self._mark_unreadable({ + "name": path.name, + "type": "machine-learning-model", + "license": "Unknown", + "legal_status": "UNKNOWN", + "hash": self._calculate_hash(path), + "details": {}, + }, path, unreadable_type, message) + + @staticmethod + def _unreadable_precheck(path: Path | None) -> tuple[str, str] | None: + """Classify a file that cannot hold a model regardless of extension. + + Runs before any format parser because neither case is that format + failing — an empty file and an LFS stub are the same whether they are + named `.safetensors` or `.gguf`, and the LFS case wants its own remedy + in the message rather than whatever error the header parse happens to + raise. (The stub used to reach the SafeTensors parser and set + `meta["error"]` to the empty string, which no truthiness check would + have caught.) + """ + if path is None: + return None + try: + if path.stat().st_size == 0: + return ("EmptyFile", "file is empty — 0 bytes, nothing to inspect") + with open(path, "rb") as handle: + head = handle.read(len(LFS_POINTER_MAGIC)) + except OSError: + return None + if head.startswith(LFS_POINTER_MAGIC): + return ( + "LfsPointer", + "this is a git-LFS pointer file, not the model itself — " + "run `git lfs pull` to fetch the real artifact, then re-scan", + ) + return None + def _record_fetch_error(self, target: str, exc: Exception) -> None: """Record a remote fetch failure as a structured, non-fatal error. @@ -512,6 +643,41 @@ def _identify_container(head: bytes) -> str | None: return label return None + @staticmethod + def _looks_like_pth_config(content: bytes) -> bool: + """True if ``content`` is a Python path-configuration file (#131). + + `.pth` is the one model extension that is also a real text format: + Python's `site` module reads every `.pth` in site-packages and treats + each line as a directory to append to `sys.path`, or executes it when + it starts with `import`. Those files are in every virtualenv, so they + have to keep scanning clean. + + Being *text* was never evidence of being one of them — that is what let + an HTML error page saved over a checkpoint score LOW. So this checks the + actual shape: every meaningful line is an `import` statement or a single + bare path, which prose, markup and LFS stubs all fail on whitespace. + + Validated against all 141 `.pth` files in this machine's virtualenvs. + """ + try: + text = content.decode("utf-8") + except UnicodeDecodeError: + return False + if not text.strip(): + return False + for raw_line in text.splitlines(): + line = raw_line.strip() + if not line or line.startswith("#"): + continue + if line.startswith(("import ", "import\t")): + continue + # 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 + return True + @staticmethod def _looks_like_text(content: bytes) -> bool: """True if the first kilobyte decodes as UTF-8 and is mostly printable.""" @@ -624,6 +790,7 @@ def _inspect_pytorch(self, source, name: str | None = None, is_remote: bool = Fa "details": {} } stream = None + unreadable: tuple[str, str] | None = None try: # Choose stream if local_path: @@ -705,10 +872,24 @@ def _inspect_pytorch(self, source, name: str | None = None, is_remote: bool = Fa meta["risk_level"] = "MEDIUM (Pickle Scan Incomplete)" elif looks_like_pickle_stream(content): meta["risk_level"] = "MEDIUM (Pickle Present)" - elif self._looks_like_text(content): + elif ( + Path(name).suffix.lower() == ".pth" + and self._looks_like_pth_config(content) + ): meta["risk_level"] = "LOW" meta["type"] = "configuration" meta["framework"] = "Python Path Config" + elif self._looks_like_text(content): + # Text, but not a path config — and nothing produces a + # text `.pt`/`.bin` at all. A corrupt download, an HTML + # error page saved to disk, or a checkpoint that never + # finished writing; all were graded LOW before #131. + unreadable = ( + "UnrecognizedFormat", + "no model format could read this file — not a zip " + "archive, not a pickle stream, and not a valid " + "`.pth` path configuration", + ) else: # Binary, not a parsable pickle, not a known container. meta["risk_level"] = "CRITICAL (Legacy Binary)" @@ -720,6 +901,8 @@ def _inspect_pytorch(self, source, name: str | None = None, is_remote: bool = Fa stream.close() except Exception: pass + if unreadable and local_path is not None: + self._mark_unreadable(meta, local_path, *unreadable) return meta def _npy_member_threats(self, blob: bytes, details: Dict[str, Any]): @@ -786,6 +969,7 @@ def _inspect_pickle_variant(self, source, name: str | None = None, details = meta["details"] stream = None + variant_unreadable: tuple[str, str] | None = None try: stream = open(local_path, "rb") if local_path else source budget = (PICKLE_VARIANT_MAX_SCAN_BYTES if local_path @@ -912,6 +1096,28 @@ def _inspect_pickle_variant(self, source, name: str | None = None, meta["risk_level"] = f"MEDIUM (Unscanned Container: {unreadable})" elif carries_pickle: meta["risk_level"] = "MEDIUM (Pickle Present)" + elif ( + Path(name).suffix.lower() in PICKLE_STREAM_EXTENSIONS + and details.get("container") == "bare" + and pickle_content_opcode_count(blob) == 0 + ): + # The extension says the whole file is a pickle, and the + # disassembler found nothing past the protocol header. + # `\x80\x05` — a bare header with no stream behind it — used to + # land in the LOW branch below, which exists for numeric `.npy` + # arrays that carry no pickle by design. A `.pkl` carrying no + # pickle at all is a different thing. + # + # "No content opcodes" rather than "never reached STOP": a real + # pickle need not be the whole file. joblib appends its arrays + # as raw bytes after STOP, and a STOP-based rule called 75 + # valid files from its own test corpus unreadable. + variant_unreadable = ( + "TruncatedStream", + "declared a pickle by its extension, but nothing past the " + "protocol header could be disassembled — truncated or not " + "a pickle at all", + ) else: # A `.npy` of ordinary numbers reaches here: a real file, fully # read, carrying no pickle at all. @@ -924,6 +1130,8 @@ def _inspect_pickle_variant(self, source, name: str | None = None, stream.close() except Exception: pass + if variant_unreadable and local_path is not None: + self._mark_unreadable(meta, local_path, *variant_unreadable) return meta def _inspect_safetensors(self, source, name: str | None = None, is_remote: bool = False) -> Dict[str, Any]: @@ -945,14 +1153,40 @@ def _inspect_safetensors(self, source, name: str | None = None, is_remote: bool "details": {} } f = None + # Only a completed header parse earns the SafeTensors label and the LOW + # verdict seeded above (#131). 200 bytes of noise used to keep both. + parsed = False try: f = open(local_path, "rb") if local_path else source f.seek(0) length_bytes = f.read(8) if len(length_bytes) == 8: header_len = struct.unpack(' max(available, 0): + raise ValueError( + f"header declares {header_len} bytes but only " + f"{max(available, 0)} follow the length prefix" + ) + raw_header = f.read(header_len) + if len(raw_header) < header_len: + raise ValueError( + f"header declares {header_len} bytes, file holds " + f"{len(raw_header)}" + ) + header_json = json.loads(raw_header) + if not isinstance(header_json, dict): + raise ValueError("header is not a JSON object") + parsed = True + # EXTRACT METADATA metadata = header_json.get("__metadata__", {}) @@ -986,6 +1220,12 @@ def _inspect_safetensors(self, source, name: str | None = None, is_remote: bool f.close() except Exception: pass + if not parsed and local_path is not None: + self._mark_unreadable( + meta, local_path, "TruncatedStream", + f"SafeTensors header could not be read: " + f"{meta.get('error') or 'header shorter than 8 bytes'}", + ) return meta @staticmethod @@ -1180,7 +1420,17 @@ def _inspect_gguf(self, source, name: str | None = None, is_remote: bool = False # 1. Check Magic "GGUF" magic = f.read(4) if magic != b'GGUF': - meta['risk_level'] = "UNKNOWN (Invalid Header)" + # Honest before #131, but consequence-free: an `UNKNOWN` label + # scores 0 in the CLI's `_risk_score`, below LOW, so the scan + # exited 0 and the file rated safer than a clean model. + if local_path is not None: + self._mark_unreadable( + meta, local_path, "UnrecognizedFormat", + "not a GGUF file — the header does not start with the " + "GGUF magic bytes", + ) + else: + meta['risk_level'] = "UNKNOWN (Invalid Header)" return meta # 2. Read the metadata block in as few reads as possible. @@ -1545,6 +1795,13 @@ def _inspect_keras(self, source, name: str | None = None, is_remote: bool = Fals meta["risk_level"] = self._keras_risk_label( salvage, meta["details"]["lambda_layers"] ) + elif local_path is not None: + # Same consequence-free-UNKNOWN fix as GGUF above (#131). + self._mark_unreadable( + meta, local_path, "UnrecognizedFormat", + "not a recognized Keras container — no HDF5 or zip " + "signature found", + ) else: meta["risk_level"] = "UNKNOWN (Unrecognized Container)" return meta @@ -1869,6 +2126,7 @@ def _inspect_onnx(self, source, name: str | None = None, is_remote: bool = False } f = None + onnx_unreadable: tuple[str, str] | None = None try: f = open(local_path, "rb") if local_path else source budget = ONNX_MAX_SCAN_BYTES if local_path else ONNX_MAX_REMOTE_SCAN_BYTES @@ -1918,7 +2176,15 @@ def _inspect_onnx(self, source, name: str | None = None, is_remote: bool = False } if not looks_like_onnx: - meta["risk_level"] = "UNKNOWN (Unparsable ONNX)" + # Same consequence-free-UNKNOWN fix as GGUF/Keras (#131). + if local_path is not None: + onnx_unreadable = ( + "UnrecognizedFormat", + "not a parseable ONNX model — the bytes do not " + "deserialize as a ModelProto", + ) + else: + meta["risk_level"] = "UNKNOWN (Unparsable ONNX)" else: meta["risk_level"] = self._onnx_risk_label(threats) except Exception as e: @@ -1929,7 +2195,10 @@ def _inspect_onnx(self, source, name: str | None = None, is_remote: bool = False f.close() except Exception: pass + if onnx_unreadable and local_path is not None: + self._mark_unreadable(meta, local_path, *onnx_unreadable) return meta + def _parse_requirements(self, path: Path): try: req_file = RequirementsFile.from_file(path) diff --git a/tests/test_coverage_fix.py b/tests/test_coverage_fix.py index 37a636d..06862e4 100644 --- a/tests/test_coverage_fix.py +++ b/tests/test_coverage_fix.py @@ -143,7 +143,10 @@ def test_scanner_invalid_gguf(tmp_path): scanner = DeepScanner(str(tmp_path)) meta = scanner._inspect_gguf(f) - assert "Invalid Header" in meta["risk_level"] + # #131 routes a bad magic header through the unreadable path, which sets + # the label from a closed set rather than naming the header specifically. + assert meta["risk_level"] == "UNKNOWN (Unrecognized Format)" + assert meta["unreadable"] is True def test_scanner_malformed_requirements(tmp_path): """Test handling of malformed requirements.txt.""" diff --git a/tests/test_gguf_template.py b/tests/test_gguf_template.py index 1219049..cca77b4 100644 --- a/tests/test_gguf_template.py +++ b/tests/test_gguf_template.py @@ -326,8 +326,12 @@ def test_unknown_value_type_stops_the_walk_cleanly(tmp_path): def test_invalid_magic_is_still_rejected(tmp_path): (tmp_path / "bad.gguf").write_bytes(b"NOPE" + b"\x00" * 32) - art = DeepScanner(str(tmp_path)).scan()["artifacts"][0] - assert art["risk_level"] == "UNKNOWN (Invalid Header)" + results = DeepScanner(str(tmp_path)).scan() + art = results["artifacts"][0] + # #131: rejected *and* recorded. The label alone scored 0 in `_risk_score`, + # so "rejected" still meant exit 0. + assert art["risk_level"] == "UNKNOWN (Unrecognized Format)" + assert results["errors"] # --- end to end ----------------------------------------------------------- diff --git a/tests/test_keras.py b/tests/test_keras.py index 5a4f7f7..f03a6f1 100644 --- a/tests/test_keras.py +++ b/tests/test_keras.py @@ -328,10 +328,15 @@ def test_truncated_h5_still_flags_the_payload(tmp_path): def test_unreadable_keras_file_records_an_error_not_a_crash(tmp_path): path = tmp_path / "empty.h5" path.write_bytes(b"") - art = DeepScanner(str(tmp_path)).scan()["artifacts"][0] + results = DeepScanner(str(tmp_path)).scan() + art = results["artifacts"][0] - assert art["framework"] == "Keras" assert "CRITICAL" not in art["risk_level"] + # Since #131 this test's name is finally true: an empty file records an + # actual error rather than a Keras-labelled artifact with no verdict. + assert art["framework"] is None + assert art["unreadable_type"] == "EmptyFile" + assert results["errors"] # --- containers that do not present cleanly ------------------------------- @@ -386,10 +391,15 @@ def test_damaged_keras_archive_still_yields_its_signature(tmp_path): def test_unrecognized_container_with_no_signature_stays_unknown(tmp_path): (tmp_path / "junk.keras").write_bytes(b"not a container at all" * 10) - art = DeepScanner(str(tmp_path)).scan()["artifacts"][0] + results = DeepScanner(str(tmp_path)).scan() + art = results["artifacts"][0] - assert art["risk_level"] == "UNKNOWN (Unrecognized Container)" + # #131 gave this verdict its consequence. `UNKNOWN (...)` was honest but + # scored 0 in the CLI's `_risk_score` — below LOW — so an unreadable file + # exited 0 and rated safer than a clean model. + assert art["risk_level"] == "UNKNOWN (Unrecognized Format)" assert "CRITICAL" not in art["risk_level"] + assert results["errors"] def test_config_larger_than_the_read_budget_is_not_called_clean(tmp_path, monkeypatch): diff --git a/tests/test_onnx.py b/tests/test_onnx.py index ccb4e37..493d77d 100644 --- a/tests/test_onnx.py +++ b/tests/test_onnx.py @@ -206,10 +206,16 @@ def test_garbage_onnx_file_is_not_reported_as_a_model(tmp_path): def test_empty_onnx_file_does_not_crash(tmp_path): (tmp_path / "empty.onnx").write_bytes(b"") - art = DeepScanner(str(tmp_path)).scan()["artifacts"][0] + results = DeepScanner(str(tmp_path)).scan() + art = results["artifacts"][0] - assert art["framework"] == "ONNX" - assert art["details"]["parsed"] is False + # Still the no-crash guarantee this test exists for. Since #131 a zero-byte + # file is classified before any parser runs, so it carries no ONNX label — + # the label asserts that ONNX's parser succeeded. + assert art["framework"] is None + assert art["unreadable"] is True + assert art["unreadable_type"] == "EmptyFile" + assert results["errors"] def test_truncated_model_still_reports_the_nodes_it_covers(tmp_path, monkeypatch): diff --git a/tests/test_scanner_cli.py b/tests/test_scanner_cli.py index 9de41d9..bc3e17e 100644 --- a/tests/test_scanner_cli.py +++ b/tests/test_scanner_cli.py @@ -321,8 +321,15 @@ def test_deep_scanner_flags_legacy_pt_when_not_zip(tmp_path): scanner = DeepScanner(tmp_path) results = scanner.scan() art = {a["name"]: a for a in results["artifacts"]}[legacy.name] - assert art["risk_level"] == "LOW" - assert art["framework"] == "Python Path Config" + # Until #131 this asserted LOW / "Python Path Config", which is what the + # test's own name said it should not be: text in a `.pt` is a corrupt + # download or a saved error page, and nothing produces a text `.pt` path + # config. The path-config classification is now `.pth`-only and validated + # against that format's actual spec. + assert art["risk_level"] == "UNKNOWN (Unrecognized Format)" + assert art["framework"] is None + assert art["unreadable"] is True + assert [e for e in results["errors"] if e["file"].endswith(legacy.name)] def test_cli_scan_outputs_sbom_with_components(tmp_path): diff --git a/tests/test_unreadable_artifacts.py b/tests/test_unreadable_artifacts.py new file mode 100644 index 0000000..85c1652 --- /dev/null +++ b/tests/test_unreadable_artifacts.py @@ -0,0 +1,293 @@ +"""Unparseable model files are errors, not clean LOW-risk artifacts (#131). + +`LOW` is an assertion: AIsbom opened the artifact and found nothing dangerous. +For a file no parser ever read, the honest output is "could not read this", and +collapsing the two is the #125 failure in a different place — something never +examined passing a CI gate. + +Before this change, six distinct corruption shapes all graded clean and exited +0, because the inspectors' own failure signal was never consulted: five of them +write `meta["error"]` and nothing in the codebase read it. Two of the shapes +never raise at all (a text `.pt`, a two-byte pickle), and the git-LFS pointer +set `meta["error"]` to the *empty string* — so a truthiness check on that key +would still have missed it. Each inspector therefore has to report whether its +parse actually succeeded, which is what these tests pin. + +The bytes here are real corruption (a truncated zip, a two-byte pickle, random +bytes, an LFS stub, plain text) rather than curated fixtures, per the +`probe-real-trees-for-false-positives` lesson: the false-positive risk lives in +ordinary files, so the guard has to be proven against ordinary files. +""" + +import json +import struct + +import pytest +from typer.testing import CliRunner + +from aisbom.cli import app +from aisbom.properties import build_component_properties +from aisbom.scanner import UNREADABLE_TYPES, DeepScanner + +runner = CliRunner() + + +# --- the six reproduced shapes ------------------------------------------- + +LFS_POINTER = ( + b"version https://git-lfs.github.com/spec/v1\n" + b"oid sha256:4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f5a6b7c8d9e0f1a2b3c4d5e\n" + b"size 440449768\n" +) + +# Every corruption shape the issue reproduced, plus the two it missed: a zero +# byte file and an LFS pointer stub (the common real-world unparseable +# `.safetensors`, left by a clone without `git lfs pull`). +SHAPES = [ + ("lfs_pointer.safetensors", LFS_POINTER, "LfsPointer"), + ("empty.gguf", b"", "EmptyFile"), + ("truncated_pickle.pkl", b"\x80\x05", "TruncatedStream"), + ("random.safetensors", b"\x91\x3f" * 100, "TruncatedStream"), + ("garbage.pt", b"totally not a model at all", "UnrecognizedFormat"), + ("truncated_zip.pt", b"PK\x03\x04corrupt-not-a-real-zip", "UnrecognizedFormat"), +] + + +def _scan(tmp_path, name: str, payload: bytes): + (tmp_path / name).write_bytes(payload) + return DeepScanner(tmp_path).scan() + + +def _errors_for(results, name: str): + return [e for e in results["errors"] if e["file"].endswith(name)] + + +def _artifact(results, name: str): + for art in results["artifacts"]: + if art["name"] == name: + return art + return None + + +@pytest.mark.parametrize("name,payload,expected_type", SHAPES) +def test_unreadable_file_is_recorded_as_an_error(tmp_path, name, payload, expected_type): + """Each corruption shape lands in results["errors"] with its subtype.""" + results = _scan(tmp_path, name, payload) + + errors = _errors_for(results, name) + assert errors, f"{name} produced no error entry" + assert errors[0]["unreadable"] is True + assert errors[0]["unreadable_type"] == expected_type + assert expected_type in UNREADABLE_TYPES + + +@pytest.mark.parametrize("name,payload,_expected", SHAPES) +def test_unreadable_file_is_never_low_risk(tmp_path, name, payload, _expected): + """The whole point: an unexamined file must not read as a clean verdict.""" + art = _artifact(_scan(tmp_path, name, payload), name) + + assert art is not None, "the file should still appear in the SBOM" + assert art["risk_level"].startswith("UNKNOWN ("), art["risk_level"] + assert "LOW" not in art["risk_level"] + assert art["unreadable"] is True + + +def test_random_bytes_are_not_described_as_safetensors(tmp_path): + """A format label is an assertion that that format's parser succeeded.""" + results = _scan(tmp_path, "random.safetensors", b"\x91\x3f" * 100) + art = _artifact(results, "random.safetensors") + + assert not art.get("framework"), art.get("framework") + names = {n for n, _ in build_component_properties(art)} + assert "aisbom:format" not in names + + +def test_unreadable_artifact_carries_marker_properties(tmp_path): + """The SBOM records that the file was seen and not examined.""" + results = _scan(tmp_path, "lfs_pointer.safetensors", LFS_POINTER) + props = dict(build_component_properties(_artifact(results, "lfs_pointer.safetensors"))) + + assert props["aisbom:unreadable"] == "true" + assert props["aisbom:unreadable_type"] == "LfsPointer" + + +def test_lfs_pointer_error_names_the_remedy(tmp_path): + """A pointer stub is a config problem; "could not parse" would misdirect.""" + results = _scan(tmp_path, "model.safetensors", LFS_POINTER) + + assert "git lfs pull" in _errors_for(results, "model.safetensors")[0]["error"] + + +# --- the .pth collision --------------------------------------------------- +# +# `.pth` is the one model extension that is *also* a real text format: Python's +# `site` module reads every `.pth` in site-packages, appending each line to +# sys.path or executing it when it starts with `import`. Those files exist in +# every virtualenv and must keep scanning clean, so the text branch narrows to +# `.pth` alone and validates against that spec instead of accepting any text. + +@pytest.mark.parametrize( + "content", + [ + "/usr/local/lib/python3.11/site-packages", + "import _virtualenv", + "import os; var = 'SETUPTOOLS_USE_DISTUTILS'; enabled = os.environ.get(var)", + "# a comment\n/opt/some/path\n", + "/first/path\n/second/path\n", + ], +) +def test_real_pth_path_config_still_scans_clean(tmp_path, content): + """Genuine path-config files are not corruption and must not be flagged. + + An *empty* `.pth` is deliberately absent from these cases: zero of the 141 + real `.pth` files across this machine's virtualenvs are empty, so an empty + model-extension file is treated as a failed download uniformly rather than + exempting one extension on a case that does not occur in practice. + """ + (tmp_path / "site-packages.pth").write_text(content) + results = DeepScanner(tmp_path).scan() + + assert results["errors"] == [] + art = _artifact(results, "site-packages.pth") + assert art["risk_level"] == "LOW" + assert art["framework"] == "Python Path Config" + + +@pytest.mark.parametrize( + "content", + [ + "totally not a model at all", + "404 Not Found", + "version https://git-lfs.github.com/spec/v1\noid sha256:abc\nsize 1\n", + ], +) +def test_text_that_is_not_a_path_config_is_unreadable(tmp_path, content): + """Text alone was never evidence of a path config — an HTML error page + saved over a checkpoint used to score LOW.""" + (tmp_path / "model.pth").write_text(content) + results = DeepScanner(tmp_path).scan() + + assert results["errors"], "non-path-config text should not pass as clean" + assert _artifact(results, "model.pth")["risk_level"].startswith("UNKNOWN (") + + +@pytest.mark.parametrize("ext", [".pt", ".bin"]) +def test_path_config_classification_is_pth_only(tmp_path, ext): + """Nothing produces a text `.pt`/`.bin` path config; such a file is a + corrupt download or a saved error page, not a configuration.""" + (tmp_path / f"model{ext}").write_text("/usr/local/lib/python3.11/site-packages") + results = DeepScanner(tmp_path).scan() + + assert results["errors"] + assert _artifact(results, f"model{ext}")["framework"] != "Python Path Config" + + +# --- regression from the real-tree probe ---------------------------------- + +def test_pickle_with_trailing_raw_bytes_is_not_unreadable(tmp_path): + """A real pickle need not be the whole file. + + Promoted from the real-tree probe, which is the only reason this is here: + an earlier draft required the opcode walk to reach STOP, and that called + **75** valid files unreadable — every `.pkl` in joblib's own test corpus, + across every virtualenv on the machine. joblib writes its arrays as raw + bytes straight after the pickle's STOP, so the walk dies on that trailing + data after 54 perfectly good content opcodes. + + The bytes below are that layout: a complete protocol-3 pickle carrying a + `NumpyArrayWrapper` global, followed by raw array data. + """ + body = ( + b"\x80\x03]q\x00(cjoblib.numpy_pickle\nNumpyArrayWrapper\nq\x01)" + b"\x81q\x02}q\x03X\x05\x00\x00\x00shapeq\x04K\x05\x85q\x05sbe." + ) + trailing = bytes(range(256)) * 4 # raw array payload, not opcodes + results = _scan(tmp_path, "model.pkl", body + trailing) + + assert not _errors_for(results, "model.pkl"), results["errors"] + assert _artifact(results, "model.pkl")["risk_level"] != "UNKNOWN (Truncated)" + + +# --- the pre-existing honest labels get their consequence ---------------- +# +# Three inspectors already told the truth about files they could not read — +# `UNKNOWN (Invalid Header)` (GGUF), `UNKNOWN (Unrecognized Container)` +# (Keras), `UNKNOWN (Unparsable ONNX)` — but the verdict had no consequence: +# `_risk_score` maps an `UNKNOWN` label to 0, *below* LOW, so such a file +# exited 0 and rated safer than a clean model. Same class of problem as the +# rest of the slice, so it is fixed on the same terms rather than left as a +# knowingly inconsistent corner. + +@pytest.mark.parametrize( + "name,payload", + [ + ("bad.gguf", b"NOPE" + b"\x00" * 32), + ("junk.keras", b"not a container at all" * 10), + ("junk.onnx", b"\xff\xff\xff\xff" * 40), + ], +) +def test_unparsable_known_format_is_an_error(tmp_path, name, payload): + results = _scan(tmp_path, name, payload) + + assert _errors_for(results, name), f"{name} produced no error entry" + assert _artifact(results, name)["unreadable"] is True + + +def test_unparsable_gguf_exits_1_rather_than_0(tmp_path): + """An UNKNOWN label scores 0 in `_risk_score`, so this used to exit 0.""" + (tmp_path / "bad.gguf").write_bytes(b"NOPE" + b"\x00" * 32) + result = runner.invoke(app, ["scan", str(tmp_path), "--output", str(tmp_path / "s.json")]) + + assert result.exit_code == 1, result.output + + +# --- clean models stay clean --------------------------------------------- + +def test_valid_safetensors_still_scores_low(tmp_path): + """No false "unreadable" for a real artifact (the regression that matters).""" + header = json.dumps({"__metadata__": {"license": "apache-2.0"}}).encode() + payload = struct.pack(" Date: Thu, 17 Sep 2026 22:35:40 -0500 Subject: [PATCH 2/2] fix(scanner): apply unreadable handling to remote scans and caught parse errors (#131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- aisbom/scanner.py | 179 +++++++++++++++++++++-------- tests/test_unreadable_artifacts.py | 122 ++++++++++++++++++++ 2 files changed, 253 insertions(+), 48 deletions(-) diff --git a/aisbom/scanner.py b/aisbom/scanner.py index eb6b5dc..047bd48 100644 --- a/aisbom/scanner.py +++ b/aisbom/scanner.py @@ -121,6 +121,14 @@ # every pointer, so matching the prefix is exact rather than heuristic. LFS_POINTER_MAGIC = b"version https://git-lfs.github.com/spec/v1" +# Named once because both the local precheck and the remote failure path report +# it. The remedy is the useful half: this is a configuration problem, not a +# corrupt artifact, and "could not parse" would send the reader hunting. +LFS_POINTER_MESSAGE = ( + "this is a git-LFS pointer file, not the model itself — run `git lfs pull` " + "to fetch the real artifact, then re-scan" +) + # --- ONNX protobuf field numbers --- # ONNX has no magic bytes — a .onnx file is a bare serialized ModelProto — so # these field numbers are the schema. Confirmed against models serialized by the @@ -543,6 +551,39 @@ def _mark_unreadable( meta["unreadable_type"] = unreadable_type return meta + def _finalize_inspection( + self, meta: Dict[str, Any], ident, deferred: tuple[str, str] | None = None, + ) -> Dict[str, Any]: + """Route a failed inspection into results['errors'] (#131). + + Called at the tail of every inspector, local and remote alike, so a + remote `.safetensors` of random bytes is reported the same way a local + one is — the first cut guarded each call on `local_path`, which meant + `hf://` and HTTPS scans, the flagship path, kept the old clean verdict + (Codex review, PR #115). + + Two routes in. `deferred` is an inspector that decided for itself that + it could not read the file. Otherwise a caught exception is picked up + from ``meta["error"]``, which every inspector sets and, before this, + nothing read — including the git-LFS case that sets it to the *empty + string*, so membership is tested rather than truthiness. + + A verdict already reached is never overwritten. An inspector can find + a CRITICAL payload and *then* throw; downgrading that to "unreadable" + would lose the finding, which is the one outcome worse than the bug + this fixes. Only the seeded placeholders give way. + """ + if deferred is not None: + return self._mark_unreadable(meta, ident, *deferred) + if "error" not in meta: + return meta + if str(meta.get("risk_level", "")).split(" ")[0] not in ("UNKNOWN", "LOW"): + return meta + detail = meta["error"] or "the parser failed without reporting a reason" + return self._mark_unreadable( + meta, ident, "UnrecognizedFormat", f"could not be read: {detail}", + ) + def _inspect_claimed(self, path: Path, inspector) -> Dict[str, Any]: """Run ``inspector`` on a file a model extension claimed, unless the file is disqualified before any parser is worth running. @@ -569,6 +610,36 @@ def _unreadable_stub( "details": {}, }, path, unreadable_type, message) + @staticmethod + def _peek_head(source, local_path: Path | None) -> bytes | None: + """Read the first bytes of ``source`` without disturbing the caller. + + Used on failure paths only. Tolerates anything: a closed handle, a + stream that will not seek, a vanished file — all of which mean "cannot + classify further", not an error worth raising over. + """ + try: + if local_path is not None: + with open(local_path, "rb") as handle: + return handle.read(len(LFS_POINTER_MAGIC)) + source.seek(0) + return source.read(len(LFS_POINTER_MAGIC)) + except Exception: + return None + + @staticmethod + def _lfs_pointer_verdict(head: bytes | None) -> tuple[str, str] | None: + """Classify ``head`` as a git-LFS pointer, if it is one. + + Shared by the local precheck and the remote failure paths. Remote is + not hypothetical: `raw.githubusercontent.com` serves the pointer text, + not the file, for anything LFS-tracked — so a plain HTTPS scan of a + GitHub-hosted model hits exactly this. + """ + if head and head.startswith(LFS_POINTER_MAGIC): + return ("LfsPointer", LFS_POINTER_MESSAGE) + return None + @staticmethod def _unreadable_precheck(path: Path | None) -> tuple[str, str] | None: """Classify a file that cannot hold a model regardless of extension. @@ -590,13 +661,7 @@ def _unreadable_precheck(path: Path | None) -> tuple[str, str] | None: head = handle.read(len(LFS_POINTER_MAGIC)) except OSError: return None - if head.startswith(LFS_POINTER_MAGIC): - return ( - "LfsPointer", - "this is a git-LFS pointer file, not the model itself — " - "run `git lfs pull` to fetch the real artifact, then re-scan", - ) - return None + return DeepScanner._lfs_pointer_verdict(head) def _record_fetch_error(self, target: str, exc: Exception) -> None: """Record a remote fetch failure as a structured, non-fatal error. @@ -643,8 +708,8 @@ def _identify_container(head: bytes) -> str | None: return label return None - @staticmethod - def _looks_like_pth_config(content: bytes) -> bool: + @classmethod + def _looks_like_pth_config(cls, content: bytes) -> bool: """True if ``content`` is a Python path-configuration file (#131). `.pth` is the one model extension that is also a real text format: @@ -672,12 +737,32 @@ def _looks_like_pth_config(content: bytes) -> bool: continue if line.startswith(("import ", "import\t")): continue - # 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): + if not cls._looks_like_path_entry(line): return False return True + @staticmethod + def _looks_like_path_entry(line: str) -> bool: + """True if ``line`` could be a directory path in a `.pth` file. + + Interior spaces are legal — `site.addpackage` only `rstrip`s a line + before joining it, so `/opt/My Models/site-packages` and the Windows + `C:\\Program Files\\...` shape are both valid entries (Codex review, + PR #115). Rejecting all whitespace would have called those corrupt. + + But accepting *any* spaced text would undo the fix this validator + exists for: `totally not a model at all` would read as a path. So a + line carrying whitespace additionally has to look addressed — it needs + a path separator. Prose does not have one; a directory path does. + """ + if any(ch in "<>" or not ch.isprintable() for ch in line): + # Markup and control characters are in no directory path, and the + # `<` is what rejects an HTML error page saved over a checkpoint. + return False + if any(ch.isspace() for ch in line): + return "/" in line or "\\" in line + return True + @staticmethod def _looks_like_text(content: bytes) -> bool: """True if the first kilobyte decodes as UTF-8 and is mostly printable.""" @@ -901,9 +986,7 @@ def _inspect_pytorch(self, source, name: str | None = None, is_remote: bool = Fa stream.close() except Exception: pass - if unreadable and local_path is not None: - self._mark_unreadable(meta, local_path, *unreadable) - return meta + return self._finalize_inspection(meta, local_path or name, unreadable) def _npy_member_threats(self, blob: bytes, details: Dict[str, Any]): """Scan one `.npy` buffer; return ``(threats, carries_pickle)``. @@ -1130,9 +1213,7 @@ def _inspect_pickle_variant(self, source, name: str | None = None, stream.close() except Exception: pass - if variant_unreadable and local_path is not None: - self._mark_unreadable(meta, local_path, *variant_unreadable) - return meta + return self._finalize_inspection(meta, local_path or name, variant_unreadable) def _inspect_safetensors(self, source, name: str | None = None, is_remote: bool = False) -> Dict[str, Any]: """Reads Safetensors header for Metadata/License.""" @@ -1220,13 +1301,18 @@ def _inspect_safetensors(self, source, name: str | None = None, is_remote: bool f.close() except Exception: pass - if not parsed and local_path is not None: - self._mark_unreadable( - meta, local_path, "TruncatedStream", + if not parsed: + # Re-read the head only now that the parse has already failed, so + # the extra Range request a remote scan pays for it is never paid + # by a healthy file. It buys the LFS-pointer subtype and its + # remedy on the remote path, where the precheck cannot run. + verdict = self._lfs_pointer_verdict(self._peek_head(source, local_path)) + return self._finalize_inspection(meta, local_path or name, verdict or ( + "TruncatedStream", f"SafeTensors header could not be read: " f"{meta.get('error') or 'header shorter than 8 bytes'}", - ) - return meta + )) + return self._finalize_inspection(meta, local_path or name) @staticmethod def _read_gguf_window(f, is_remote: bool) -> bytes: @@ -1423,15 +1509,15 @@ def _inspect_gguf(self, source, name: str | None = None, is_remote: bool = False # Honest before #131, but consequence-free: an `UNKNOWN` label # scores 0 in the CLI's `_risk_score`, below LOW, so the scan # exited 0 and the file rated safer than a clean model. - if local_path is not None: - self._mark_unreadable( - meta, local_path, "UnrecognizedFormat", + return self._finalize_inspection( + meta, local_path or name, + self._lfs_pointer_verdict(self._peek_head(source, local_path)) + or ( + "UnrecognizedFormat", "not a GGUF file — the header does not start with the " "GGUF magic bytes", - ) - else: - meta['risk_level'] = "UNKNOWN (Invalid Header)" - return meta + ), + ) # 2. Read the metadata block in as few reads as possible. # Walking the stream field by field costs one HTTP Range request per @@ -1795,15 +1881,17 @@ def _inspect_keras(self, source, name: str | None = None, is_remote: bool = Fals meta["risk_level"] = self._keras_risk_label( salvage, meta["details"]["lambda_layers"] ) - elif local_path is not None: + else: # Same consequence-free-UNKNOWN fix as GGUF above (#131). - self._mark_unreadable( - meta, local_path, "UnrecognizedFormat", - "not a recognized Keras container — no HDF5 or zip " - "signature found", + return self._finalize_inspection( + meta, local_path or name, + self._lfs_pointer_verdict(self._peek_head(source, local_path)) + or ( + "UnrecognizedFormat", + "not a recognized Keras container — no HDF5 or zip " + "signature found", + ), ) - else: - meta["risk_level"] = "UNKNOWN (Unrecognized Container)" return meta parsed = None @@ -2177,14 +2265,11 @@ def _inspect_onnx(self, source, name: str | None = None, is_remote: bool = False if not looks_like_onnx: # Same consequence-free-UNKNOWN fix as GGUF/Keras (#131). - if local_path is not None: - onnx_unreadable = ( - "UnrecognizedFormat", - "not a parseable ONNX model — the bytes do not " - "deserialize as a ModelProto", - ) - else: - meta["risk_level"] = "UNKNOWN (Unparsable ONNX)" + onnx_unreadable = self._lfs_pointer_verdict(blob) or ( + "UnrecognizedFormat", + "not a parseable ONNX model — the bytes do not " + "deserialize as a ModelProto", + ) else: meta["risk_level"] = self._onnx_risk_label(threats) except Exception as e: @@ -2195,9 +2280,7 @@ def _inspect_onnx(self, source, name: str | None = None, is_remote: bool = False f.close() except Exception: pass - if onnx_unreadable and local_path is not None: - self._mark_unreadable(meta, local_path, *onnx_unreadable) - return meta + return self._finalize_inspection(meta, local_path or name, onnx_unreadable) def _parse_requirements(self, path: Path): try: diff --git a/tests/test_unreadable_artifacts.py b/tests/test_unreadable_artifacts.py index 85c1652..ae486b8 100644 --- a/tests/test_unreadable_artifacts.py +++ b/tests/test_unreadable_artifacts.py @@ -134,6 +134,12 @@ def test_lfs_pointer_error_names_the_remedy(tmp_path): "import os; var = 'SETUPTOOLS_USE_DISTUTILS'; enabled = os.environ.get(var)", "# a comment\n/opt/some/path\n", "/first/path\n/second/path\n", + # Interior spaces are legal: `site.addpackage` only rstrips the line + # before joining it (Codex review, PR #115). The 141-file corpus this + # validator was built against happened to contain no spaced path, so + # the real-tree probe could not have caught this. + "/opt/My Models/site-packages", + "C:\\Program Files\\Python311\\Lib\\site-packages", ], ) def test_real_pth_path_config_still_scans_clean(tmp_path, content): @@ -156,9 +162,13 @@ def test_real_pth_path_config_still_scans_clean(tmp_path, content): @pytest.mark.parametrize( "content", [ + # Prose: spaced, but with no path separator to make it addressed. This + # is the case that stops the whitespace allowance above from swallowing + # the whole fix. "totally not a model at all", "404 Not Found", "version https://git-lfs.github.com/spec/v1\noid sha256:abc\nsize 1\n", + "Traceback (most recent call last): OSError, no such file", ], ) def test_text_that_is_not_a_path_config_is_unreadable(tmp_path, content): @@ -182,6 +192,118 @@ def test_path_config_classification_is_pth_only(tmp_path, ext): assert _artifact(results, f"model{ext}")["framework"] != "Python Path Config" +# --- remote artifacts get the same treatment ------------------------------ +# +# The first cut of this slice guarded every unreadable recording on +# `local_path is not None`, which meant `hf://` and HTTPS scans — the flagship +# "verify before you `git clone`" path — kept the old clean verdict entirely. +# Caught by the Codex review on PR #115. + +def _serve(monkeypatch, content: bytes, url: str): + """Serve ``content`` over the Range-request path the remote scanner uses.""" + import aisbom.remote as remote + + class _Resp: + status_code = 206 + + def __init__(self, body, hdrs): + self.content = body + self.headers = hdrs + + def raise_for_status(self): + return None + + def fake_get(request_url, headers=None): + rng = (headers or {}).get("Range", "bytes=0-0") + start, _, end = rng.split("=")[1].partition("-") + start = int(start) + end = int(end) if end else len(content) - 1 + body = content[start : end + 1] + return _Resp(body, { + "Content-Range": f"bytes {start}-{end}/{len(content)}", + "Content-Length": str(len(body)), + }) + + monkeypatch.setattr(remote, "requests", remote._RequestsStub()) + monkeypatch.setattr(remote.requests, "get", fake_get) + monkeypatch.setattr( + "aisbom.scanner.DeepScanner._resolve_remote_targets", lambda self, t: [url] + ) + return DeepScanner(url).scan() + + +def test_remote_random_bytes_are_not_reported_clean(monkeypatch): + """A remote `.safetensors` of noise used to come back LOW / SafeTensors.""" + url = "http://example.com/random.safetensors" + results = _serve(monkeypatch, b"\x91\x3f" * 100, url) + + assert results["errors"], "a remote unreadable file recorded no error" + assert results["errors"][0]["unreadable"] is True + art = results["artifacts"][0] + assert not art.get("framework") + assert "LOW" not in art["risk_level"] + + +def test_remote_lfs_pointer_names_the_remedy(monkeypatch): + """`raw.githubusercontent.com` serves the pointer, not the file, for + anything LFS-tracked — so this is a real remote case, not a contrived one. + The subtype survives even though the local precheck cannot run.""" + url = "http://example.com/model.safetensors" + results = _serve(monkeypatch, LFS_POINTER, url) + + assert results["errors"][0]["unreadable_type"] == "LfsPointer" + assert "git lfs pull" in results["errors"][0]["error"] + + +# --- a caught exception must not stay a clean verdict --------------------- + +def test_caught_inspector_error_becomes_unreadable(tmp_path): + """Every inspector wraps its body in `except Exception: meta["error"]`, and + nothing read that key. ONNX and SafeTensors seed `risk_level` to LOW, so a + file that threw mid-parse exited 0 as clean (Codex review, PR #115).""" + path = tmp_path / "model.onnx" + path.write_bytes(b"\x08\x07\x12\x04test") # parses far enough to seed LOW + scanner = DeepScanner(tmp_path) + meta = scanner._finalize_inspection( + {"name": "model.onnx", "risk_level": "LOW", "error": "boom"}, path, + ) + + assert meta["unreadable"] is True + assert "boom" in scanner.errors[0]["error"] + + +def test_caught_error_never_downgrades_a_real_finding(tmp_path): + """An inspector can find a CRITICAL payload and *then* throw. Losing that + finding to an "unreadable" label is the one outcome worse than the bug + this slice fixes.""" + scanner = DeepScanner(tmp_path) + meta = scanner._finalize_inspection( + { + "name": "model.pt", + "risk_level": "CRITICAL (RCE Detected: os.system)", + "error": "stream closed early", + }, + tmp_path / "model.pt", + ) + + assert meta["risk_level"].startswith("CRITICAL") + assert "unreadable" not in meta + assert scanner.errors == [] + + +def test_empty_string_error_is_still_caught(tmp_path): + """The git-LFS stub set `meta["error"]` to the empty string, so a + truthiness check on that key would have missed it. Membership is tested.""" + scanner = DeepScanner(tmp_path) + meta = scanner._finalize_inspection( + {"name": "model.safetensors", "risk_level": "LOW", "error": ""}, + tmp_path / "model.safetensors", + ) + + assert meta["unreadable"] is True + assert "without reporting a reason" in scanner.errors[0]["error"] + + # --- regression from the real-tree probe ---------------------------------- def test_pickle_with_trailing_raw_bytes_is_not_unreadable(tmp_path):