Skip to content

fix: oversized files must not produce a zero-finding report (LLM-stage inclusion + AE7 coverage finding) - #509

Open
eitanch228 wants to merge 3 commits into
NVIDIA:mainfrom
eitanch228:main
Open

fix: oversized files must not produce a zero-finding report (LLM-stage inclusion + AE7 coverage finding)#509
eitanch228 wants to merge 3 commits into
NVIDIA:mainfrom
eitanch228:main

Conversation

@eitanch228

@eitanch228 eitanch228 commented Sep 9, 2026

Copy link
Copy Markdown

Summary

A file larger than MAX_ANALYZABLE_FILE_BYTES (16 MiB) currently produces a
zero-finding report even under the default LLM-on scan:

  1. nodes/build_context.py excludes truncated files from llm_file_cache
    entirely, so the semantic LLM analyzers (the scanner's most capable
    stage) never see the file.
  2. The unreviewed region produces no finding: the report shows
    CAUTION / 0 with an empty issues list, and the only trace is
    analysis_completeness.status: "partial" in JSON metadata.

A payload placed past the read cap is therefore invisible while the scan reads
as clean. This is a fail-open: "too large to review" must itself be a finding.

Reproduction

A runnable MCP server whose malicious helpers sit past the 16 MiB mark
(17 MB server.py: benign weather code + a 16.2 MiB string literal before the
cut, exfiltration helpers after it; module-level names resolve at call time,
so the payload still executes, canary-verified):

$ skillspector scan flooded-server --format json        # default, LLM-on
recommendation: CAUTION, score: 0, issues: 0

llm_components contains only manifest.json; server.py never reaches the
model. Confirmed against three model backends (a local Claude CLI login, plus
claude-sonnet-5 and claude-opus-5 reached through an OpenAI-compatible
endpoint with archived prompts): every prompt covered only manifest.json.
The same payload at normal size is flagged DO_NOT_INSTALL / 100 by all
three backends, so the miss is structural, not a model-capability failure.

Fix

Two parts, both required:

1. build_context.py: keep truncated files in the LLM stage.
Truncated text files are cached in llm_file_cache as a bounded prefix
(MAX_LLM_TRUNCATED_FILE_CHARS, 256 KiB) plus an explicit marker:

[SKILLSPECTOR: this file is 16978656 bytes; only the first 16777216 bytes were
readable and the excerpt above is capped at 262144 characters. The remaining
bytes were not reviewed by any analyzer - treat the unseen region as an audit
gap.]

LLMAnalyzerBase.get_batches already chunks oversized content into
token-budgeted batches, so no batching changes are needed; the 256 KiB cap
keeps worst-case token cost bounded (~2-8 chunks depending on model budget).

2. finalize_inspection_ledger.py: new AE7 coverage finding (HIGH,
analysis-evasion).
Any artifact left partial by the per-file size cap
produces a finding: "File exceeds the analyzable size limit; trailing content
was not inspected." This extends the existing AE1 principle ("referenced
artifact was not completely inspected") to artifacts nothing referenced.
Paths already covered by AE1 are skipped (no double-reporting). Scoped to the
per-file cap (size_limit); aggregate budget exhaustion already fails closed
through the completeness projection and the SAFE -> CAUTION upgrade in
nodes/report.py.

After the fix, the same files under the same default scan flip to
DO_NOT_INSTALL / 100:

PoC Stock scanner Patched scanner
Flooded exfil server (17 MB) CAUTION / 0, 0 findings DO_NOT_INSTALL / 100, 9 findings
Flooded RCE server (17 MB, neutrally named) CAUTION / 0, 0 findings DO_NOT_INSTALL / 100, 9 findings

With only the bounded head + marker available, the model catches the evasion
structurally: it flags the 17 MB repeated-sentence padding as having "no
benign explanation" (SSD-2), the unconditional _sync_telemetry /
_check_for_service_update call chain in the visible head (SDI-1), the
docstring as "social engineering" (SSD-4), and AE7 names the file and its
true size. Even a perfectly innocuous-looking head still cannot produce a
clean report, because AE7 fires regardless.

Why not full-file chunked review

Reading and reviewing every byte of arbitrary-size files is a token-cost
policy decision (a 16 MiB file is ~4M tokens, hundreds of calls per
analyzer). The results above show the fail-closed minimum is sufficient in
practice: a bounded head + an explicit gap marker lets the model identify
both the payload's call sites and the padding itself as evasion, and AE7
guarantees the remainder is never a silent gap. Happy to take maintainer
guidance on a follow-up that chunks the full file behind an opt-in budget
flag.

Test plan

  • test_truncated_text_file_stays_in_llm_cache_with_audit_gap_marker:
    truncated file stays in llm_file_cache / llm_components, carries the
    marker, still reported partial
  • test_size_truncated_artifact_synthesizes_ae7: disposition/reason
    matrix (fires on partial+size_limit only)
  • test_ae7_skips_paths_already_covered_by_ae1: no double-reporting
  • Full suite: 3991 passed, 14 skipped, 4 xfailed, no regressions
  • End-to-end, static (--no-llm), 17 MB PoC: CAUTION / 0, 0 findings
    -> CAUTION / 32, 1 HIGH (AE7)
  • End-to-end, default LLM-on (local Claude CLI provider), both 17 MB
    PoCs (exfil + RCE): CAUTION / 0, 0 findings -> DO_NOT_INSTALL / 100,
    9 findings each, 4/4 LLM calls OK
  • Normal-size fixtures unchanged: benign SAFE/0, naive control
    DO_NOT_INSTALL/100
  • Maintainer CI

@eitanch228
eitanch228 marked this pull request as draft September 9, 2026 15:02
@eitanch228
eitanch228 marked this pull request as ready for review September 9, 2026 15:03
@eitanch228

Copy link
Copy Markdown
Author

Hey , @hartsock @rodboev @korjavin
would love for you to take a look at this and get back to me.

Thanks!!

A file larger than MAX_ANALYZABLE_FILE_BYTES is excluded from
llm_file_cache entirely, so the semantic LLM analyzers never see it,
and its unreviewed region produces no finding: the report shows
CAUTION / 0 with an empty issues list while a payload past the read
cap stays invisible (fail-open).

Two-part fix:

1. build_context: cache a bounded prefix (MAX_LLM_TRUNCATED_FILE_CHARS)
   of truncated text files in llm_file_cache with an explicit
   audit-gap marker, instead of excluding them. The existing batcher
   already chunks oversized content into token-budgeted batches.

2. finalize_inspection_ledger: new AE7 coverage finding (HIGH,
   analysis-evasion) for artifacts left partial by the per-file size
   cap, extending the AE1 principle to artifacts nothing referenced.
   Paths already covered by AE1 are skipped; aggregate budget
   exhaustion stays handled by the existing fail-closed verdict
   upgrade.

Signed-off-by: eitanch228 <eitan.ch@pluto.security>
@eitanch228 eitanch228 reopened this Sep 10, 2026
@liadt-venos

Copy link
Copy Markdown

Token Usage

Sessions on main · updated 2026-09-10 10:51 IDT

Model Input Output Cache write Cache read Total Cost
Claude Code claude-opus-5 61,538 15,847,975 36,917,460 7,752,684,019 7,833,853,692 $2079.48
Claude Code claude-sonnet-5 25,428 9,669,547 28,314,896 4,438,935,235 4,485,230,702 $1203.18
Claude Code 336 111,547 648,885 63,076,837 64,082,149 $17.25
Claude Code claude-fable-5-1 20,308 655,758 0 49,407,586 51,163,590 $82.40
Codex gpt-5.6-sol 12,974,646 59,965 0 12,570,112 25,604,723 $18.39
Codex gpt-5.6-terra 0 181 0 0 181 $0.002
Total 12,459,935,037 $3400.70

Boost saved ~17.9M context tokens across these sessions — filtered tool output that never reached the model.

16 sessions · some rows estimated (no provider usage)Powered by JFrog Boost

@MohammedAlkindi

Copy link
Copy Markdown

Windows check: at the merge-base (69dcdfb) and at b24eed5, both touched test files give the same 12 failed / 2 skipped; the PR adds 6 passing (105 -> 111), no new failures.

One hardening question. grep -rn "\[SKILLSPECTOR" src/ returns one hit, the new line 771, so this is the first scanner-authored text to enter llm_file_cache. It reaches the model framed only by File: <path> and line numbers (llm_analyzer_base.py:382, :569), so a forged [SKILLSPECTOR: ...] line inside the 256 KiB prefix renders identically to the real one. The real marker can't be suppressed, only impersonated by one claiming the tail was reviewed and benign. AE7 still fires, so the verdict holds; the model reasoning your PoC table relies on does not.

Would a per-scan nonce be worth it?

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Reviewed head b24eed5c967bd64622b4baea224b2376dcdfcca4 — APPROVE.

Oversized text now reaches the semantic stage through a bounded view, while deterministic AE7 coverage ensures the unreviewed tail cannot produce a zero-finding result. Referenced paths avoid duplicate AE1/AE7 findings, aggregate limits retain their existing completeness gate, and the tests cover the disposition matrix. The model-visible marker is not an authorization boundary; deterministic AE7 remains the fail-closed control. I found no required changes.

Required checks pass, but GitHub currently reports mergeStateStatus=BEHIND; update against current main and re-run required checks before merging.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Reviewed head 84786ffbdf19b838c07ffccf6b6ef7ecd0d81652 — APPROVE.

I re-reviewed the complete replacement head. Its only change since the previously assessed commit is the upstream README badge merged from main; the PR's five-file delta is unchanged. Oversized text reaches the semantic stage through a bounded view, while deterministic AE7 coverage ensures the unreviewed tail cannot produce a zero-finding result. Referenced paths avoid duplicate AE1/AE7 findings, aggregate limits retain their existing completeness gate, and the tests cover the disposition matrix. The model-visible marker is not an authorization boundary; deterministic AE7 remains the fail-closed control. I found no required changes.

Merge remains blocked until all current-head required checks finish successfully and GitHub reports a clean merge state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants