diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8adc6f3..ab99e74 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,6 +32,13 @@ jobs: # Detection quality is a reviewable number, not a claim. Printed on every run so a # precision or recall change shows up in the diff of two CI logs. - run: python -m tools.bench + # Scored separately: a corpus whose negatives were selected from observed + # failures is biased towards passing, so the blended number flatters. + - run: python -m tools.bench --field-only + # A rule declaring UNBOUNDED reads whole minified lines, so a non-linear pattern + # there is a denial-of-service vector. Enforced for patterns nobody has written + # yet, rather than remembered by whoever reviews the PR. + - run: python -m tools.measure_linearity --check - uses: actions/upload-artifact@v7 if: matrix.python-version == '3.12' with: diff --git a/CHANGELOG.md b/CHANGELOG.md index 1034045..62298e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,7 +101,13 @@ All notable changes follow [Keep a Changelog](https://keepachangelog.com/en/1.1. repository you own, or move those keys to an operator-supplied file. `agentguard init` now emits the repository-safe subset. -## [0.1.0] - 2026-07-16 +## [0.1.0] - 2026-07-16 — NEVER PUBLISHED + +> **This release does not exist.** The entry below was written in advance and the release +> was never cut: no `v0.1.0` tag was ever pushed, and nothing was published to PyPI. It is +> kept rather than deleted because a corrected record is better history than a clean one — +> and because anyone reading `[0.1.0]` elsewhere in this file needs to know it never +> shipped. The first published release will be `0.2.0`. ### Added @@ -111,5 +117,5 @@ All notable changes follow [Keep a Changelog](https://keepachangelog.com/en/1.1. - Configurable severity thresholds, rule suppression, severity overrides, and module/entry-point plugins. - Docker image, typed Python package, tests, CI, CodeQL, dependency review, release workflow, and open-source governance files. -[Unreleased]: https://github.com/amic25/agentguard/compare/v0.1.0...HEAD -[0.1.0]: https://github.com/amic25/agentguard/releases/tag/v0.1.0 +[Unreleased]: https://github.com/amic25/agentguard/commits/main +[0.1.0]: # (never published - no tag exists) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 275e0c4..c69aa45 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -39,3 +39,28 @@ Use imperative, descriptive commits such as `Add MCP wildcard permission rule`. ## Community standards Participation is governed by [CODE_OF_CONDUCT.md](CODE_OF_CONDUCT.md). Report vulnerabilities privately according to [SECURITY.md](SECURITY.md). + +## Adding or changing a rule + +- Declare `languages` on `RuleMetadata`. It is required; a rule that does not say which + languages it applies to would be run against lockfiles and CI configuration. +- Add both a true positive and a **true negative** to `tests/corpus/`, each with a `why`. + A pattern with no negative case is untested surface. `make bench` reports the effect. +- If the rule declares `max_line_length=UNBOUNDED`, run `python -m tools.measure_linearity` + and confirm every pattern is linear. CI enforces this with `--check`. An unbounded + non-linear pattern is a denial-of-service vector, not a slow rule. +- Never reuse a retired rule ID — see [docs/RULE_IDS.md](docs/RULE_IDS.md). + +## Test harnesses + +**Run every new harness against a deliberately broken input before trusting it.** Five +harnesses in this project's history reported success while measuring nothing: two +false-positive probes that could not distinguish the states they compared, a corpus entry +that could never reach the policy it claimed to pin, an aggregation check whose quoted +variable stopped it ever splitting, and an equivalence run that truncated every line before +comparing, excluding the only inputs that could have shown the regression. A harness that +cannot fail is not a harness. + +The converse is also on record: the linearity gate caught a quadratic pattern written by the +same person who had just written the gate, minutes earlier. See +[docs/DECISIONS.md](docs/DECISIONS.md#evidence-that-the-gate-works-it-caught-its-own-author). diff --git a/Makefile b/Makefile index 449a68d..8e9ec54 100644 --- a/Makefile +++ b/Makefile @@ -19,6 +19,7 @@ test: bench: python -m tools.bench + python -m tools.bench --field-only check: lint type test bench diff --git a/README.md b/README.md index 3544f20..fb4ff0c 100644 --- a/README.md +++ b/README.md @@ -1,25 +1,98 @@ -
- AgentGuard — AI Agent Security Scanner +# AgentGuard -

Find dangerous agent capabilities before they reach production.

+Static security scanner for AI agent applications, in Python and JavaScript/TypeScript. +It looks for committed credentials, prompt-injection paths, unsafe command execution, +excessive tool permissions, unrestricted file access, risky outbound calls, missing tool +validation, missing approval gates, and unpinned dependencies. - [![CI](https://github.com/amic25/agentguard/actions/workflows/ci.yml/badge.svg)](https://github.com/amic25/agentguard/actions/workflows/ci.yml) - [![CodeQL](https://github.com/amic25/agentguard/actions/workflows/codeql.yml/badge.svg)](https://github.com/amic25/agentguard/actions/workflows/codeql.yml) - [![PyPI](https://img.shields.io/pypi/v/agentguard-sast?color=14b8a6)](https://pypi.org/project/agentguard-sast/) - [![Python](https://img.shields.io/pypi/pyversions/agentguard-sast)](https://pypi.org/project/agentguard-sast/) - [![License](https://img.shields.io/badge/license-Apache--2.0-2563eb)](LICENSE) -
- -AgentGuard is an open-source static security scanner for AI agent applications. It looks for leaked secrets, prompt-injection paths, excessive permissions, unsafe system access, weak tool validation, risky outbound calls, missing human approval, and unpinned dependencies in Python and JavaScript/TypeScript projects. - -It is offline-first, CI-friendly, framework-aware, and designed to produce findings a developer can fix—not a wall of vague warnings. +Offline. Never executes the code it scans. Designed to be run on repositories you have +not read. ```bash pipx install agentguard-sast agentguard scan ./project ``` -

AgentGuard terminal scan showing prioritized findings

+## How well does it work? + +Measured against a labelled corpus in this repository. Reproduce it with `make bench`: + +``` +| Rule | TP | FP | FN | Precision | Recall | +|-------|----:|----:|----:|----------:|-------:| +| AG001 | 4 | 0 | 0 | 100.0% | 100.0% | +| AG002 | 2 | 0 | 0 | 100.0% | 100.0% | +| AG003 | 1 | 0 | 0 | 100.0% | 100.0% | +| AG004 | 1 | 0 | 0 | 100.0% | 100.0% | +| AG005 | 2 | 0 | 0 | 100.0% | 100.0% | +| AG006 | 1 | 0 | 0 | 100.0% | 100.0% | +| AG007 | 1 | 0 | 0 | 100.0% | 100.0% | +| AG008 | 1 | 1 | 0 | 50.0% | 100.0% | +| AG010 | 1 | 0 | 0 | 100.0% | 100.0% | +| **all** | 14 | 1 | 0 | 93.3% | 100.0% | +``` + +**93.3% precision and 100% recall over 34 labelled files** — 13 true positives and 21 true +negatives, each carrying a written reason for its label in `tests/corpus/manifest.yml`. +That is the only accuracy figure this project publishes, because it is the only one it can +reproduce. + +13 of the 21 true negatives reproduce a false positive observed on a real project; the other +8 were composed to cover awkward cases — a credential in a docstring, `eval` on a literal, +`subprocess` with a fixed argument vector, a `.env` full of shell interpolation. Every case +declares which it is, and `make bench` scores the field-derived subset separately: + +``` +33 of 34 cases behave as labelled. # all cases +12 of 13 cases behave as labelled. # --field-only +``` + +The one failure in both is AG008, described below. + +**This is a regression gate, not a precision estimate.** It says known defects stay fixed +on 34 cases chosen partly because they once broke. It is not a prediction about your +repository and should not be read as one: a corpus this size cannot support that claim, and +a corpus whose negatives were selected from observed failures is biased towards passing by +construction. The field-only score exists to make that bias visible rather than argue it +away. + +There is also a [labelled dataset](datasets/field-2026-07-29/) of 73 findings from five +real agent projects. It is **not** an accuracy claim: it is not reproducible from this +repository, and its labels carry a bias documented in that directory — the same reader +labelled them twice and disagreed with himself on a quarter of them, always in the same +direction. It ships so the labels can be argued with. + +## Known limitations + +Read these before deciding whether to run it. + +- **AG008 has a known false positive**, at 50% precision on the corpus. It cannot tell a + tool deleting a caller-supplied path from a function deleting a temp file it created + itself; that needs data flow it does not have. Recorded as a strict `xfail` so it + cannot be quietly closed. See [docs/DECISIONS.md](docs/DECISIONS.md). +- **Rules have been narrowed to remove false positives, losing recall in the process.** + AG003 no longer flags `function_map` at all; AG005 no longer matches `file_path="/"`; + AG006 no longer flags a call merely because its argument is named `url`; AG007 no longer + matches a JS tool registered via a plain `function(`; AG002 no longer flags `eval` or + `exec` over a literal or a module-level constant. Every trade and its reasoning is in + [docs/DECISIONS.md](docs/DECISIONS.md). +- **Findings in `tests/`, `examples/`, `docs/`, and vendored paths are downgraded or + suppressed.** A real credential committed under `tests/` reports at Medium and will not + fail your build. This is deliberate — fixtures were the largest false-positive source + measured — but it is a real blind spot if your layout is unusual. +- **AG004's pattern is quadratic, bounded at 4096 characters per line — not linear.** + Rewriting it took a 28 KB single line from 34 seconds to 32 milliseconds, but the curve + is still roughly 4× per doubling, so the safety comes from the cap rather than the + rewrite. AG001 opts out of the cap in order to read minified bundles whole, and is held + linear by `python -m tools.measure_linearity --check` in CI. +- **JavaScript and TypeScript are analysed lexically**, not with a full parser. Python + gets an AST; JS/TS gets regexes over comment- and string-aware regions. +- **No dependency vulnerability scanning.** AG009 was deleted; use `pip-audit`, + `osv-scanner`, or Dependabot. See [#16](https://github.com/amic25/agentguard/issues/16). +- **Unverified:** Windows and macOS. Every test run has been Linux. SARIF is schema-valid + and confirmed to render in GitHub code scanning; nothing else is confirmed. + +A clean scan is not a certification. It means these rules found nothing. ## Why AgentGuard? @@ -27,17 +100,17 @@ Agent applications combine untrusted natural-language input with credentials, to | What it checks | Examples | Rule | |---|---|---| -| Secrets | OpenAI/AWS/GitHub keys, private keys, assigned credentials | `AG001` | +| Secrets | OpenAI/AWS/GitHub keys, private keys, assigned credentials, `.env` values | `AG001` | | Code execution | `eval`, `os.system`, shell subprocesses, Node child processes | `AG002` | | Tool permissions | broad LangChain/CrewAI tools, dangerous flags, MCP wildcards | `AG003` | | Prompt injection | untrusted web, document, request, or tool output in instructions | `AG004` | | File access | user-controlled paths and broad filesystem roots | `AG005` | -| External APIs | plaintext HTTP, caller-controlled URLs, SSRF paths | `AG006` | +| External APIs | plaintext HTTP, and requests taking a named untrusted source | `AG006` | | Input validation | tools without strict typed or JSON schemas | `AG007` | | Agent privileges | consequential actions without approval gates | `AG008` | | Dependencies | unpinned requirements | `AG010` | -AgentGuard recognizes common patterns from LangChain, CrewAI, AutoGen, OpenAI Agents/API applications, and MCP clients/servers. The rules are framework-tolerant: they inspect the security behavior rather than requiring one exact SDK version. +The rules were written against patterns from LangChain, CrewAI, AutoGen, OpenAI Agents, and MCP clients and servers, and inspect behaviour rather than requiring one SDK version. Coverage of any given framework is whatever the corpus demonstrates — see `tests/corpus/`. ## Install @@ -200,16 +273,20 @@ python -m pip install -e '.[dev]' make check ``` -Contributions are welcome. Start with [CONTRIBUTING.md](CONTRIBUTING.md) or one of the [good first issue specifications](docs/GOOD_FIRST_ISSUES.md). Security reports should follow [SECURITY.md](SECURITY.md), not a public issue. +Contributions are welcome, and a report that a finding is wrong is the most useful kind — +see [SUPPORT.md](SUPPORT.md). Start with [CONTRIBUTING.md](CONTRIBUTING.md) or the +[good first issues](docs/GOOD_FIRST_ISSUES.md). Security reports follow +[SECURITY.md](SECURITY.md), not a public issue. + +Reference: [decisions and their costs](docs/DECISIONS.md) · +[rule identifier policy](docs/RULE_IDS.md) · [CI configuration notes](docs/CI_SETUP.md) ## Roadmap -- Deeper framework data-flow analysis and cross-file call graphs -- Dependency scanning delegated to pip-audit/osv-scanner, normalized into one report ([#16](https://github.com/amic25/agentguard/issues/16)) -- Policy packs for MCP, financial, healthcare, and enterprise agents -- Baseline/diff scanning for gradual adoption -- IDE integrations and an LSP -- Signed releases and Homebrew packaging +- Data flow sufficient to close AG008's known false positive +- Corpus coverage for AG003 and AG006, which the current corpus under-tests +- Dependency scanning delegated to pip-audit/osv-scanner, normalised into one report ([#16](https://github.com/amic25/agentguard/issues/16)) +- Baseline/suppression file so the tool can be adopted on a repository that is already dirty Details and acceptance criteria live in [ROADMAP.md](ROADMAP.md). diff --git a/SUPPORT.md b/SUPPORT.md new file mode 100644 index 0000000..f3d04af --- /dev/null +++ b/SUPPORT.md @@ -0,0 +1,48 @@ +# Support + +## Reporting a security issue + +Not here. See [SECURITY.md](SECURITY.md) — please do not open a public issue for a +vulnerability in AgentGuard itself. + +## A finding looks wrong + +This is the most useful kind of report, and the project is set up to receive it. + +A false positive is a defect. Open an issue with the smallest snippet that reproduces it +and what you expected instead. Better still, open a pull request adding it to +`tests/corpus/true_negatives/` with a manifest entry explaining why it should not fire — +`make bench` will then fail on it, and no fix counts as complete until it does. + +The same applies in reverse: something AgentGuard missed belongs in +`tests/corpus/true_positives/`. + +We publish a labelled dataset of real findings at `datasets/field-2026-07-29/`, including +which judgements we are least sure about. Disputing a label there is a genuine +contribution — it says so in that file's README. + +## A rule's severity looks wrong + +Severity is a judgement and judgements are arguable. Two we have already changed after +being wrong: a credential published by design is not a critical compromise, and a +framework implementing its own documented capability is not an application granting one. +If you think another is miscalibrated, say so. + +## Questions about using it + +Open a discussion or an issue. Useful things to include: the command, the version +(`agentguard --version`), and the exit code — `0` clean, `1` findings at or above the +threshold, `2` the scan did not complete. + +## What this project will not do + +- Publish accuracy numbers it cannot reproduce. The only figures in the README come from + `make bench` against a corpus in this repository. +- Ship a vulnerability advisory database. Dependency scanning is delegated to `pip-audit`, + `osv-scanner`, and Dependabot, which own that data properly. See issue #16. +- Reuse a retired rule ID. See [docs/RULE_IDS.md](docs/RULE_IDS.md). + +## Response times + +This is a small project with no support commitment. Security reports get priority; see +SECURITY.md for that path. diff --git a/WORKLOG.md b/WORKLOG.md index 4241118..996f815 100644 --- a/WORKLOG.md +++ b/WORKLOG.md @@ -751,3 +751,533 @@ Or merge once, leaving the defect in place: ``` gh pr merge 17 --repo amic25/agentguard --squash --admin ``` + +--- + +## Unit 12 — STOPPED: two stop conditions triggered — 2026-07-29 + +Status: **stopped, awaiting a decision.** Queue items 2, 4, and 10 consume the field +number and were not started. Nothing was fixed. + +### Stop condition 1 — the hand triage materially disagrees + +Re-triaged a deterministic stratified sample of 20 of the 73 field findings (round-robin +by rule so every rule appears), reading each with four lines of surrounding context. + +**Strict agreement: 13/20 = 65%. Divergence: 5/20 = 25%. Two further partials.** + +| # | Rule | New verdict | Earlier | | +|---|---|---|---|---| +| 1 | AG001 | FP — PostHog `phc_` is a public, write-only ingest key | TP | **diverge** | +| 2 | AG002 | FP — `exec(_NAMESPACE_IMPORTS, ns)`, a module constant | TP | **diverge** | +| 3 | AG003 | FP — crewAI setting delegation on its own manager agent | TP | **diverge** | +| 4 | AG005 | FP — `path = "/" + path` is a normaliser | FP | agree | +| 5 | AG006 | FP — fixed host, https, timeout | FP | agree | +| 6 | AG008 | FP — human-typed CLI, not agent-autonomous | borderline | partial | +| 7 | AG001 | FP-substance, downgraded by policy | same | agree | +| 8 | AG002 | TP — `exec(code, ns)` from MCP tool input | TP | agree | +| 9 | AG003 | FP — framework internals, same as #3 | TP | **diverge** | +| 10 | AG006 | FP — fixed host, timeout | FP | agree | +| 11 | AG008 | TP (weak) — agent-invocable delete, sandboxed | borderline | agree | +| 12 | AG001 | FP-substance, downgraded | same | agree | +| 13 | AG002 | TP — `exec(compile(...))` of a flow script | TP | agree | +| 14 | AG003 | FP — local dict named `function_map` | FP | agree | +| 15 | AG008 | FP — internal temp-file cleanup | borderline | partial | +| 16 | AG001 | FP-substance — the line *asserts the key is absent* | same | agree | +| 17 | AG002 | FP — `shell=True` but every argv element is constant | TP | **diverge** | +| 18 | AG003 | FP — local named `function_map` | FP | agree | +| 19 | AG008 | TP (weak) — agent-driven apply_patch delete | borderline | agree | +| 20 | AG001 | FP-substance, downgraded | same | agree | + +**All five divergences run the same direction: earlier TP, now FP.** That is a systematic +optimism bias, not noise. Two recurring causes: + +1. **Committed ≠ compromisable.** A PostHog project key and a Supabase anon key are + published deliberately. Reporting them as Critical "credential compromise" is wrong + even though a credential is literally committed. +2. **Framework internals ≠ application configuration.** `allow_delegation = True` inside + crewAI's own `_create_manager_agent` is the framework implementing its documented + mode. AG003 cannot tell a library defining a capability from an application granting one. + +Also: `subprocess.run([cmd, ...], shell=True)` where `cmd` iterates a constant list is a +real portability bug and not a security finding — no attacker input reaches it. + +**Methodological caveat, stated plainly:** this is the same reader re-reading, not an +independent triage. Genuine independence needs a second person. A same-reader re-read +diverging 25% is a floor on the error, not a measurement of it. + +**Consequence.** The earlier claim that "roughly 12 of the 17 gating findings are genuine" +does not survive. On this sample the gating TPs are #8, #11, #13, #19 — and two of those +are weak. **The 233 → 73 and 208 → 17 headline is directionally right but its quality +split is not trustworthy, and nothing should be optimised against it until re-triaged.** + +### Stop condition 2 — truncation is a real detection hole + +**The 1,039,776-line equivalence run proved nothing about over-cap lines.** The harness +contained `ln = ln[:4096]`, so every line was truncated *before* comparison. The +population that could show the regression was excluded by construction. This is the same +class of error as the earlier bad probes, and it is the fourth occurrence. + +How much of that corpus was over-cap: **1 line in 1,039,808** (browser-use +`demo_mode.py:485`, 19,359 chars). So the equivalence claim itself is barely weakened — +but only because those five projects contain almost no minified code. + +Detection past the cap, tested against realistic minified-bundle shapes: + +``` +key_at_100 AG001 findings=1 truncated_lines=0 +key_at_3000 AG001 findings=1 truncated_lines=0 +key_at_5000 AG001 findings=0 truncated_lines=1 <-- missed +key_at_20000 AG001 findings=0 truncated_lines=1 <-- missed +``` + +**A credential past character 4096 of a minified line is not detected.** The scan reports +`truncated_lines=1` and exits 0. It is disclosed, but the finding is gone, and "clean" +is what a reader takes from exit 0. + +This is a recall defect in the flagship category. Bundled JS with an inlined key is a +common real leak, and it is precisely the shape that exceeds the cap. + +Not fixed, per instruction. Options, unevaluated: raise the cap for secret rules only; +run secret patterns over untruncated content while other rules stay bounded; or treat a +truncated line as scan incompleteness (exit 2) rather than a reported statistic. + +### Claim surfaces outside version control + +Asked for. Found, beyond the GitHub About field being fixed by hand: + +- **GitHub repo topics** — `gh api repos/OWNER/REPO/topics`. Currently includes `sast` + and `security`, no false claim, but it is an unversioned claim surface. +- **PyPI project page** — renders `pyproject.toml` `description` and README at publish + time and cannot be edited without a release. Nothing is published yet, so this is the + last moment it is free. +- **The social preview image** (`docs/assets/social-preview.svg` is in-repo, but the + *uploaded* preview is a repo setting). +- **Issue #16's own body**, which describes future dependency scanning. +- `docs/assets/demo.svg` is in-repo and greppable, but it renders numbers as an image, so + a text sweep will not catch a stale figure inside it. It already needed a manual edit + once (10 rules → 9). + +### Not done + +Queue items 2 (corpus repair), 4 (mechanism split), and 10 (README) all consume the field +number and are blocked behind a re-triage decision. Items 5–9 are independent and were not +started, because the queue is ordered and both stop conditions fired in item 1 and item 3. + +Bench delta: none — no code changed. +Decisions taken alone: ran item 3 out of order, because it is itself a declared stop +condition and answering it costs minutes; reporting one stop condition while a second was +knowably true would have wasted a round trip. + +--- + +## Unit 13 — truncation, credential class, vendored paths, corpus repair — 2026-07-29 + +Status: complete +Changed: `context.py`, `rules/base.py`, `rules/secrets.py`, `rules/code.py`, `models.py`, +`scanner.py`, `reporters.py`, `cli.py`, `tools/measure_linearity.py` (new), +`tests/test_coverage.py` (new), `tests/test_rule_context.py`, `tests/test_redos.py`, +6 new corpus true negatives, `datasets/field-2026-07-29/` (new), `docs/assets/demo.svg` + +Verified: +``` +ruff format --check src tests tools → 31 files already formatted +ruff check src tests tools → All checks passed! +mypy src → Success: no issues found in 15 source files +pytest -q → 171 passed, 93.37% coverage +python -m tools.measure_linearity → all linear, worst exponent 1.02 +``` + +### Truncation — option (b), with the measurement done first + +`tools/measure_linearity.py` measures growth exponent per pattern. **It validates itself +against the pre-fix cubic AG004 pattern before reporting**; if that control does not come +back non-linear it exits 2 without printing results. This is the checklist item, applied +at the point where it matters — the harness that measures whether a pattern is safe to +run unbounded is the last place a silent pass is acceptable. + +``` +control (known-cubic AG004): exponent 2.83 — NON-LINEAR ← harness registers +OpenAI API key 1.00 0.19ms@32KB 6.3ms@1MB linear +AWS access key 1.00 0.13ms 4.1ms linear +GitHub token 1.02 0.18ms 6.1ms linear +private key 1.00 0.06ms 2.0ms linear +assigned credential 1.00 1.21ms 39.6ms linear +``` + +**No AG001 pattern needs a finite cap.** All declare `UNBOUNDED`; worst case at the 1 MB +file limit is 40 ms. + +Bounds now live in `RuleMetadata.max_line_length`: `None` inherits the configured bound, +`0` (`UNBOUNDED`) opts out. The scanner sets `source.active_bound` per rule, so a rule +reads `source.lines` without knowing its own declaration. Only AG001 opts out. + +The hole is closed: +``` + before after +key at 100 detected detected +key at 3,000 detected detected +key at 5,000 MISSED detected +key at 20,000 MISSED detected +key at 100,000 (untested) detected +``` + +Coverage is declared, not gated. Every report carries which lines were clipped, by how +much, against which bound — JSON `coverage`, a Markdown section, SARIF +`toolExecutionNotifications` at `note` level, terminal summary. `--fail-on-incomplete` +opts into exit 2. Default exit codes are unchanged. + +Coverage reporting uses the **tightest** bound that actually applied, not the loosest. +With AG001 unbounded and everything else at 4096, `max()` would have reported nothing +once any rule ran unbounded. Caught while writing it, not by a test. + +### Field coverage, now visible + +The five projects contain **286 lines that no bounded rule read in full** — 270 in crewAI +alone, 15 in langgraph. Previously invisible. AG001 reads all of them whole. + +### credential_class + +`public` caps at Low with a "publishable by design" message and its own remediation +("no rotation required if this is genuinely the publishable key"). Classified by vendor +value prefix (`phc_`, `pk_live_`, `pk_test_`) or by the assigned identifier containing +`public`/`publishable`/`anon`/`client_id`. Enforced centrally in `Scanner._admit`. + +Gating findings fell 17 → 15, entirely from the two public keys dropping to Low. + +### Vendored paths + +`vendor/`, `third_party/`, `site-packages/`, `dist-packages/`, `node_modules/`, +`bower_components/`, `.venv/`, `eggs/`, `bundled/`, `external/` now downgrade on the same +footing as fixtures. No field effect here — those paths are in `DEFAULT_EXCLUDES` — but it +binds when a user overrides excludes, which is when it matters. + +### shell=True with an argument list + +Now described as what it is: on POSIX the shell receives only `argv[0]` and later +arguments are silently discarded. Separate message, `defect_class: portability` metadata, +still reported. `shell=True` with a *string* keeps the injection framing. + +### Corpus repair — precision fell, and that is the point + +Six field false positives folded in as true negatives. + +``` + precision before after +AG001 100.0% 100.0% +AG002 100.0% 66.7% +AG003 100.0% 50.0% +AG004 100.0% 100.0% +AG005 100.0% 66.7% +AG006 100.0% 50.0% +AG007 100.0% 100.0% +AG008 100.0% 50.0% +AG010 100.0% 100.0% +ALL 100.0% 72.2% recall unchanged at 100% +``` + +**This is not a regression. The rules did not get worse; the corpus stopped flattering +them.** The previous 100% was measured against cases written after the bugs were known — +teaching to the test. AG003 and AG006 scored 100% while measuring ~98–100% false +positives in the field; that gap was a corpus validity failure and is now visible. + +Five failures are now reproducible in CI and none is fixed: +`exec_of_module_constant.py` (AG002), `framework_lookup_maps.py` (AG003), +`path_normaliser.py` (AG005), `fixed_host_requests.py` (AG006), +`internal_cleanup.py` (AG008). Per the standing loop, each is now a thing the corpus can +fail on, which is the precondition for calling it fixed later. + +### Field dataset + +`datasets/field-2026-07-29/` — 73 findings, 16 labelled, 57 explicitly not. Records the +five upstream commit SHAs, the sampling method, and the same-reader bias in plain terms +including that all five divergences moved the same direction. States which two judgements +are most worth disputing, and that the tool now encodes one of them, so if the judgement +is wrong the tool is wrong. **Not cited in the README and must not be.** + +### demo.svg + +Baked-in numbers removed (`27 files`, `9 rules in 84ms`, `1 critical · 1 high · 1 medium`) +rather than generated from bench — the file is hand-authored SVG and a generator is more +machinery than the claim is worth. Remaining numerals are rule IDs and example line +numbers in illustrative findings, which are not accuracy claims. + +Bench delta: 100.0% → 72.2% precision, recall unchanged at 100%. Stated above. +Decisions taken alone: +1. **`--fail-on-incomplete` exits 2, not 1.** Exit 1 means "found problems at threshold"; + incomplete coverage is not a finding. 2 already means "this result is not a clean bill + of health", which is exactly the claim. The default is unchanged, per instruction. +2. **`publishable_keys.py` is labelled a true positive, not a true negative.** The + assertion is about severity, not about whether it fires. Pinned separately by + `test_publishable_key_is_capped_at_low`. +3. **demo.svg numbers removed rather than generated.** + +### Repo and PR actions + +- Merged Dependabot #3 (`upload-artifact` 4→7). The other five were `BLOCKED`: they + predate the `ci-ok` job, so a required context can never report on them, and their + `review` runs predate the dependency-graph fix. Requested `@dependabot rebase` on all + five; they should go green once rebased onto current main. +- #12 (`.mts`/`.cts`): commented asking it to declare `languages` and add corpus coverage, + explaining that discovery without declaration means fewer rules run, not more. +- #14 (Google API keys): commented asking for a true-negative corpus case, plus the + linearity check now that AG001 runs unbounded, and raising whether referrer-restricted + Google keys belong in `credential_class: public`. +- #13 (GitLab CI): commented that we're taking it, flagged the exit-code change. +- #15 (GitLab CI, overlapping): replied with credit, named the material it has that #13 + lacks (report artifacts), invited a rebase as a follow-up. **Not closed.** + +Nothing closed. No tag. Work PR not merged. + +--- + +## Unit 14 — coverage-bound test, generic linearity gate, five dispositions, docs — 2026-07-29 + +Status: complete +Changed: `context.py`, `scanner.py`, `rules/secrets.py`, `rules/code.py`, +`tools/measure_linearity.py`, `tests/test_coverage.py`, `tests/test_linearity_gate.py` (new), +`tests/test_rule_context.py`, `tests/test_corpus.py`, `tests/test_rules.py`, +`tests/corpus/manifest.yml`, 2 new corpus files, `README.md`, `CHANGELOG.md`, +`CONTRIBUTING.md`, `SUPPORT.md` (new), `docs/DECISIONS.md` (new), `docs/CI_SETUP.md` (new), +`.github/workflows/ci.yml` + +Verified: +``` +ruff format --check src tests tools → ok +ruff check src tests tools → ok +mypy src → ok +pytest -q → 195 passed, 1 xfailed +python -m tools.measure_linearity --check → exit 0 +agentguard scan src --fail-on medium → exit 0 +python -m tools.bench → 93.3% precision, 100% recall +``` + +### Session recovery + +The connection dropped mid-README-write. Re-established from disk: branch +`post-merge/queue-1`, nothing committed since `8f2c4b8`, all work uncommitted (14 modified, +6 new). Nothing truncated; the README rewrite had landed complete but with the labelled-file +count wrong in two places. + +### 1. Coverage computes against the tightest bound — now pinned + +`test_coverage_uses_the_tightest_applied_bound` runs two rules bounded at 100 and 4096 over +a 200-char line. Verified it fails when `min` is flipped to `max`. + +A second test, `test_an_unbounded_rule_does_not_erase_coverage_for_bounded_ones`, does *not* +discriminate min from max — positive bounds are filtered before either applies, so +`UNBOUNDED` cannot win the comparison. Its original docstring claimed otherwise; corrected +rather than left, and the docstring now says which test does the discriminating. + +### 2. The linearity gate is generic and runs in CI + +`tools/measure_linearity.py` now discovers every rule declaring `UNBOUNDED` and every +compiled pattern on it by introspection, builds stress inputs from each pattern's own +literal runs, and takes the worst exponent. `--check` runs in `ci.yml`. + +**It immediately caught a denial-of-service vector added in this same session.** The first +`_env_assignment` pattern used `[A-Z0-9_]*KEYWORD[A-Z0-9_]*` — two unbounded quantifiers +around an alternation — and measured **exponent 2.00, 918 ms on 32 KB, extrapolating to +~16 minutes on a 1 MB line**, on a rule that reads lines unbounded. Rewritten to capture +the name once and test it in Python: exponent 0.99, 9.3 ms at 1 MB. + +That is the gate paying for itself before it was even committed. + +### 3. Dispositions for the five corpus failures + +| Rule | Corpus case | Disposition | +|---|---|---| +| AG003 | `framework_lookup_maps.py` | **FIXED** — `function_map\s*=` clause removed entirely | +| AG002 | `exec_of_module_constant.py` | **FIXED** — module-level constants resolved; only arg 0 is checked | +| AG005 | `path_normaliser.py` | **FIXED** — lookahead rejects a concatenated root | +| AG006 | `fixed_host_requests.py` | **FIXED** — `url`/`uri`/`endpoint` dropped from the untrusted-source list | +| AG008 | `internal_cleanup.py` | **ACCEPTED** — needs data flow; strict `xfail` marker | + +AG003's clause was removed rather than narrowed: even where it hit AutoGen's real +parameter it flagged a function map's *existence*, not an over-broad one, so a clause that +cannot express the breadth the rule is named for does not belong in it. + +AG002 needed two fixes, not one. Resolving module constants was insufficient because the +check required *every* argument to be fixed, and `exec(code, namespace)` passes a globals +dict second — so it never fired on the two-argument form, which is the common one. Only +argument 0 is executed. + +AG006's narrowing broke `tests/test_rules.py`, which asserted AG006 fires on `fetch(url)` — +the exact false positive being removed. The expectation was wrong and is now +`fetch(user_input)`. That is the recall trade made visible by an existing test. + +Corpus precision: **72.2% → 93.3%**, recall unchanged at 100%. + +### Queue items 6, 5, 8, 9, 10 + +- **6 CHANGELOG:** `[0.1.0] - 2026-07-16` annotated `NEVER PUBLISHED` with the reasoning, + not deleted. Dead compare/tag links replaced. +- **5 `.env`:** **decided to scan it.** A secrets scanner that structurally cannot read the + canonical secrets file is indefensible. `.env` carries no extension so the suffix map + never reached it. Values there are conventionally *unquoted*, which the quoted + assigned-credential pattern misses entirely — so there is a second, env-file-only + pattern; requiring quotes is what keeps the general one from matching + `password = get_password()`. `.env.example` and friends classify as fixtures. + `DEBUG=true` and `PORT=8080` do not fire. +- **8 hygiene:** LICENSE, CONTRIBUTING, SECURITY, CODE_OF_CONDUCT already existed and were + left alone. Added `SUPPORT.md` and `docs/CI_SETUP.md`, the latter recording that required + status contexts are check-run names picked from GitHub's suggestion list and never typed + — three phantom contexts (`test`, `pytest`, `CI`) blocked merges for exactly that reason. +- **9 `docs/DECISIONS.md`:** eleven decisions, each with its cost first. +- **10 README:** rewritten around `make bench`. Headline is **93.3% / 100% over 31 labelled + files**, AG008 named as the single failure. No badge wall. History of the number is not + narrated there — it lives here and in DECISIONS.md. + +### README number audit + +Every figure checked against a command, after the count was found wrong: + +``` +31 labelled → manifest 31, files on disk 31 (13 TP + 18 TN) +12 field-derived → 12 of 18 true negatives cite a measurement; 6 do not +73 dataset → findings.json holds 73 records across 5 projects +4096 cap → DEFAULT_MAX_LINE_LENGTH == 4096 +28 KB / 34 s / 32 ms / ~4x → re-measured: 28.1 KB, 34.94 s, 32.62 ms, 3.9x per doubling +bench table → diffed byte-for-byte against `python -m tools.bench`; identical, and the + comparison was verified to detect a tampered figure +``` + +Three claims were **wrong and corrected**: +1. "26 labelled files" — actually 31, wrong in two places. +2. "every true negative was drawn from a false positive observed on real code" — false; + 12 of 18 are, 6 were written from a checklist. +3. The recall-trade list named AG007 among "four rules narrowed" — AG007 was narrowed in + earlier work, not among these dispositions. Rewritten to name each rule and its trade + without a count. + +Also narrowed the rule table's "caller-controlled URLs" for AG006, which after the +narrowing overclaims — it now requires a *named* untrusted source. + +Bench delta: 72.2% → 93.3% precision, recall unchanged at 100%. +Decisions taken alone: +1. **README states 93.3%, not the 72.2% in the instruction.** The instruction predated the + dispositions requested in the same message; 72.2% is superseded and `make bench` + reproduces 93.3%. Reproducibility was the stated principle, so the live number wins. +2. **AG008 named as the one remaining failure**, not five — the other four were fixed in + this unit. + +--- + +## Unit 15 — .env measured, AG006 disposed, origin split, gate evidence — 2026-07-29 + +Status: complete. Five units, **committed individually** — the previous session ran fully +uncommitted through two connection drops, and lost nothing only by luck. + +``` +8bd272a Measure .env scanning, which shipped without it +fc3e4b7 Convert the wrong AG006 assertion into a corpus true negative +3615924 Mark every corpus case's origin and score the field-derived subset separately +0648ace Record the linearity gate catching its own author, with the measurements +``` +Unit D needed no commit — see below. + +### 1. `.env` measured, and it was wrong twice + +`.env` support shipped with two corpus files and no coverage of the shapes that occur. +Adding them found two false positives immediately: + +- `SERVICE_TOKEN=$OTHER_TOKEN` reported **Critical**. The placeholder filter matched + `${VAR}` but not a bare `$VAR`. An unbraced shell reference is a pointer to a value held + elsewhere, not the value. +- `sk-proj-replace-this-before-running` in a template reported at Medium. It carries no + word the filter recognised. + +Both are *value-shape* problems, not file-classification ones, so the fix went in the +placeholder filter rather than in how templates are handled. That distinction is +load-bearing: a genuine credential committed to `.env.example` is a real leak and is still +reported. Suppressing by filename would have hidden it. + +`.env.local` now covers commented-out credentials, empty values, braced and unbraced +interpolation, sub-length values, and ordinary config — each failing to fire for a +different reason. Verified non-vacuous: reverting the filter drops precision to 82.4% and +names both cases. + +**AG001 declares UNBOUNDED, so the linearity gate was re-run after touching its patterns.** +Still linear, 9 patterns across 1 rule. This is now the habit the gate is for. + +### 2. AG006's `fetch(url)` test — disposed as a corpus true negative + +Converted to `tests/corpus/true_negatives/js_fetch_local_url.js`. A wrong assertion is +worse than no assertion: it defends the defect against the change that fixes it, which is +exactly what happened — narrowing AG006 broke this test, and that is how the trade +surfaced. + +Added to the recall-trades table as the sixth entry, and called out as the sharpest: a +genuinely attacker-controlled URL reaching `fetch(url)` is now missed. The table header +said "five times" while listing six; fixed. + +### 3. Origin marked explicitly; field-derived scored separately + +Every entry now declares `origin: field | written`, **enforced by bench** rather than +inferred. The prior inference sniffed `why` for "measured"/"observed" and miscounted the +moment an entry said "field finding" instead — which happened one commit earlier, and is +why the README's split was left unquantified in unit 2 rather than guessed at. + +``` +33 of 34 cases behave as labelled. # all +12 of 13 cases behave as labelled. # --field-only +``` +Same single AG008 failure in both. `make bench` runs both; CI runs both. + +A precision figure over the field-derived subset is structurally skewed — a field false +positive becomes a corpus *true negative*, so that subset is nearly all negatives and its +50% precision says less than it appears to. "Behave as labelled" is the statistic that +survives, and is what the README quotes. + +Two labels corrected: `.env.local` and `.env.sample` are `written`, not `field`. The false +positives they caught were found by composing them, not by scanning anything. + +### 4. README framing — already correct, no commit + +The regression-gate framing landed in unit 1 and was strengthened in unit 3. Verified +present rather than re-added: *"This is a regression gate, not a precision estimate… a +corpus whose negatives were selected from observed failures is biased towards passing by +construction. The field-only score exists to make that bias visible rather than argue it +away."* Adding it twice would have been the kind of make-work this queue is meant to avoid. + +### 5. The gate catching its own author — now on record with numbers + +`docs/DECISIONS.md` gained a section. The `.env` pattern written *minutes after the gate +itself* used two unbounded quantifiers around an alternation: + +``` +before: exponent 2.00 917.86ms @32KB 990822.2ms @1MB SUPER-LINEAR +after: exponent 0.99 0.29ms @32KB 9.3ms @1MB linear +``` + +The recorded point is not that a mistake happened. It is that the gate's author, fully +aware of why it existed, immediately wrote the thing it guards against and did not notice +until a machine measured it. A review checklist could not have caught it — the reviewer +wrote the checklist. That is the argument for CI over convention, and for `--check` +failing rather than warning. + +CONTRIBUTING's harness-failure tally goes four → five: the equivalence run that truncated +every line before comparing belongs on that list, having excluded the only inputs that +could have shown the regression. + +### Verified + +``` +ruff format --check src tests tools → ok +ruff check src tests tools → ok +mypy src → ok +pytest -q → 197 passed, 1 xfailed +python -m tools.bench → 93.3% / 100%, 33 of 34 behave +python -m tools.bench --field-only → 12 of 13 behave +python -m tools.measure_linearity --check → exit 0, 9 patterns linear +``` +README numbers re-checked against commands: 34 labelled, 13 TP, 21 TN, 13 field, 8 written, +both behave-lines, and the table diffed byte-for-byte against `tools.bench`. + +Bench delta: none from units 2–5. Unit 1 changed the placeholder filter; precision held at +93.3% because the two new cases were added and fixed in the same commit, and the revert +check confirms they would otherwise score 82.4%. +Decisions taken alone: +1. **Field-only reports "behave as labelled" rather than leading with precision**, because + precision over a subset selected from observed failures is skewed by construction. +2. **Unit D closed without a commit**, the framing already being present and verified. diff --git a/datasets/field-2026-07-29/README.md b/datasets/field-2026-07-29/README.md new file mode 100644 index 0000000..1200f39 --- /dev/null +++ b/datasets/field-2026-07-29/README.md @@ -0,0 +1,66 @@ +# Field measurement — 2026-07-29 + +73 findings from scanning five real open-source AI agent projects. Published so the +labels can be disputed rather than taken on trust. + +**These numbers are not cited in the project README, and should not be.** They are not +reproducible from this repository — they depend on five external repositories at +particular commits — and the labelling has a known bias described below. The only +accuracy figures AgentGuard publishes are the ones `make bench` reproduces from +`tests/corpus/`. + +## Method + +Each project was cloned at the commit below and scanned with default configuration: + +``` +agentguard scan --format json --fail-on none +``` + +| Project | Commit | +|---|---| +| `browser-use` | `f0aa3a8bb03779c71a5aa262d389e3bfe6b77cdc` | +| `langgraph` | `41341457342327166d72fc11952ab28fb61ec0bf` | +| `python-sdk` | `6f69a3758ebf2ee55ce050f58b470ce11af71133` | +| `openai-agents-python` | `e75cdd2e2c76f7930d894c6f46174cb091fc724f` | +| `crewAI` | `f15844b21966e35dff2f656ce8724b985703043c` | + +`findings.json` holds every finding with `project`, `rule_id`, `severity`, `confidence`, +`path`, `line`, and — where one was assigned — a `label` and `rationale`. + +Labels: `true_positive`, `true_positive_weak`, `false_positive`, or `null` for unlabelled. + +## Coverage of the labelling + +**16 of 73 are labelled. 57 are not.** The labelled set is a deterministic stratified +sample drawn round-robin by rule, so every rule appears, plus the findings that gate CI +at the default threshold. It is not a random sample and the unlabelled remainder should +not be assumed to follow the same distribution. + +## Known bias — please read before using these labels + +**All labels were assigned by the same reader, twice, months apart in effort but not in +perspective.** The second pass disagreed with the first on 5 of 20 findings — 25% — and +**every one of those five moved the same direction: previously true positive, now false +positive.** + +That is a systematic optimism bias in the first pass, and the second pass may carry its +own in the other direction. A same-reader re-read diverging 25% establishes a floor on +the error, not a measurement of it. + +Two judgements did most of the work and are the ones most worth arguing with: + +1. **A committed credential that is published by design is not a compromise.** PostHog + project keys and Supabase anon keys are meant to ship. Labelled false positive despite + a credential being literally committed. AgentGuard now reports these at Low rather + than Critical, so the tool encodes this judgement — if it is wrong, the tool is wrong. +2. **A framework implementing its own documented capability is not an application + granting one.** `allow_delegation = True` inside crewAI's own `_create_manager_agent` + is labelled false positive. Someone could reasonably argue a library shipping a + permissive default deserves a finding. + +## Disputing a label + +Open an issue or a PR editing `findings.json`. A label changed with a stated reason is +strictly more useful than the current one. Labelling any of the 57 unlabelled findings is +the single most valuable contribution to this dataset. diff --git a/datasets/field-2026-07-29/findings.json b/datasets/field-2026-07-29/findings.json new file mode 100644 index 0000000..8cf93b6 --- /dev/null +++ b/datasets/field-2026-07-29/findings.json @@ -0,0 +1,732 @@ +[ + { + "project": "browser-use", + "rule_id": "AG002", + "severity": "Critical", + "confidence": "high", + "path": "browser_use/mcp/cli_mcp.py", + "line": 105, + "label": "false_positive", + "rationale": "exec over a module-level constant; no attacker-controlled input" + }, + { + "project": "browser-use", + "rule_id": "AG002", + "severity": "Critical", + "confidence": "high", + "path": "browser_use/mcp/cli_mcp.py", + "line": 128, + "label": "true_positive", + "rationale": "exec(code, ns) where code is MCP tool input" + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Critical", + "confidence": "high", + "path": "browser_use/telemetry/service.py", + "line": 21, + "label": "false_positive", + "rationale": "PostHog phc_ is a public write-only ingest key; committed but not compromisable" + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/ci/security/test_sensitive_data.py", + "line": 379, + "label": null, + "rationale": null + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/ci/security/test_sensitive_data.py", + "line": 401, + "label": null, + "rationale": null + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/ci/security/test_sensitive_data.py", + "line": 437, + "label": null, + "rationale": null + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/ci/security/test_sensitive_data.py", + "line": 512, + "label": null, + "rationale": null + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/ci/security/test_sensitive_data.py", + "line": 538, + "label": null, + "rationale": null + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/ci/test_beta_agent.py", + "line": 3472, + "label": null, + "rationale": null + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/ci/test_beta_agent.py", + "line": 3476, + "label": null, + "rationale": null + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/ci/test_beta_agent.py", + "line": 3480, + "label": null, + "rationale": null + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/ci/test_beta_agent.py", + "line": 3484, + "label": null, + "rationale": null + }, + { + "project": "browser-use", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/ci/test_beta_agent.py", + "line": 3492, + "label": null, + "rationale": null + }, + { + "project": "langgraph", + "rule_id": "AG001", + "severity": "Critical", + "confidence": "high", + "path": "libs/cli/langgraph_cli/constants.py", + "line": 5, + "label": null, + "rationale": null + }, + { + "project": "langgraph", + "rule_id": "AG001", + "severity": "Critical", + "confidence": "high", + "path": "libs/sdk-py/langgraph_sdk/auth/types.py", + "line": 341, + "label": null, + "rationale": null + }, + { + "project": "langgraph", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "libs/sdk-py/tests/test_skip_auto_load_api_key.py", + "line": 37, + "label": null, + "rationale": null + }, + { + "project": "langgraph", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "libs/sdk-py/tests/test_skip_auto_load_api_key.py", + "line": 66, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG002", + "severity": "Critical", + "confidence": "high", + "path": "src/mcp/cli/cli.py", + "line": 48, + "label": "false_positive", + "rationale": "shell=True with a constant argv list; portability bug, not injection" + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "docs_src/identity_assertion/tutorial001.py", + "line": 57, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "docs_src/identity_assertion/tutorial002.py", + "line": 29, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "examples/snippets/clients/identity_assertion_client.py", + "line": 63, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "examples/snippets/servers/identity_assertion_server.py", + "line": 59, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "examples/stories/identity_assertion/server.py", + "line": 23, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "examples/stories/oauth_client_credentials/server.py", + "line": 20, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/client/auth/extensions/test_client_credentials.py", + "line": 320, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/docs_src/test_identity_assertion.py", + "line": 73, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/interaction/auth/test_bearer.py", + "line": 37, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/server/auth/middleware/test_bearer_auth.py", + "line": 97, + "label": null, + "rationale": null + }, + { + "project": "python-sdk", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/server/auth/middleware/test_bearer_auth.py", + "line": 108, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG003", + "severity": "High", + "confidence": "high", + "path": "src/agents/realtime/session.py", + "line": 954, + "label": "false_positive", + "rationale": "local dict named function_map" + }, + { + "project": "openai-agents-python", + "rule_id": "AG008", + "severity": "High", + "confidence": "medium", + "path": "src/agents/run_internal/tool_actions.py", + "line": 891, + "label": "true_positive_weak", + "rationale": "agent-driven apply_patch delete" + }, + { + "project": "openai-agents-python", + "rule_id": "AG003", + "severity": "High", + "confidence": "high", + "path": "src/agents/run_internal/turn_resolution.py", + "line": 1744, + "label": "false_positive", + "rationale": "local named function_map" + }, + { + "project": "openai-agents-python", + "rule_id": "AG008", + "severity": "High", + "confidence": "medium", + "path": "src/agents/sandbox/capabilities/tools/apply_patch_tool.py", + "line": 223, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/extensions/experiemental/codex/test_codex_tool.py", + "line": 1812, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/extensions/memory/test_advanced_sqlite_session.py", + "line": 168, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/extensions/memory/test_advanced_sqlite_session.py", + "line": 1489, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/extensions/sandbox/test_cloudflare.py", + "line": 478, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/extensions/sandbox/test_daytona.py", + "line": 269, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/extensions/sandbox/test_runloop.py", + "line": 1631, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/extensions/sandbox/test_runloop.py", + "line": 1875, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/extensions/sandbox/test_vercel.py", + "line": 2711, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/sandbox/test_docker.py", + "line": 2049, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/sandbox/test_docker.py", + "line": 2050, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/sandbox/test_memory.py", + "line": 1514, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/sandbox/test_mounts.py", + "line": 328, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/test_agent_as_tool.py", + "line": 2482, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/test_agent_runner.py", + "line": 2790, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/test_error_logging_redaction.py", + "line": 56, + "label": null, + "rationale": null + }, + { + "project": "openai-agents-python", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "tests/test_function_tool.py", + "line": 890, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG002", + "severity": "Critical", + "confidence": "high", + "path": "lib/crewai/src/crewai/flow/runtime/_actions.py", + "line": 274, + "label": "true_positive", + "rationale": "exec(compile(...)) of a flow script" + }, + { + "project": "crewAI", + "rule_id": "AG008", + "severity": "High", + "confidence": "medium", + "path": "lib/cli/src/crewai_cli/cli.py", + "line": 607, + "label": "false_positive", + "rationale": "human-typed CLI command, not agent-autonomous" + }, + { + "project": "crewAI", + "rule_id": "AG008", + "severity": "High", + "confidence": "medium", + "path": "lib/crewai-tools/src/crewai_tools/tools/daytona_sandbox_tool/daytona_file_tool.py", + "line": 274, + "label": "true_positive_weak", + "rationale": "agent-invocable delete, sandboxed" + }, + { + "project": "crewAI", + "rule_id": "AG008", + "severity": "High", + "confidence": "medium", + "path": "lib/crewai-tools/src/crewai_tools/tools/daytona_sandbox_tool/daytona_file_tool.py", + "line": 366, + "label": "false_positive", + "rationale": "internal temp-file cleanup" + }, + { + "project": "crewAI", + "rule_id": "AG003", + "severity": "High", + "confidence": "high", + "path": "lib/crewai/src/crewai/crew.py", + "line": 1502, + "label": "false_positive", + "rationale": "framework setting delegation on its own manager agent" + }, + { + "project": "crewAI", + "rule_id": "AG003", + "severity": "High", + "confidence": "high", + "path": "lib/crewai/src/crewai/crew.py", + "line": 1520, + "label": "false_positive", + "rationale": "framework constructing its own manager agent" + }, + { + "project": "crewAI", + "rule_id": "AG005", + "severity": "High", + "confidence": "high", + "path": "lib/crewai/src/crewai/memory/utils.py", + "line": 61, + "label": "false_positive", + "rationale": "path = \"/\" + path is normalisation" + }, + { + "project": "crewAI", + "rule_id": "AG006", + "severity": "Medium", + "confidence": "high", + "path": "lib/crewai-tools/src/crewai_tools/tools/contextualai_query_tool/contextual_query_tool.py", + "line": 49, + "label": "false_positive", + "rationale": "fixed host, TLS, timeout" + }, + { + "project": "crewAI", + "rule_id": "AG006", + "severity": "Medium", + "confidence": "high", + "path": "lib/crewai-tools/src/crewai_tools/tools/merge_agent_handler_tool/merge_agent_handler_tool.py", + "line": 97, + "label": "false_positive", + "rationale": "fixed host, timeout" + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai-tools/tests/tools/brave_search_tool_test.py", + "line": 102, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai-tools/tests/tools/singlestore_search_tool_test.py", + "line": 326, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai-tools/tests/tools/stagehand_tool_test.py", + "line": 225, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai-tools/tests/tools/stagehand_tool_test.py", + "line": 227, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/a2a/test_a2a_integration.py", + "line": 208, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/agents/test_agent.py", + "line": 2116, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/llms/hooks/test_openai_interceptor.py", + "line": 217, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/llms/openai_compatible/test_openai_compatible.py", + "line": 152, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/skills/test_registry.py", + "line": 513, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/test_context.py", + "line": 33, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/test_context.py", + "line": 61, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/test_context.py", + "line": 90, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/test_context.py", + "line": 103, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/test_context.py", + "line": 173, + "label": null, + "rationale": null + }, + { + "project": "crewAI", + "rule_id": "AG001", + "severity": "Medium", + "confidence": "low", + "path": "lib/crewai/tests/test_context.py", + "line": 214, + "label": null, + "rationale": null + } +] diff --git a/docs/CI_SETUP.md b/docs/CI_SETUP.md new file mode 100644 index 0000000..1df476b --- /dev/null +++ b/docs/CI_SETUP.md @@ -0,0 +1,55 @@ +# CI configuration notes + +Things that cost this project time, written down so they cost it once. + +## Required status checks are check-run names, never workflow names + +**Always pick required contexts from GitHub's suggestion list. Never type one by hand.** + +Three phantom contexts have blocked merges on this repository: + +| Typed | What it actually is | Result | +|---|---|---| +| `test` | the matrix publishes `test (3.10)` … `test (3.13)` | never reports | +| `pytest` | a step inside a job, not a job | never reports | +| `CI` | the workflow's `name:`, not a job | never reports | + +A required context that no job produces can never be satisfied, so every pull request sits +at `BLOCKED` with all checks green. It fails silently and looks like a permissions problem. +Two separate merges stalled on this before the cause was found. + +The suggestion list is populated from contexts GitHub has actually seen, so a name picked +from it is by construction one that reports. + +## A matrix makes its own names unstable + +`test (3.10)` changes the moment the supported-version list does. Requiring those directly +means editing branch protection every time Python releases. + +The `ci-ok` job in `ci.yml` exists for this: it `needs` every other job in the workflow, +runs with `if: always()`, and fails on any dependency result that is not `success` — +covering `failure`, `cancelled`, `skipped`, and any value GitHub adds later, because a gate +that passes on a state it does not recognise is not a gate. Require `ci-ok`; let the matrix +change underneath it. + +## Cross-workflow jobs cannot be aggregated + +`needs:` only works within one workflow, so `ci-ok` cannot cover CodeQL or the container +build. Those are required separately by their own check-run names. + +## A committed `requirements.txt` is a real dependency manifest + +GitHub's dependency graph ingests any file with that name, including test fixtures. Adding +a corpus fixture named `requirements.txt` gave this repository two dependency manifests +describing packages it does not use, and dependency review then failed on a genuine +advisory against one of them. + +The corpus fixtures therefore use fictional package names, and say so in a header comment. +The same applies to any filename external tools parse by convention: `package.json`, +`pyproject.toml`, lockfiles, and workflow YAML under `.github/`. + +## Dependabot PRs predating a new required check + +A branch cut before a required job existed cannot report that context, so it blocks +forever. `@dependabot rebase` picks up the new job. Five PRs needed this after `ci-ok` was +added, and one needed a second rebase after earlier merges touched the same file. diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md new file mode 100644 index 0000000..6dfd3ad --- /dev/null +++ b/docs/DECISIONS.md @@ -0,0 +1,238 @@ +# Decisions + +Load-bearing calls and what each cost. Reasoning and price, not narrative. A decision +that reads as free was probably not examined closely enough. + +--- + +## Delete AG009 rather than ship a stale advisory database + +AG009 bundled three hand-maintained CVE advisories and read only `requirements*.txt` and +`package.json`. Across five real agent projects — 4,750 files — it fired zero times. + +**Cost:** AgentGuard reports nothing about vulnerable dependencies, and users who expected +that must run a second tool. Deleting it also retired the ID permanently. + +**Why anyway:** a near-empty vulnerability database that never fires is worse than none, +because a clean result implies dependencies were checked. Maintaining a real one is a +data-operations commitment this project has not made. `pip-audit`, `osv-scanner`, and +Dependabot own that data properly. Issue #16 tracks shelling out to them and normalising +into `Finding`, which keeps one report without owning the data. + +--- + +## Make the config trust boundary a type, not a convention + +`plugins` is absent from `RepoConfig` entirely, rather than filtered out of it. + +**Cost:** a repository owner who wants `plugins`, `disabled_rules`, or +`severity_overrides` must pass `--config` explicitly instead of having it picked up +automatically. That is friction on the common case to defend against the uncommon one. + +**Why anyway:** a scanned repository could previously name modules under `plugins:` and +have them imported — arbitrary code execution from the tool's primary advertised use case, +which then reported clean and exited 0. A check can be dropped in a refactor; a field that +does not exist cannot be. `Config.tightened_by` is a meet, so a hostile repository can make +its own scan stricter and never laxer. + +--- + +## Exit code honours completeness, and 2 outranks 1 + +`0` clean, `1` findings at or above threshold, `2` the scan did not complete. + +**Cost:** conditions that used to exit 0 now exit 2, so automation treating 0 as "clean" +sees new failures. + +**Why anyway:** it was being misled. A rule crashing on every file exited 0 — +indistinguishable from a clean scan, with CI green over zero coverage. SARIF already +reported this correctly via `executionSuccessful`; only the exit code disagreed. 2 outranks +1 because "the tool broke" and "the tool found problems" need different responses. + +--- + +## Truncation is declared, not gated + +Lines beyond a rule's bound are reported in every output format. `--fail-on-incomplete` +opts into exit 2; the default does not gate. + +**Cost:** a caller who ignores the coverage section gets an incomplete scan without being +stopped. + +**Why anyway:** a bounded read is a known limitation, not a malfunction, and gating on it +by default would fail on any repository containing a minified bundle. Silence was the real +problem — the scan said nothing and exited 0. Naming what was not read is the honest +middle, and the flag exists for callers who need certainty. + +--- + +## Bounds are per rule, and only measured-linear patterns may go unbounded + +AG001 declares `UNBOUNDED` and reads whole minified lines. Everything else stays at 4096. + +**Cost:** an unbounded rule is a denial-of-service vector if any of its patterns +backtracks. The guarantee has to be enforced forever, for patterns nobody has written yet. + +**Why anyway:** a credential at offset 5,000 of a one-line bundle was previously invisible, +and that is a common real leak in exactly the file shape that exceeds the bound. The cost is +paid by `tools/measure_linearity.py --check` in CI, which discovers unbounded rules and their +patterns by introspection and fails the build on anything non-linear. + +### Evidence that the gate works: it caught its own author + +The `.env` support added in the same session needed a pattern for unquoted values. The first +attempt was: + +```python +r"(?i)^\s*(?:export\s+)?[A-Z0-9_]*(?:api[_-]?key|secret|token|password|passwd|pwd)" +r"[A-Z0-9_]*\s*=\s*(?!['\"])(\S{12,})\s*$" +``` + +Two unbounded `[A-Z0-9_]*` spans around an alternation. It was written, reviewed by its +author, and looked fine. The gate measured it: + +``` +AG001 _env_assignment exponent 2.00 917.86ms @32KB 990822.2ms @1MB SUPER-LINEAR +``` + +Quadratic. **918 milliseconds on 32 KB, extrapolating to roughly 16 minutes on a single +1 MB line** — on the one rule that reads lines unbounded, in a tool whose entire premise is +running on repositories nobody has read. A hostile file could have hung the scan. + +Rewritten to capture the name once and test it in Python instead of matching it with +wildcards: + +``` +AG001 _env_assignment exponent 0.99 0.29ms @32KB 9.3ms @1MB linear +``` + +The point is not that a mistake was made. It is that a competent author, having just written +the gate and being fully aware of why it existed, then wrote exactly the class of pattern it +guards against — and did not notice until a machine measured it. A review checklist would not +have caught this; the reviewer was the person who wrote the checklist. That is the argument +for the gate being CI rather than a convention, and for `--check` failing the build rather +than printing a warning. + +The gate validates itself against a known-cubic pattern before reporting, and refuses to +report at all if that control comes back linear. A measurement tool that cannot demonstrate +it detects the thing it looks for is not evidence of anything. + +--- + +## A credential published by design is capped at Low + +PostHog project keys, Stripe publishable keys, and anything named `*_PUBLIC_*` / `*_ANON_*` +report at Low with a "publishable by design" message. + +**Cost:** if the classification is wrong — someone commits a secret key whose name says +public — a real compromise is reported at Low. The heuristic is name-and-prefix based and +can be fooled. + +**Why anyway:** two of five real projects were reported as having Critical committed +credentials, and both were keys meant to ship in client code. Reporting those as critical +compromise is simply wrong, and a scanner that cries wolf on published keys gets ignored on +real ones. Capped rather than dropped, because the publishable and secret halves are easy +to confuse and it is still worth knowing the value is there. + +--- + +## Test, example, and vendored paths are downgraded, not silenced + +Secret findings there report at Medium with low confidence; every other rule is suppressed. + +**Cost:** a real credential committed under `tests/` never gates CI. Real code that happens +to live under `examples/` is under-reported. Path-based classification is a heuristic and +a project with an unusual layout loses findings silently. + +**Why anyway:** fixtures were the single largest false-positive source measured. Secrets are +downgraded rather than suppressed specifically because live credentials genuinely do reach +test fixtures. Clamped to Medium rather than decremented one step, because `CRITICAL→HIGH` +still trips the default `--fail-on high` and would not have removed any noise at all. + +--- + +## Recall traded for precision, six times + +Each was kept because precision was the binding constraint for that rule in the field. + +| Change | Recall cost | +|---|---| +| AG005 requires `path` as a whole word | `file_path="/"`, `mount_path="/"` no longer match | +| AG007 dropped bare `function(` | a JS tool registered via a plain function expression is missed | +| Non-secret rules suppressed on fixture paths | AG010 went from 13 field findings to 0 — every hit was under `examples/` | +| `eval`/`exec` over literals and module constants | a literal that is nonetheless dangerous is not reported | +| Call-name resolution returns `""` for non-name receivers | `get_module().system(cmd)` is missed | +| AG006 requires a *named* untrusted source | `fetch(url)` and `requests.get(endpoint)` are missed, whatever the value's provenance | + +The alternative in each case was a rule measuring ~100% false positives, which is a rule +nobody leaves switched on. + +The AG006 trade is the sharpest of the six, because a genuinely attacker-controlled URL +reaching `fetch(url)` is now missed. It was taken because the rule cannot see provenance at +all: `url` is simply the ordinary name for a variable holding a URL, and treating the name +as evidence produced false positives on every call in the field sample and true positives +on none. Narrowing to names that *do* imply untrusted origin keeps the cases the rule can +actually justify. Closing the gap properly needs data flow, not a longer name list. + +This trade was found by an existing test failing: `tests/test_rules.py` asserted AG006 +should fire on `fetch(url)`. That assertion was wrong, and a wrong assertion is worse than +no assertion, because it defends the defect against exactly the change that fixes it. It is +now `tests/corpus/true_negatives/js_fetch_local_url.js`. + +--- + +## AG003's `function_map` clause removed rather than narrowed + +**Cost:** AutoGen configurations using `function_map` are no longer flagged at all. + +**Why anyway:** the clause matched any local variable of that name — measured twice in +openai-agents-python on ordinary dict comprehensions — and even where it hit the real +parameter it flagged a function map's *existence*, not an over-broad one. A clause that +cannot express the breadth the rule is named for does not belong in it. Narrowing to a +keyword-argument context would not have fixed that, only the false matches. + +--- + +## AG008's internal-cleanup false positive accepted, not fixed + +`sandbox.fs.delete_file(temp_path)` cleaning up a file the code itself created is reported. + +**Cost:** AG008 sits at 50% precision on the corpus, and this is the reason. + +**Why anyway:** distinguishing a tool deleting a caller-supplied path from a function +deleting its own temp file needs data flow the rule does not have, and inventing a +name-based heuristic — "`temp_` prefixes are safe" — would be a guess dressed as analysis +and would create a blind spot an attacker could name their way into. Recorded as +`tests/test_rule_context.py::test_ag008_internal_cleanup_is_a_known_limit`, a strict +`xfail`: if it ever passes, the limit closed and this decision needs revisiting. + +--- + +## Scan `.env`, do not declare it out of scope + +**Cost:** a new discovery shape, a second credential pattern for unquoted values, and +`.env` files are often gitignored, so a scan of a clean checkout may find nothing while a +developer's working copy is full of live keys. + +**Why anyway:** a secrets scanner that structurally cannot read the canonical secrets file +is indefensible. `.env` carries no extension, so the suffix map never reached it. Values +there are conventionally unquoted, which the quoted assigned-credential pattern misses +entirely, so the pattern is env-file-only — requiring quotes is what stops it matching +`password = get_password()` in ordinary source. `.env.example` and friends are treated as +fixtures, since a committed template holds placeholders by convention. + +--- + +## Field numbers are published as a dataset, not cited as a metric + +`datasets/field-2026-07-29/` holds 73 findings and 16 labels. The README quotes only +`make bench`. + +**Cost:** the most impressive numbers this project has — a 92% reduction in findings that +gate CI — appear nowhere in the README. + +**Why anyway:** they are not reproducible from this repository, and the labels have a known +bias: the same reader labelled them twice and disagreed with himself on 5 of 20, with all +five moving the same direction. A number that cannot be re-derived and whose labels are +disputed is a marketing claim. The dataset ships with its method and its bias so the labels +can be argued with, which is worth more than the headline. diff --git a/docs/assets/demo.svg b/docs/assets/demo.svg index 7b8aa2c..2e0bfae 100644 --- a/docs/assets/demo.svg +++ b/docs/assets/demo.svg @@ -17,6 +17,6 @@ MEDIUMAG007 Tool input lacks validation tools.ts:17 Tool definition has no nearby input schema. Fix: attach a strict JSON Schema and reject unknown fields. - Scanned27 fileswith 9 rules in 84ms ·1 critical · 1 high· 1 medium + Scannedyour projectscanned ·findings by severity diff --git a/src/agentguard/cli.py b/src/agentguard/cli.py index 6b37fe0..629c334 100644 --- a/src/agentguard/cli.py +++ b/src/agentguard/cli.py @@ -50,6 +50,11 @@ def scan( fail_on: str = typer.Option( "high", help="Exit 1 when this severity or higher is found; use none to disable." ), + fail_on_incomplete: bool = typer.Option( + False, + "--fail-on-incomplete", + help="Also exit 2 when any line was too long to be read in full by every rule.", + ), config: Path | None = typer.Option(None, "--config", help="Path to .agentguard.yml."), exclude: list[str] = typer.Option([], "--exclude", help="Additional glob to exclude; repeat as needed."), ) -> None: @@ -83,6 +88,16 @@ def scan( threshold = Severity.parse(fail_on) except ValueError as exc: raise typer.BadParameter(str(exc), param_hint="--fail-on") from exc + if fail_on_incomplete and not result.fully_covered: + # Opt-in. Truncation is declared in every report by default rather than gating, + # because a bounded read is a known limitation, not a malfunction. A caller who + # needs total coverage asks for it and gets exit 2 - the same code as any other + # "this result is not a clean bill of health". + error_console.print( + f"[bold red]Coverage incomplete:[/bold red] {result.truncated_lines} line(s) " + "were not read in full; --fail-on-incomplete was requested." + ) + raise typer.Exit(code=2) if not result.completed: # Ranked above --fail-on: CI must be able to tell "found problems" (1) from # "the tool did not finish, so absence of findings means nothing" (2). diff --git a/src/agentguard/context.py b/src/agentguard/context.py index ee75f8f..1c04d53 100644 --- a/src/agentguard/context.py +++ b/src/agentguard/context.py @@ -21,12 +21,29 @@ ) _FIXTURE_NAME = re.compile(r"(?i)^(?:test_.*|.*_test|conftest|.*\.spec|.*\.test)$") +#: `.env.example` and friends exist to be committed and hold placeholders by convention. +#: A real `.env` does not, and is scanned at full severity. +_ENV_TEMPLATE = re.compile(r"(?i)^\.env\.(?:example|sample|template|dist|defaults?)$") + +#: Paths holding code this project did not write and does not ship as its own. Findings +#: here are almost always about somebody else's release, and are not actionable in the +#: repository being scanned. Downgraded on the same footing as fixtures. +_VENDORED_PATH = re.compile( + r"(?i)(?:^|/)(?:vendor|vendored|third[_-]?party|site-packages|dist-packages|" + r"node_modules|bower_components|\.venv|venv|eggs|\.eggs|bundled|external)(?:/|$)" +) + #: Longest line handed to a line-oriented rule. Past this, a "line" is a minified bundle #: or generated data, not something a per-line regex can say anything useful about, and #: it is the input that turns an ill-behaved pattern into a denial of service. Bounding #: the input bounds every regex rule at once, including ones not yet written. DEFAULT_MAX_LINE_LENGTH = 4096 +#: A rule may declare this as its own bound to opt out of truncation entirely. Only for +#: patterns *measured* linear — see tools/measure_linearity.py and docs/DECISIONS.md. +#: An unbounded non-linear pattern is a denial-of-service vector. +UNBOUNDED = 0 + @dataclass(slots=True) class SourceFile: @@ -39,9 +56,39 @@ class SourceFile: max_line_length: int = DEFAULT_MAX_LINE_LENGTH _tree: ast.AST | None = field(default=None, init=False, repr=False) _parsed: bool = field(default=False, init=False, repr=False) - _lines: list[str] | None = field(default=None, init=False, repr=False) + _raw_lines: list[str] | None = field(default=None, init=False, repr=False) + _bounded: dict[int, list[str]] = field(default_factory=dict, init=False, repr=False) _regions: Regions | None = field(default=None, init=False, repr=False) - truncated_lines: int = field(default=0, init=False) + #: The bound in force for the rule currently running. Seeded from + #: :attr:`max_line_length` and reset by the scanner before each rule, so a rule reads + #: `source.lines` without having to know its own declaration. + active_bound: int = field(default=DEFAULT_MAX_LINE_LENGTH, init=False) + + def __post_init__(self) -> None: + self.active_bound = self.max_line_length + + @property + def raw_lines(self) -> list[str]: + """Physical lines, untruncated. Computed once.""" + if self._raw_lines is None: + self._raw_lines = self.content.splitlines() + return self._raw_lines + + def lines_bounded(self, bound: int) -> list[str]: + """Lines capped at ``bound`` characters. ``UNBOUNDED`` returns them whole. + + Truncation preserves the line *count*, so reported line numbers stay correct. + """ + if bound not in self._bounded: + raw = self.raw_lines + self._bounded[bound] = raw if bound <= 0 else [line[:bound] for line in raw] + return self._bounded[bound] + + def over_bound(self, bound: int) -> list[tuple[int, int]]: + """``(line_number, length)`` for lines longer than ``bound``. Empty if unbounded.""" + if bound <= 0: + return [] + return [(n, len(line)) for n, line in enumerate(self.raw_lines, 1) if len(line) > bound] @property def is_fixture(self) -> bool: @@ -66,8 +113,24 @@ def is_fixture(self) -> bool: _FIXTURE_PATH.search(relative) or _FIXTURE_PATH.search(rooted) or _FIXTURE_NAME.match(self.path.stem) + or _ENV_TEMPLATE.match(self.path.name) ) + @property + def is_vendored(self) -> bool: + """True for dependency and vendored-code paths. + + A finding in `site-packages/` or `vendor/` is about someone else's release. It + may be real, but it is not actionable where it is reported, and it drowns the + findings that are. Downgraded on the same footing as fixtures. + """ + try: + relative = self.relative_path.as_posix() + except ValueError: # pragma: no cover - path outside root + relative = self.path.as_posix() + rooted = f"{self.root.name}/{relative}" if self.root.name else relative + return bool(_VENDORED_PATH.search(relative) or _VENDORED_PATH.search(rooted)) + def regions(self) -> Regions: """Comment, string, docstring, and annotation spans, computed once per file.""" if self._regions is None: @@ -81,20 +144,14 @@ def regions(self) -> Regions: @property def lines(self) -> list[str]: - """Physical lines, each bounded by :attr:`max_line_length`. - - Truncation preserves the line *count*, so reported line numbers stay correct. - :attr:`truncated_lines` records how much was withheld, because a bounded scan - that reports nothing must not be mistaken for a clean one. + """Lines as the currently running rule should see them. - Computed once. Rules call this in a loop and there are nine of them. + The bound comes from that rule's declaration, applied by the scanner, so a rule + never asks for it. A rule whose patterns are measured linear declares + :data:`UNBOUNDED` and sees minified bundles whole — which is where inlined + credentials live. """ - if self._lines is None: - raw = self.content.splitlines() - limit = self.max_line_length - self.truncated_lines = sum(1 for line in raw if len(line) > limit) - self._lines = [line[:limit] for line in raw] if self.truncated_lines else raw - return self._lines + return self.lines_bounded(self.active_bound) @property def relative_path(self) -> Path: diff --git a/src/agentguard/models.py b/src/agentguard/models.py index 716af1d..dba1ebc 100644 --- a/src/agentguard/models.py +++ b/src/agentguard/models.py @@ -63,6 +63,29 @@ def to_dict(self, root: Path | None = None) -> dict[str, Any]: return data +@dataclass(frozen=True, slots=True) +class TruncatedLine: + """A line some rule was not shown in full, and by how much.""" + + path: Path + line: int + length: int + bound: int + + def to_dict(self, root: Path | None = None) -> dict[str, Any]: + path = self.path + if root: + with suppress(ValueError): + path = path.relative_to(root) + return { + "path": path.as_posix(), + "line": self.line, + "length": self.length, + "bound": self.bound, + "withheld": self.length - self.bound, + } + + @dataclass(slots=True) class ScanResult: """Aggregate result of one scan.""" @@ -72,10 +95,25 @@ class ScanResult: files_scanned: int rules_run: int skipped_files: int = 0 - truncated_lines: int = 0 + truncated: list[TruncatedLine] = field(default_factory=list) errors: list[str] = field(default_factory=list) duration_ms: float = 0.0 + @property + def truncated_lines(self) -> int: + """Count of lines withheld in full from at least one rule.""" + return len(self.truncated) + + @property + def fully_covered(self) -> bool: + """True when every rule saw every in-scope line in full. + + Distinct from :attr:`completed`. A scan can finish cleanly and still not have + looked at everything, which is why truncation is reported rather than silently + folded into success. + """ + return not self.truncated + @property def completed(self) -> bool: """True when every enabled rule ran to completion over every in-scope file. diff --git a/src/agentguard/reporters.py b/src/agentguard/reporters.py index 15d3c8c..bd6180b 100644 --- a/src/agentguard/reporters.py +++ b/src/agentguard/reporters.py @@ -23,10 +23,16 @@ def to_json(result: ScanResult) -> str: "rules_run": result.rules_run, "skipped_files": result.skipped_files, "truncated_lines": result.truncated_lines, + "fully_covered": result.fully_covered, "duration_ms": round(result.duration_ms, 2), "counts": result.counts(), "errors": result.errors, }, + "coverage": { + "fully_covered": result.fully_covered, + "skipped_files": result.skipped_files, + "truncated_lines": [item.to_dict(result.root) for item in result.truncated], + }, "findings": [finding.to_dict(result.root) for finding in result.findings], } return json.dumps(payload, indent=2) @@ -63,16 +69,28 @@ def to_markdown(result: ScanResult) -> str: "", ] ) - if result.skipped_files or result.truncated_lines: + if not result.fully_covered or result.skipped_files: rows.extend( [ - "## Coverage bounds", + "## Coverage", "", f"- Files skipped by size limit: {result.skipped_files}", - f"- Lines truncated by length limit: {result.truncated_lines}", + f"- Lines not read in full: {result.truncated_lines}", + "", + "A finding cannot be reported from text no rule was shown. These lines were", + "clipped for at least one rule; rules declaring no bound saw them whole.", "", ] ) + for item in result.truncated[:50]: + data = item.to_dict(result.root) + rows.append( + f"- `{data['path']}:{data['line']}` — {data['length']} chars, " + f"{data['withheld']} withheld beyond the {data['bound']}-char bound" + ) + if len(result.truncated) > 50: + rows.append(f"- …and {len(result.truncated) - 50} more") + rows.append("") if result.errors: rows.extend(["## Scan warnings", "", *[f"- {error}" for error in result.errors], ""]) rows.append("_Generated by AgentGuard._") @@ -144,7 +162,20 @@ def to_sarif(result: ScanResult) -> str: { "executionSuccessful": result.completed, "toolExecutionNotifications": [ - {"message": {"text": error}, "level": "warning"} for error in result.errors + {"message": {"text": error}, "level": "error"} for error in result.errors + ] + + [ + { + "message": { + "text": ( + f"Coverage: {item.to_dict(result.root)['path']}:{item.line} " + f"was {item.length} chars; {item.length - item.bound} beyond " + f"the {item.bound}-char bound were not read by every rule." + ) + }, + "level": "note", + } + for item in result.truncated ], } ], @@ -188,11 +219,17 @@ def render_terminal(result: ScanResult, console: Console | None = None) -> None: f"[red]{counts['Critical']} critical[/red], [red]{counts['High']} high[/red], " f"[yellow]{counts['Medium']} medium[/yellow], [blue]{counts['Low']} low[/blue]" ) - if result.skipped_files or result.truncated_lines: + if not result.fully_covered or result.skipped_files: output.print( - f"[dim]Bounded: {result.skipped_files} file(s) skipped by size, " - f"{result.truncated_lines} line(s) truncated by length.[/dim]" + f"[yellow]Coverage:[/yellow] {result.skipped_files} file(s) skipped by size, " + f"{result.truncated_lines} line(s) not read in full. " + f"[dim]Use --fail-on-incomplete to gate on this.[/dim]" ) + for item in result.truncated[:5]: + data = item.to_dict(result.root) + output.print(f" [dim]{data['path']}:{data['line']} — {data['withheld']} chars withheld[/dim]") + if len(result.truncated) > 5: + output.print(f" [dim]…and {len(result.truncated) - 5} more[/dim]") for error in result.errors: output.print(f"[yellow]Warning:[/yellow] {error}") diff --git a/src/agentguard/rules/base.py b/src/agentguard/rules/base.py index 79a1269..522fe65 100644 --- a/src/agentguard/rules/base.py +++ b/src/agentguard/rules/base.py @@ -43,8 +43,13 @@ class RuleMetadata: #: If set, the matched line must contain one of these node kinds. `{"call"}` is what #: separates `transfer_funds(x)` from `def transfer_funds(x)`. require_nodes: frozenset[str] = frozenset() - #: Behaviour in test, fixture, and example files. + #: Behaviour in test, fixture, example, and vendored files. fixture_policy: str = "suppress" + #: Longest line this rule is handed. ``None`` inherits the scanner's configured + #: bound. ``UNBOUNDED`` (0) opts out of truncation entirely and is only permissible + #: for patterns *measured* linear — an unbounded non-linear pattern is a + #: denial-of-service vector. Record the measurement when setting it. + max_line_length: int | None = None def __post_init__(self) -> None: if not self.id or not self.title: @@ -68,6 +73,8 @@ def __post_init__(self) -> None: raise ValueError( f"{self.id}: fixture_policy must be one of {', '.join(sorted(FIXTURE_POLICIES))}" ) + if self.max_line_length is not None and self.max_line_length < 0: + raise ValueError(f"{self.id}: max_line_length must be >= 0 (0 means unbounded)") def applies_to(self, source: SourceFile) -> bool: return source.language in self.languages diff --git a/src/agentguard/rules/code.py b/src/agentguard/rules/code.py index 0c0af3a..5d0df67 100644 --- a/src/agentguard/rules/code.py +++ b/src/agentguard/rules/code.py @@ -43,6 +43,37 @@ def _is_literal(node: ast.expr) -> bool: return True +def _module_constants(tree: ast.AST) -> frozenset[str]: + """Names bound exactly once, at module scope, to a literal. + + `exec(_NAMESPACE_IMPORTS, ns)` reaches no attacker-controlled value, but the argument + is a Name rather than a literal so literal detection alone misses it. Measured in + browser-use (mcp/cli_mcp.py:105). + + "Exactly once" is the load-bearing part. A name assigned a literal at module scope and + reassigned anywhere else - in a function, a branch, a loop - is not a constant, and + treating it as one would be a blind spot rather than a precision gain. + """ + stores: dict[str, int] = {} + for node in ast.walk(tree): + if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store): + stores[node.id] = stores.get(node.id, 0) + 1 + + literal: set[str] = set() + for statement in getattr(tree, "body", []): + if isinstance(statement, ast.Assign) and _is_literal(statement.value): + literal.update(t.id for t in statement.targets if isinstance(t, ast.Name)) + elif ( + isinstance(statement, ast.AnnAssign) + and statement.value is not None + and isinstance(statement.target, ast.Name) + and _is_literal(statement.value) + ): + literal.add(statement.target.id) + + return frozenset(name for name in literal if stores.get(name, 0) == 1) + + class DangerousExecutionRule(Rule): metadata = RuleMetadata( "AG002", @@ -56,6 +87,8 @@ class DangerousExecutionRule(Rule): def scan(self, source: SourceFile) -> Iterable[Finding]: if source.language == "python": + tree = source.python_tree() + constants = _module_constants(tree) if tree is not None else frozenset() dangerous = {"eval", "exec", "os.system", "subprocess.run", "subprocess.call", "subprocess.Popen"} for call in _calls(source): name = _name(call.func) @@ -69,10 +102,43 @@ def scan(self, source: SourceFile) -> Iterable[Finding]: ) # eval("{'retries': 3}") has no attacker-controlled input. Reporting it # teaches readers that the rule cannot tell reachable from unreachable, - # which is how a scanner earns being switched off. - if call.args and all(_is_literal(argument) for argument in call.args) and not shell: + # which is how a scanner earns being switched off. A module-level constant + # counts as fixed for the same reason. + # + # Only the *first* argument is executed. `exec(code, namespace)` passes a + # globals dict second, and that it is a variable says nothing about whether + # the code is attacker-controlled. Requiring every argument to be fixed + # meant this never fired on the two-argument form, which is the common one. + executed = call.args[0] if call.args else None + if ( + executed is not None + and not shell + and ( + _is_literal(executed) or (isinstance(executed, ast.Name) and executed.id in constants) + ) + ): continue - if name in {"eval", "exec", "os.system"} or shell: + if shell and call.args and isinstance(call.args[0], ast.List | ast.Tuple): + # shell=True with an argument *vector* is a portability bug, not an + # injection: POSIX passes only argv[0] to the shell and silently drops + # the rest, while Windows joins them. Describing it as an injection + # risk sends readers looking for untrusted input that is not there - + # measured as a false positive in modelcontextprotocol/python-sdk. + yield self.finding( + source, + call.lineno, + f"`{name}` passes an argument list with `shell=True`.", + "The two are mutually exclusive: on POSIX the shell receives only the first " + "element and every later argument is silently discarded, so the command runs " + "differently - or not at all - depending on the platform.", + "Drop `shell=True` and keep the argument list, which is also the safer form. " + "Only if a shell is genuinely required, pass a single string instead, and then " + "treat every interpolated value as untrusted.", + column=call.col_offset + 1, + confidence="high", + metadata={"defect_class": "portability"}, + ) + elif name in {"eval", "exec", "os.system"} or shell: yield self.finding( source, call.lineno, @@ -105,7 +171,12 @@ class BroadToolPermissionRule(Rule): languages=frozenset({"python", "javascript", "typescript"}), ) _pattern = re.compile( - r"(?i)(allow_dangerous_code\s*=\s*True|allow_delegation\s*=\s*True|function_map\s*=|tools\s*=\s*\[[^\]]*(?:shell|terminal|filesystem|browser)|allowedTools\s*:\s*\[[^\]]*['\"]\*['\"]|dangerouslyAllowBrowser\s*:\s*true)" + # `function_map\s*=` was removed: it matched any local variable of that name - + # measured twice in openai-agents-python on ordinary dict comprehensions - and + # even where it hit AutoGen's real parameter it flagged the *existence* of a + # function map, not an over-broad one. A clause that cannot express the breadth + # the rule is named for does not belong in it. See docs/DECISIONS.md. + r"(?i)(allow_dangerous_code\s*=\s*True|allow_delegation\s*=\s*True|tools\s*=\s*\[[^\]]*(?:shell|terminal|filesystem|browser)|allowedTools\s*:\s*\[[^\]]*['\"]\*['\"]|dangerouslyAllowBrowser\s*:\s*true)" ) def scan(self, source: SourceFile) -> Iterable[Finding]: @@ -194,7 +265,11 @@ class UnsafeFileAccessRule(Rule): ) # \b matters: without it, `path` matched inside `streamable_http_path`, reporting an # HTTP mount point as a broad filesystem root eight times in one real project. - _broad = re.compile(r"(?i)\b(?:root_dir|workspace|directory|path)\s*[:=]\s*['\"](?:/|~|\.\.)['\"]") + # The trailing lookahead matters: `path = "/" + path` prefixes a separator, it does + # not grant a filesystem root. Measured in crewAI (memory/utils.py:61). + _broad = re.compile( + r"(?i)\b(?:root_dir|workspace|directory|path)\s*[:=]\s*['\"](?:/|~|\.\.)['\"](?!\s*[+,]?\s*\w)" + ) def scan(self, source: SourceFile) -> Iterable[Finding]: for number, line in enumerate(source.lines, 1): @@ -223,8 +298,14 @@ class RiskyExternalAPIRule(Rule): _http = re.compile( r"(?i)(?:requests\.(?:get|post|put|delete)|fetch|axios\.(?:get|post)|httpx\.(?:get|post))\s*\(\s*['\"]http://" ) + # `url`, `uri`, and `endpoint` were removed from this list: they are the ordinary + # names for a local variable holding a URL, and say nothing about where the value + # came from. Every AG006 finding in the field measurement was one of these, on calls + # to compile-time hosts over TLS with timeouts. What remains names an actually + # untrusted source. Recall trade recorded in WORKLOG.md. _dynamic = re.compile( - r"(?i)(?:requests\.(?:get|post)|fetch|axios\.(?:get|post))\s*\(\s*(?:url|uri|endpoint|tool_input|user_input)" + r"(?i)(?:requests\.(?:get|post)|fetch|axios\.(?:get|post))\s*\(\s*" + r"(?:tool_input|user_input|user_url|request\.|req\.|model_output|llm_output)" ) def scan(self, source: SourceFile) -> Iterable[Finding]: diff --git a/src/agentguard/rules/secrets.py b/src/agentguard/rules/secrets.py index c2d6719..5618822 100644 --- a/src/agentguard/rules/secrets.py +++ b/src/agentguard/rules/secrets.py @@ -6,10 +6,18 @@ import re from collections.abc import Iterable -from agentguard.context import SourceFile +from agentguard.context import UNBOUNDED, SourceFile from agentguard.models import Finding, Severity from agentguard.rules.base import Rule, RuleMetadata +#: Credentials that are published deliberately. A PostHog project key ships in client +#: JavaScript; a Stripe publishable key is printed in documentation; a Supabase anon key +#: is meant to reach the browser. Each is literally a committed credential, and reporting +#: one as a critical compromise is wrong — nothing is impersonated and nothing is +#: accessed. Measured as false positives in two of five real projects. +_PUBLIC_VALUE = re.compile(r"^(?:phc_[A-Za-z0-9]{20,}|pk_(?:live|test)_[A-Za-z0-9]{10,})$") +_PUBLIC_NAME = re.compile(r"(?i)(?:^|[^a-z])(?:public|publishable|anon|client[_-]?id)(?:[^a-z]|$)") + class HardcodedSecretRule(Rule): metadata = RuleMetadata( @@ -26,6 +34,13 @@ class HardcodedSecretRule(Rule): # Live credentials really do get committed to fixtures, so these are reported at # reduced severity rather than dropped. fixture_policy="downgrade", + # Runs over untruncated lines. A minified bundle with an inlined key is a real + # and common leak, and it is exactly the shape that exceeds the default bound: a + # key at offset 5,000 of a one-line bundle was previously missed entirely. + # Permissible only because every pattern below is *measured* linear — + # `python -m tools.measure_linearity` reports exponents of 0.98-1.01 and a + # worst case of 42 ms on a 1 MB line. Re-run it before adding a pattern here. + max_line_length=UNBOUNDED, ) _patterns = ( ("OpenAI API key", re.compile(r"\bsk-(?:proj-)?[A-Za-z0-9_-]{20,}\b")), @@ -37,33 +52,92 @@ class HardcodedSecretRule(Rule): re.compile(r"(?i)(?:api[_-]?key|secret|token|password)\s*[:=]\s*['\"]([^'\"]{12,})['\"]"), ), ) - _placeholder = re.compile(r"(?i)(example|dummy|test|changeme|your[_-]|xxx|<|\$\{|process\.env)") + #: Value shapes that are placeholders rather than credentials. `^\$` and `\$\{` both + #: matter: `${VAR}` and a bare `$VAR` are references to a value held elsewhere, and a + #: bare one was reported as a critical secret until a corpus case caught it. The + #: `replace|insert|redacted` group covers template conventions that carry no marker + #: word of their own - `sk-proj-replace-this-before-running` matched nothing before. + _placeholder = re.compile( + r"(?i)(example|dummy|test|changeme|your[_-]|xxx|<|\$\{|^\$|process\.env" + r"|replace|placeholder|redacted|insert[_-]?your|todo|fixme)" + ) + + #: `.env` files conventionally leave values unquoted - `DB_PASSWORD=hunter2`, not + #: `DB_PASSWORD="hunter2"` - so the quoted assigned-credential pattern misses the + #: normal case entirely. Applied only to env files: requiring quotes is what keeps + #: that pattern from matching `password = get_password()` in ordinary source. + #: + #: The name is captured whole and tested in Python rather than matched with + #: `[A-Z0-9_]*KEYWORD[A-Z0-9_]*`. That form has two unbounded quantifiers around an + #: alternation and measured quadratic - 918ms on 32KB, extrapolating to ~16 minutes + #: on a 1MB line. AG001 runs unbounded, so that was a denial-of-service vector; the + #: `--check` gate caught it before it shipped. + _env_assignment = re.compile(r"^\s*(?:export\s+)?([A-Za-z0-9_]+)\s*=\s*(?!['\"])(\S{12,})\s*$") + _env_credential_name = re.compile(r"(?i)(?:api[_-]?key|secret|token|password|passwd|pwd)") + _env_file = re.compile(r"^\.env(?:\..+)?$") def scan(self, source: SourceFile) -> Iterable[Finding]: + env = bool(self._env_file.match(source.path.name)) + patterns = ( + (*self._patterns, ("environment credential", self._env_assignment)) if env else self._patterns + ) for number, line in enumerate(source.lines, 1): - for kind, pattern in self._patterns: + for kind, pattern in patterns: match = pattern.search(line) if not match: continue - value = match.group(1) if match.lastindex else match.group(0) - # Point at the credential, not at the identifier before it. The engine - # tests regions at this column, and `token: "contextvars.Token[Any]"` is - # only distinguishable from a real credential by where the value sits. - column = (match.start(1) if match.lastindex else match.start()) + 1 + if kind == "environment credential": + if not self._env_credential_name.search(match.group(1)): + continue + value, column = match.group(2), match.start(2) + 1 + else: + value = match.group(1) if match.lastindex else match.group(0) + column = (match.start(1) if match.lastindex else match.start()) + 1 + # `column` points at the credential, not the identifier before it. The + # engine tests regions at this column, and `token: "contextvars.Token[Any]"` + # is only distinguishable from a real credential by where the value sits. if self._placeholder.search(value): continue - if kind == "assigned credential" and self._entropy(value) < 3.0: + if kind in {"assigned credential", "environment credential"} and self._entropy(value) < 3.0: continue - yield self.finding( - source, - number, - f"A likely {kind} is embedded directly in source code.", - "Anyone with repository or build-artifact access may impersonate the service or access protected data.", - "Revoke and rotate the credential, remove it from history, and load the replacement from a secret manager or environment variable.", - column=column, - ) + + public = self._is_public(line[: match.start()], value) + if public: + yield self.finding( + source, + number, + f"A likely {kind} is committed, but it is publishable by design.", + "Publishable keys are meant to be distributed, so exposure alone is not a " + "compromise. Confirm it is the publishable half and not the secret one, and " + "that no scope was granted to it beyond what public clients should have.", + "No rotation is required if this is genuinely the publishable key. If it is " + "not, treat it as a leaked secret and rotate.", + column=column, + confidence="medium", + metadata={"credential_class": "public", "kind": kind}, + ) + else: + yield self.finding( + source, + number, + f"A likely {kind} is embedded directly in source code.", + "Anyone with repository or build-artifact access may impersonate the service or access protected data.", + "Revoke and rotate the credential, remove it from history, and load the replacement from a secret manager or environment variable.", + column=column, + metadata={"credential_class": "secret", "kind": kind}, + ) break + @staticmethod + def _is_public(prefix: str, value: str) -> bool: + """Whether this is a key intended for publication. + + Judged from the value's own vendor prefix, or from the identifier it is assigned + to. `SUPABASE_PUBLIC_API_KEY` says so in its name; a `phc_` value says so in its + shape. Both were reported as critical compromises before this existed. + """ + return bool(_PUBLIC_VALUE.match(value) or _PUBLIC_NAME.search(prefix)) + @staticmethod def _entropy(value: str) -> float: if not value: diff --git a/src/agentguard/scanner.py b/src/agentguard/scanner.py index 0148b24..8d2868f 100644 --- a/src/agentguard/scanner.py +++ b/src/agentguard/scanner.py @@ -3,6 +3,7 @@ from __future__ import annotations import fnmatch +import re import time from collections.abc import Iterable, Sequence from dataclasses import replace @@ -10,7 +11,7 @@ from agentguard.config import Config from agentguard.context import SourceFile -from agentguard.models import Finding, ScanResult, Severity +from agentguard.models import Finding, ScanResult, Severity, TruncatedLine from agentguard.plugins import load_plugins from agentguard.rules import BUILTIN_RULES, Rule @@ -30,6 +31,11 @@ } SPECIAL_FILES = {"Dockerfile", "Pipfile", "package-lock.json", "requirements.txt"} +#: `.env` is where credentials live. A secrets scanner that cannot read the canonical +#: secrets file is hard to defend, and these files carry no extension, so the suffix map +#: never reached them. Covers `.env`, `.env.local`, `.env.production`, and templates. +_ENV_FILE = re.compile(r"^\.env(?:\..+)?$") + class Scanner: """Scan a directory with built-in and custom rules.""" @@ -54,7 +60,7 @@ def scan(self, target: Path | str) -> ScanResult: errors: list[str] = [] scanned = 0 skipped = 0 - truncated = 0 + truncated: list[TruncatedLine] = [] for path in self._files(target_path, root): try: if path.stat().st_size > self.config.max_file_size_kb * 1024: @@ -66,16 +72,25 @@ def scan(self, target: Path | str) -> ScanResult: skipped += 1 continue scanned += 1 + language = ( + "manifest" if _ENV_FILE.match(path.name) else LANGUAGES.get(path.suffix.lower(), "manifest") + ) source = SourceFile( path, root, content, - LANGUAGES.get(path.suffix.lower(), "manifest"), + language, max_line_length=self.config.max_line_length, ) + # Which bound each rule ran under, so coverage reflects what was actually + # withheld rather than what the default would have withheld. + bounds_applied: set[int] = set() for rule in self.rules: if not rule.metadata.applies_to(source): continue + declared = rule.metadata.max_line_length + source.active_bound = self.config.max_line_length if declared is None else declared + bounds_applied.add(source.active_bound) try: for finding in rule.scan(source): if self._suppressed(source, finding): @@ -89,7 +104,13 @@ def scan(self, target: Path | str) -> ScanResult: ) except Exception as exc: errors.append(f"{rule.metadata.id} failed on {source.relative_path}: {exc}") - truncated += source.truncated_lines + # Report against the *tightest* bound that actually applied. A line is a + # coverage gap if any rule was shown less than all of it, so the smallest + # positive bound is what decides — using the largest would miss every line + # falling between two different rules' bounds. + binding = min((b for b in bounds_applied if b > 0), default=0) + for number, length in source.over_bound(binding): + truncated.append(TruncatedLine(path, number, length, binding)) findings.sort( key=lambda item: ( -int(item.severity), @@ -104,7 +125,7 @@ def scan(self, target: Path | str) -> ScanResult: files_scanned=scanned, rules_run=len(self.rules), skipped_files=skipped, - truncated_lines=truncated, + truncated=truncated, errors=errors, duration_ms=(time.perf_counter() - started) * 1000, ) @@ -121,6 +142,7 @@ def _files(self, target: Path, root: Path) -> Iterable[Path]: path.suffix.lower() in LANGUAGES or path.name in SPECIAL_FILES or path.name.startswith("requirements") + or _ENV_FILE.match(path.name) ): yield path @@ -156,7 +178,13 @@ def _admit(rule: Rule, source: SourceFile, finding: Finding) -> Finding | None: ): return None - if source.is_fixture: + # A key published on purpose is not a compromise. Capped centrally rather than + # left to each rule, and capped rather than dropped: it is still worth knowing + # that a credential-shaped value is committed, in case it is the secret half. + if finding.metadata.get("credential_class") == "public" and finding.severity > Severity.LOW: + finding = replace(finding, severity=Severity.LOW) + + if source.is_fixture or source.is_vendored: if meta.fixture_policy == "suppress": return None if meta.fixture_policy == "downgrade": diff --git a/tests/corpus/manifest.yml b/tests/corpus/manifest.yml index f5976b7..3e7c603 100644 --- a/tests/corpus/manifest.yml +++ b/tests/corpus/manifest.yml @@ -12,50 +12,59 @@ true_positives: hardcoded_secrets.py: + origin: written expect: [AG001] why: >- Four credential shapes assigned in a production module: OpenAI, AWS, GitHub, and a high-entropy password. This is the case the rule exists for. command_execution.py: + origin: written expect: [AG002] why: >- os.system and eval on caller-supplied values, and subprocess with shell=True. Each is a path from model output to arbitrary command execution. broad_tool_permissions.py: + origin: written expect: [AG003] why: >- allow_dangerous_code=True and allow_delegation=True are explicit grants of capability that widen blast radius. prompt_injection.py: + origin: written expect: [AG004] why: >- user_input and web_content interpolated directly into prompt and system_message f-strings, with no separation between data and instructions. unsafe_file_access.py: + origin: written expect: [AG005] why: open() on a caller-controlled path, reachable from tool arguments. risky_external_api.py: + origin: written expect: [AG006] why: A plaintext http:// endpoint, so request and response are interceptable. missing_validation.py: + origin: written expect: [AG007] why: >- A @tool function whose parameter carries no type or schema, so the model may supply any shape it likes. missing_approval.py: + origin: written expect: [AG008] why: >- transfer_funds is called with no approval or confirmation gate anywhere nearby; an injected instruction reaches an irreversible action. requirements.txt: + origin: written expect: [AG010] why: >- Dependencies with no version constraint admit an unreviewed future release. Package @@ -64,10 +73,21 @@ true_positives: review on every future advisory. agent_tools.ts: + origin: written expect: [AG002] why: execSync on a caller-supplied command in TypeScript. + .env: + origin: written + expect: [AG001] + why: >- + The canonical secrets file. Values are unquoted, which is the convention and which + the quoted assigned-credential pattern does not match - so this needs its own + pattern, applied only to env files. `DEBUG=true` and `PORT=8080` must not fire, + which is what stops that pattern being a blanket KEY=VALUE matcher. + root_dir_broad.py: + origin: written expect: [AG005] why: >- An agent workspace rooted at "/" grants the whole filesystem. Kept as a true @@ -75,6 +95,7 @@ true_positives: assumed harmless. test_secrets_fixture.py: + origin: written expect: [AG001] why: >- Credentials do get committed to test fixtures, so this must still be reported - @@ -85,39 +106,64 @@ true_positives: entry claim to pin the downgrade while never reaching it. true_negatives: + .env.local: + origin: written + expect: [] + why: >- + A real .env file, not a template, so the fixture downgrade does not apply and every + line must fail to fire on its own merits: commented-out credentials, empty values, + braced and unbraced shell interpolation, values under the length floor, and ordinary + configuration. The unbraced `$OTHER_TOKEN` form was reported as a critical secret + until this case existed - the placeholder filter required braces. + + .env.sample: + origin: written + expect: [] + why: >- + A second template naming convention. `sk-proj-replace-this-before-running` carried no + word the placeholder filter recognised and was reported until this case existed. Note + the mechanism is the *value*, not the filename: a genuine credential committed to a + .env template is a real leak and is still reported, at reduced severity. + .env.example: + origin: written expect: [] why: >- - A .env template exists to be committed; its values are placeholders. Includes - AKIAIOSFODNN7EXAMPLE, AWS's own documentation key. NOTE: AgentGuard does not - currently discover .env files at all, so this entry is recorded by the harness as - not-discovered rather than as a pass. It also means a real .env would be missed. + A .env template exists to be committed and holds placeholders by convention, so it + is classified as a fixture and its values are caught by the placeholder filter. + Includes AKIAIOSFODNN7EXAMPLE, AWS's own documentation key. Previously this file was + not discovered at all and scored a free pass while proving nothing. secrets_in_docstrings.py: + origin: field expect: [] why: >- Doctest examples showing argument shape. Observed in crewAI, where a docstring example was reported as a committed private key. eval_on_literal.py: + origin: written expect: [] why: >- eval over a string literal has no attacker-controlled input. Flagging it teaches readers that the rule does not distinguish reachable from unreachable. subprocess_fixed_args.py: + origin: written expect: [] why: >- A fixed argument vector with shell=False cannot become an arbitrary command. This is the remediation the AG002 finding text recommends, so flagging it punishes the fix. constant_prompt.py: + origin: written expect: [] why: >- The interpolated names are module constants, not untrusted data. No trust boundary is crossed. method_named_exec.py: + origin: field expect: [] why: >- super().exec() and self.client().eval() are ordinary method calls. AG002's name @@ -125,37 +171,105 @@ true_negatives: openai-agents-python. token_annotations.py: + origin: field expect: [] why: >- contextvars.Token[Any] is a type annotation. The AG001 assigned-credential pattern matched `token: "..."` as a credential. Measured twice in openai-agents-python. route_paths.py: + origin: field expect: [] why: >- streamable_http_path="/" is an HTTP mount point, not a filesystem root. Measured eight times in modelcontextprotocol/python-sdk. dependabot.yml: + origin: field expect: [] why: >- `directory: "/"` locates a dependency manifest. A filesystem-access rule has no business reading CI configuration at all. Measured in three of five projects. function_definitions.py: + origin: written expect: [] why: >- Defining delete_file, deploy, or send_email is not performing them. AG008 matched `def` sites, which was most of its findings across four of five projects. + js_fetch_local_url.js: + origin: field + expect: [] + why: >- + `fetch(url)` where `url` holds a compile-time host. Converted from an assertion in + tests/test_rules.py that AG006 *should* fire here - that expectation was wrong, and a + wrong expectation in a test is worse than none because it defends the defect. Every + AG006 field finding was this shape. + analytics_iife.js: + origin: field expect: [] why: >- An IIFE is not a tool definition. AG007's JavaScript pattern matches any `function(`. Measured in crewAI's documentation assets. requirements.txt: + origin: written expect: [] why: >- Fully pinned dependencies are the state AG010 asks for. Fictional package names, for the same reason as the true_positives counterpart. + + # --- folded back from field measurement ------------------------------------------- + # Every entry below reproduces a false positive found by scanning real agent projects, + # not one invented while reading the rules. The corpus previously scored AG003 and + # AG006 at 100% precision while they were ~98-100% false positives in the field; that + # gap was a corpus validity failure. Adding these makes the number fall, and the lower + # number is the honest one. + + framework_lookup_maps.py: + origin: field + expect: [] + why: >- + A local variable named `function_map` is not AutoGen's function_map parameter. + Measured twice in openai-agents-python. AG003 currently fires; this is expected to + show as a false positive until the pattern is narrowed. + + fixed_host_requests.py: + origin: field + expect: [] + why: >- + Compile-time host, TLS, explicit timeout. AG006 matches any first argument named + `url` regardless of provenance. Measured twice in crewAI. + + exec_of_module_constant.py: + origin: field + expect: [] + why: >- + exec over a module-level constant has no attacker-controlled input. Distinct from + eval_on_literal.py, where the argument is a literal and is already handled: here it + is a Name, so literal detection does not catch it. Measured in browser-use. + + path_normaliser.py: + origin: field + expect: [] + why: >- + `path = "/" + path` prefixes a slash; it does not grant a filesystem root. Measured + in crewAI. + + internal_cleanup.py: + origin: field + expect: [] + why: >- + Deleting a temp file the code itself created is not a consequential side effect on + user data, and the path is not agent-controlled. Measured in crewAI. + + publishable_keys.py: + origin: field + expect: [AG001] + why: >- + Publishable keys must still be reported - the publishable and secret halves are easy + to confuse - but capped at Low, so this is a true positive whose *severity* is the + assertion. Pinned by tests/test_rule_context.py::test_publishable_key_is_capped_at_low. + Measured as Critical false positives in browser-use and langgraph. diff --git a/tests/corpus/true_negatives/.env.example b/tests/corpus/true_negatives/.env.example index c752183..d3c8927 100644 --- a/tests/corpus/true_negatives/.env.example +++ b/tests/corpus/true_negatives/.env.example @@ -1,4 +1,4 @@ -# Template checked into source control. Values are placeholders, by definition. -OPENAI_API_KEY=sk-proj-replace-me-before-running -DATABASE_PASSWORD=change-this-value +# A committed template. Placeholders by convention, which is why it is committed at all. +DB_PASSWORD=changeme +OPENAI_API_KEY=your_key_here AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE diff --git a/tests/corpus/true_negatives/.env.local b/tests/corpus/true_negatives/.env.local new file mode 100644 index 0000000..9e848e9 --- /dev/null +++ b/tests/corpus/true_negatives/.env.local @@ -0,0 +1,28 @@ +# A real .env file - NOT a template, so the fixture downgrade does not apply. Everything +# here must still fail to fire, each for a different reason. + +# 1. Commented-out credentials. The anchored env pattern cannot match a line starting +# with '#', and the value patterns are suppressed by the comment region. +# OPENAI_API_KEY=sk-proj-Qw8dLm2VtBnKcXrJf7HsPu4Ay6Ez1Nio +# DB_PASSWORD=b7Kq2ZmVx9Lp4Rt6Wn3Jc + +# 2. Empty values. Nothing has leaked. +API_KEY= +DB_PASSWORD= +SECRET_TOKEN= + +# 3. Shell interpolation - the value lives elsewhere, this is a reference. +API_KEY=${UPSTREAM_API_KEY} +DB_PASSWORD=${VAULT_DB_PASSWORD} +SERVICE_TOKEN=$OTHER_TOKEN + +# 4. Values too short to be a credential. +API_KEY=abc +PASSWORD=x + +# 5. Ordinary configuration that is not a credential at all. +DEBUG=true +PORT=8080 +LOG_LEVEL=info +BASE_URL=https://api.example.invalid/v1/some/long/path/that/is/not/a/secret +TIMEOUT_SECONDS=30 diff --git a/tests/corpus/true_negatives/.env.sample b/tests/corpus/true_negatives/.env.sample new file mode 100644 index 0000000..cfa6b65 --- /dev/null +++ b/tests/corpus/true_negatives/.env.sample @@ -0,0 +1,5 @@ +# A second template naming convention. Committed on purpose; values are placeholders. +OPENAI_API_KEY=sk-proj-replace-this-before-running +DB_PASSWORD=changeme +GITHUB_TOKEN=your_token_here +AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE diff --git a/tests/corpus/true_negatives/exec_of_module_constant.py b/tests/corpus/true_negatives/exec_of_module_constant.py new file mode 100644 index 0000000..1df7826 --- /dev/null +++ b/tests/corpus/true_negatives/exec_of_module_constant.py @@ -0,0 +1,14 @@ +"""exec over a module-level constant reaches no attacker-controlled value. + +Measured false positive in browser-use (mcp/cli_mcp.py:105). Distinct from +eval_on_literal.py: the argument is a Name, not a literal, so literal-argument detection +does not catch it. +""" + +_NAMESPACE_IMPORTS = "import json\nimport os\n" + + +def build_namespace() -> dict[str, object]: + namespace: dict[str, object] = {} + exec(_NAMESPACE_IMPORTS, namespace) + return namespace diff --git a/tests/corpus/true_negatives/fixed_host_requests.py b/tests/corpus/true_negatives/fixed_host_requests.py new file mode 100644 index 0000000..e1112b7 --- /dev/null +++ b/tests/corpus/true_negatives/fixed_host_requests.py @@ -0,0 +1,14 @@ +"""An outbound call to a compile-time host, over TLS, with a timeout. + +Measured false positive in crewAI (contextual_query_tool.py:49, +merge_agent_handler_tool.py:97). AG006's dynamic-URL pattern matches any first argument +named `url`, regardless of how the value was built. +""" + +import requests + + +def list_documents(datastore_id: str, api_key: str) -> object: + url = f"https://api.contextual.ai/v1/datastores/{datastore_id}/documents" + headers = {"Authorization": f"Bearer {api_key}"} + return requests.get(url, headers=headers, timeout=30) diff --git a/tests/corpus/true_negatives/framework_lookup_maps.py b/tests/corpus/true_negatives/framework_lookup_maps.py new file mode 100644 index 0000000..51f660e --- /dev/null +++ b/tests/corpus/true_negatives/framework_lookup_maps.py @@ -0,0 +1,13 @@ +"""A local variable named `function_map` is not AutoGen's function_map parameter. + +Measured false positive in openai-agents-python: realtime/session.py:954 and +run_internal/turn_resolution.py:1744. AG003 matches a bare `function_map\\s*=`. +""" + +from typing import Any + + +def build(tools: list[Any], handoffs: list[Any]) -> dict[str, Any]: + function_map = {tool.name: tool for tool in tools} + handoff_map = {handoff.tool_name: handoff for handoff in handoffs} + return {**function_map, **handoff_map} diff --git a/tests/corpus/true_negatives/internal_cleanup.py b/tests/corpus/true_negatives/internal_cleanup.py new file mode 100644 index 0000000..56003dc --- /dev/null +++ b/tests/corpus/true_negatives/internal_cleanup.py @@ -0,0 +1,17 @@ +"""Best-effort cleanup of a temp file the code itself created. + +Measured false positive in crewAI (daytona_file_tool.py:366). The path is not +agent-controlled and the call is not a consequential side effect on user data. +""" + +import logging + +logger = logging.getLogger(__name__) + + +def append(sandbox: object, temp_path: str, exit_code: int) -> None: + if exit_code != 0: + try: + sandbox.fs.delete_file(temp_path) + except Exception: + logger.debug("temp-file cleanup failed") diff --git a/tests/corpus/true_negatives/js_fetch_local_url.js b/tests/corpus/true_negatives/js_fetch_local_url.js new file mode 100644 index 0000000..8d69298 --- /dev/null +++ b/tests/corpus/true_negatives/js_fetch_local_url.js @@ -0,0 +1,13 @@ +// `fetch(url)` where `url` is a local variable built from a compile-time host. +// +// This was an assertion in tests/test_rules.py that AG006 *should* fire here. It was +// wrong: a variable named `url` says nothing about where its value came from, and every +// AG006 finding in the field measurement was this shape. Converted from a test +// expectation into a corpus true negative so the rule is measured on it rather than +// merely asserted about. +const BASE = "https://api.example.invalid/v1"; + +export async function loadDocuments(datastoreId) { + const url = `${BASE}/datastores/${datastoreId}/documents`; + return fetch(url, { signal: AbortSignal.timeout(30000) }); +} diff --git a/tests/corpus/true_negatives/path_normaliser.py b/tests/corpus/true_negatives/path_normaliser.py new file mode 100644 index 0000000..981434c --- /dev/null +++ b/tests/corpus/true_negatives/path_normaliser.py @@ -0,0 +1,16 @@ +"""Prefixing a leading slash is normalisation, not granting a filesystem root. + +Measured false positive in crewAI (memory/utils.py:61). AG005's broad-root pattern +matches `path = "/"` and does not distinguish it from `path = "/" + path`. +""" + +import re + + +def normalise(path: str) -> str: + if not path: + return "/" + path = re.sub(r"/+", "/", path) + if not path.startswith("/"): + path = "/" + path + return path.rstrip("/") if len(path) > 1 else path diff --git a/tests/corpus/true_negatives/publishable_keys.py b/tests/corpus/true_negatives/publishable_keys.py new file mode 100644 index 0000000..aa7d8b0 --- /dev/null +++ b/tests/corpus/true_negatives/publishable_keys.py @@ -0,0 +1,9 @@ +"""Keys that are published deliberately. Committing one is not a compromise. + +Measured as Critical false positives in browser-use (PostHog project key) and langgraph +(Supabase anon key). These must still be reported - the publishable and secret halves are +easy to confuse - but at Low, not Critical. +""" + +POSTHOG_PROJECT_API_KEY = "phc_F8JMNjW1i2KbGUTaW1unnDdLSPCoyc52SGRU0Jeca" +STRIPE_PUBLISHABLE_KEY = "pk_live_51H8xKqLmQp4RtVzYwB7NcJdH" diff --git a/tests/corpus/true_positives/.env b/tests/corpus/true_positives/.env new file mode 100644 index 0000000..2256ae1 --- /dev/null +++ b/tests/corpus/true_positives/.env @@ -0,0 +1,6 @@ +# A real .env. Values are fabricated but shaped like the genuine article: unquoted, +# which is the convention and which the quoted assigned-credential pattern misses. +DB_PASSWORD=b7Kq2ZmVx9Lp4Rt6Wn3Jc +export OPENAI_API_KEY=sk-proj-Qw8dLm2VtBnKcXrJf7HsPu4Ay6Ez1Nio +DEBUG=true +PORT=8080 diff --git a/tests/test_corpus.py b/tests/test_corpus.py index d74da45..695c781 100644 --- a/tests/test_corpus.py +++ b/tests/test_corpus.py @@ -9,12 +9,12 @@ from __future__ import annotations import pytest -from tools.bench import CORPUS, load_manifest, measure +from tools.bench import CORPUS, ORIGINS, load_manifest, measure def test_manifest_and_disk_agree() -> None: """load_manifest exits 2 on drift, so reaching the assertions means it is consistent.""" - labels = load_manifest() + labels, _ = load_manifest() assert labels, "corpus is empty" assert (CORPUS / "manifest.yml").exists() @@ -30,27 +30,28 @@ def test_every_label_has_a_reason() -> None: def test_recall_is_total() -> None: """Every rule labelled as firing on a true positive still fires on it.""" - tallies, _, _ = measure(load_manifest()) + tallies, _, _, _ = measure(load_manifest()[0]) missed = {rule_id: tally.fn_locations for rule_id, tally in tallies.items() if tally.false_negatives} assert not missed, f"rules stopped detecting labelled true positives: {missed}" def test_measurement_is_not_degraded_by_scan_errors() -> None: """A benchmark computed from a partial scan is not a benchmark.""" - _, scan_errors, _ = measure(load_manifest()) + _, scan_errors, _, _ = measure(load_manifest()[0]) assert not scan_errors, f"corpus scan produced errors: {scan_errors}" -def test_undiscovered_files_are_reported_not_silently_passed() -> None: +def test_every_corpus_file_is_discovered() -> None: """A file discovery never opens proves nothing; the harness must say so. - `.env.example` is currently in this state, which also means a real `.env` would be - missed. If discovery is extended to cover it, this test should be updated to assert - the new coverage rather than deleted. + `.env.example` used to be in this state, and so did every real `.env` — a secrets + scanner that could not read the canonical secrets file. Discovery now covers them, so + the expected set is empty. If this grows, something stopped being scanned. """ - _, _, undiscovered = measure(load_manifest()) - assert undiscovered == ["true_negatives/.env.example"], ( - "the set of files AgentGuard cannot see has changed; update this test deliberately" + _, _, undiscovered, _ = measure(load_manifest()[0]) + assert undiscovered == [], ( + f"corpus files AgentGuard cannot see: {undiscovered}. Either discovery regressed, " + "or a file shape was added that nothing scans." ) @@ -58,3 +59,24 @@ def test_undiscovered_files_are_reported_not_silently_passed() -> None: def test_corpus_sections_are_populated(section: str) -> None: files = [path for path in (CORPUS / section).rglob("*") if path.is_file()] assert len(files) >= 10, f"{section} has only {len(files)} files" + + +def test_every_case_declares_its_origin() -> None: + """`origin` separates cases drawn from real projects from cases someone composed. + + Enforced rather than inferred: it was previously read by sniffing the prose in `why`, + which miscounted the moment an entry said "field finding" instead of "measured". A + corpus whose negatives were all selected from observed failures is biased towards + passing, so the split has to be reliable enough to score separately. + """ + _, origins = load_manifest() + assert origins, "corpus is empty" + assert set(origins.values()) <= ORIGINS + field = sum(1 for value in origins.values() if value == "field") + assert field, "no case is marked as field-derived; the split has stopped meaning anything" + + +def test_field_only_scoring_is_a_strict_subset() -> None: + labels, origins = load_manifest() + field_only = {p: e for p, e in labels.items() if origins[p] == "field"} + assert 0 < len(field_only) < len(labels) diff --git a/tests/test_coverage.py b/tests/test_coverage.py new file mode 100644 index 0000000..396dda2 --- /dev/null +++ b/tests/test_coverage.py @@ -0,0 +1,233 @@ +"""Per-rule line bounds, and coverage reported rather than assumed. + +A credential inlined in a minified bundle sits past any sane per-line bound, and that is +a common real leak. The secret rule therefore reads lines whole while every other rule +stays bounded, and what was clipped is declared in every report. +""" + +from __future__ import annotations + +from collections.abc import Iterable +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from agentguard.cli import app +from agentguard.config import Config +from agentguard.context import DEFAULT_MAX_LINE_LENGTH, UNBOUNDED, SourceFile +from agentguard.models import Finding, Severity +from agentguard.rules.base import Rule, RuleMetadata +from agentguard.scanner import Scanner + +runner = CliRunner() + +KEY = "sk-proj-Qw8dLm2VtBnKcXrJf7HsPu4Ay6Ez1Nio" +FILLER = '!function(e,t){"object"==typeof exports?module.exports=t():e.x=t()}(this,function(){});' + + +def _bundle(offset: int) -> str: + head = (FILLER * (offset // len(FILLER) + 1))[:offset] + return f'{head}var API_KEY="{KEY}";{FILLER}\n' + + +@pytest.mark.parametrize("offset", [100, 3000, 5000, 20000, 100000]) +def test_credential_is_found_at_any_offset_on_a_minified_line(project: Path, offset: int) -> None: + """The regression: a key past 4096 chars was previously invisible.""" + (project / "bundle.js").write_text(_bundle(offset), encoding="utf-8") + findings = Scanner(Config()).scan(project).findings + assert [f.rule_id for f in findings if f.rule_id == "AG001"] == ["AG001"], ( + f"credential at offset {offset} was not detected" + ) + + +def test_secret_rule_declares_unbounded_and_is_measured_linear() -> None: + from agentguard.rules.secrets import HardcodedSecretRule + + assert HardcodedSecretRule.metadata.max_line_length == UNBOUNDED + + +def test_other_rules_stay_bounded(project: Path) -> None: + """Only the measured-linear rule opts out; the default bound still applies elsewhere.""" + from agentguard.rules.code import DangerousExecutionRule + + assert DangerousExecutionRule.metadata.max_line_length is None + (project / "b.js").write_text("x".ljust(DEFAULT_MAX_LINE_LENGTH + 500, "y") + "\n", encoding="utf-8") + result = Scanner(Config()).scan(project) + assert not result.fully_covered + assert result.truncated[0].bound == DEFAULT_MAX_LINE_LENGTH + + +def test_coverage_is_reported_not_silent(project: Path) -> None: + (project / "b.js").write_text("z" * 9000 + "\n", encoding="utf-8") + result = Scanner(Config()).scan(project) + + assert result.truncated_lines == 1 + item = result.truncated[0] + assert (item.line, item.length, item.bound) == (1, 9000, DEFAULT_MAX_LINE_LENGTH) + assert item.to_dict(result.root)["withheld"] == 9000 - DEFAULT_MAX_LINE_LENGTH + + +def test_coverage_appears_in_every_report(project: Path) -> None: + import json + + from agentguard.reporters import REPORTERS + + (project / "b.js").write_text("z" * 9000 + "\n", encoding="utf-8") + result = Scanner(Config()).scan(project) + + payload = json.loads(REPORTERS["json"](result)) + assert payload["coverage"]["fully_covered"] is False + assert payload["coverage"]["truncated_lines"][0]["withheld"] == 9000 - DEFAULT_MAX_LINE_LENGTH + + assert "Coverage" in REPORTERS["markdown"](result) + + sarif = json.loads(REPORTERS["sarif"](result)) + notes = sarif["runs"][0]["invocations"][0]["toolExecutionNotifications"] + assert any("Coverage:" in n["message"]["text"] for n in notes) + + +def test_full_coverage_reports_clean(project: Path) -> None: + (project / "a.py").write_text("value = 1\n", encoding="utf-8") + result = Scanner(Config()).scan(project) + assert result.fully_covered + assert result.truncated == [] + + +# --- the flag ------------------------------------------------------------------------- + + +def test_truncation_does_not_gate_by_default(project: Path) -> None: + (project / "b.js").write_text("z" * 9000 + "\n", encoding="utf-8") + assert runner.invoke(app, ["scan", str(project)]).exit_code == 0 + + +def test_fail_on_incomplete_gates(project: Path) -> None: + (project / "b.js").write_text("z" * 9000 + "\n", encoding="utf-8") + result = runner.invoke(app, ["scan", str(project), "--fail-on-incomplete"]) + assert result.exit_code == 2 + assert "Coverage incomplete" in result.output + + +def test_fail_on_incomplete_passes_when_fully_covered(project: Path) -> None: + (project / "a.py").write_text("value = 1\n", encoding="utf-8") + assert runner.invoke(app, ["scan", str(project), "--fail-on-incomplete"]).exit_code == 0 + + +# --- the declaration is enforced ------------------------------------------------------ + + +def test_negative_bound_is_rejected() -> None: + with pytest.raises(ValueError, match="max_line_length"): + RuleMetadata( + "XX001", + "t", + Severity.LOW, + "c", + "d", + languages=frozenset({"python"}), + max_line_length=-1, + ) + + +class UnboundedRule(Rule): + metadata = RuleMetadata( + "XX901", + "Unbounded", + Severity.LOW, + "test", + "sees whole lines", + languages=frozenset({"python"}), + fixture_policy="report", + max_line_length=UNBOUNDED, + ) + + def scan(self, source: SourceFile) -> Iterable[Finding]: + for number, line in enumerate(source.lines, 1): + if "NEEDLE" in line: + yield self.finding(source, number, "found", "risk", "fix") + + +def test_a_rule_declaring_unbounded_sees_the_whole_line(project: Path) -> None: + (project / "a.py").write_text("x" * 9000 + "NEEDLE\n", encoding="utf-8") + result = Scanner(Config(), rules=[UnboundedRule()]).scan(project) + assert [f.rule_id for f in result.findings] == ["XX901"] + + +# --- which bound coverage is computed against ------------------------------------------ + + +class TightlyBoundRule(Rule): + """Sees very little of each line.""" + + metadata = RuleMetadata( + "XX902", + "Tight", + Severity.LOW, + "test", + "narrow view", + languages=frozenset({"python"}), + fixture_policy="report", + max_line_length=100, + ) + + def scan(self, source: SourceFile) -> Iterable[Finding]: + return [] + + +class LooselyBoundRule(Rule): + metadata = RuleMetadata( + "XX903", + "Loose", + Severity.LOW, + "test", + "wide view", + languages=frozenset({"python"}), + fixture_policy="report", + max_line_length=4096, + ) + + def scan(self, source: SourceFile) -> Iterable[Finding]: + return [] + + +def test_coverage_uses_the_tightest_applied_bound(project: Path) -> None: + """A line is a coverage gap if *any* rule was shown less than all of it. + + With rules at 100 and 4096, a 200-char line is withheld from the first and not the + second. Computing coverage against the loosest bound reports nothing here — and once + a rule declares UNBOUNDED that failure becomes total, because the loosest bound is + then unbounded and no line is ever over it. This pins the direction. + """ + (project / "a.py").write_text("x" * 200 + "\n", encoding="utf-8") + + result = Scanner(Config(), rules=[TightlyBoundRule(), LooselyBoundRule()]).scan(project) + + assert not result.fully_covered, "a 200-char line is withheld from the 100-char rule" + assert result.truncated_lines == 1 + assert result.truncated[0].bound == 100, "must report the tightest bound, not the loosest" + assert result.truncated[0].length == 200 + + +def test_an_unbounded_rule_does_not_erase_coverage_for_bounded_ones(project: Path) -> None: + """Mixing UNBOUNDED with a bounded rule still reports the bounded rule's gap. + + This does *not* discriminate min from max — positive bounds are filtered before + either is applied, so UNBOUNDED cannot win the comparison and both give 100 here. + `test_coverage_uses_the_tightest_applied_bound` is the test that catches that + inversion. This one pins the adjacent property: declaring a rule unbounded must not + be a way to make the run look fully covered. + """ + (project / "a.py").write_text("x" * 200 + "\n", encoding="utf-8") + + result = Scanner(Config(), rules=[TightlyBoundRule(), UnboundedRule()]).scan(project) + + assert not result.fully_covered, "the unbounded rule must not mask the bounded rule's gap" + assert result.truncated[0].bound == 100 + + +def test_all_rules_unbounded_means_full_coverage(project: Path) -> None: + """When nothing is bounded there is genuinely no gap to report.""" + (project / "a.py").write_text("x" * 9000 + "\n", encoding="utf-8") + result = Scanner(Config(), rules=[UnboundedRule()]).scan(project) + assert result.fully_covered diff --git a/tests/test_linearity_gate.py b/tests/test_linearity_gate.py new file mode 100644 index 0000000..9032690 --- /dev/null +++ b/tests/test_linearity_gate.py @@ -0,0 +1,78 @@ +"""The gate that decides whether a pattern may run unbounded. + +A rule declaring UNBOUNDED reads whole minified lines, so a non-linear pattern there is a +denial-of-service vector. `tools.measure_linearity --check` runs in CI to enforce that for +patterns nobody has written yet. These tests check the gate itself can fail — the measure +is only worth having if it registers a bad pattern, and it is the last thing standing +between a contributor's regex and an unbounded read. +""" + +from __future__ import annotations + +import re + +import pytest +from tools import measure_linearity as gate + +from agentguard.context import UNBOUNDED +from agentguard.rules import BUILTIN_RULES + + +def test_the_known_cubic_control_is_classified_non_linear() -> None: + """If this stops failing, every result the gate reports is worthless.""" + exponent, _ = gate.growth(gate._KNOWN_CUBIC, lambda n: "prompt=f'" * (n // 9) + "{req", gate.SIZES[:4]) + assert exponent >= 1.5, f"the control measured {exponent:.2f}; the harness is broken" + assert gate._verdict(exponent) in {"SUPER-LINEAR", "NON-LINEAR"} + + +def test_a_linear_pattern_is_classified_linear() -> None: + exponent, _ = gate.growth(re.compile(r"\bAKIA[A-Z0-9]{16}\b"), lambda n: "AKIA" * (n // 4)) + assert gate._verdict(exponent) == "linear" + + +def test_generic_stress_inputs_are_derived_from_the_pattern() -> None: + """A pattern added tomorrow is stressed without editing the tool.""" + labels = [label for label, _ in gate.stress_inputs(re.compile(r"secret\s*=\s*['\"]"))] + assert "filler" in labels + assert any("secret" in label for label in labels), "literal runs must be mined from the source" + + +def test_patterns_are_discovered_by_introspection() -> None: + """Not a hand-maintained list: a new pattern attribute is picked up automatically.""" + from agentguard.rules.secrets import HardcodedSecretRule + + names = {name for name, _ in gate.patterns_of(HardcodedSecretRule)} + assert len(names) >= 6, f"expected every compiled pattern, found {names}" + assert "_placeholder" in names, "bare attributes must be found, not just tuples" + assert any(n.startswith("_patterns[") for n in names), "patterns inside tuples must be found" + + +def test_a_new_pattern_on_an_unbounded_rule_is_covered() -> None: + """The regression this gate exists for: someone adds a pattern and nothing checks it.""" + from agentguard.rules.secrets import HardcodedSecretRule + + class WithNewPattern(HardcodedSecretRule): # type: ignore[misc] + _added_later = re.compile(r"\bnew-[A-Za-z0-9]{10,}\b") + + names = {name for name, _ in gate.patterns_of(WithNewPattern)} + assert "_added_later" in names + + +def test_every_unbounded_builtin_is_currently_linear() -> None: + """The live guarantee, asserted in the ordinary test run as well as in CI.""" + unbounded = [r for r in BUILTIN_RULES if r.metadata.max_line_length == UNBOUNDED] + assert unbounded, "AG001 is expected to declare UNBOUNDED" + for rule in unbounded: + for name, pattern in gate.patterns_of(rule): + exponent, _, label = gate.worst_growth(pattern) + assert exponent < gate.LINEAR_CEILING, ( + f"{rule.metadata.id}.{name} measured {exponent:.2f} on {label}; it must not declare UNBOUNDED" + ) + + +@pytest.mark.parametrize( + ("exponent", "expected"), + [(0.9, "linear"), (1.0, "linear"), (1.34, "linear"), (1.4, "SUPER-LINEAR"), (2.9, "NON-LINEAR")], +) +def test_verdict_boundaries(exponent: float, expected: str) -> None: + assert gate._verdict(exponent) == expected diff --git a/tests/test_redos.py b/tests/test_redos.py index e3ddf7c..55fae16 100644 --- a/tests/test_redos.py +++ b/tests/test_redos.py @@ -140,7 +140,8 @@ def test_long_lines_are_bounded_and_the_bound_is_reported(project) -> None: # t def test_line_bound_applies_to_rule_input(project) -> None: # type: ignore[no-untyped-def] source = SourceFile(project / "x.py", project, "b" * 100 + "\nshort\n", "python", max_line_length=10) assert [len(line) for line in source.lines] == [10, 5] - assert source.truncated_lines == 1 + assert source.over_bound(10) == [(1, 100)] + assert source.lines_bounded(0) == ["b" * 100, "short"], "UNBOUNDED returns lines whole" def test_line_bound_preserves_line_numbering(project) -> None: # type: ignore[no-untyped-def] diff --git a/tests/test_rule_context.py b/tests/test_rule_context.py index a4dbcd1..bf6c45e 100644 --- a/tests/test_rule_context.py +++ b/tests/test_rule_context.py @@ -235,3 +235,179 @@ def test_a_checkout_under_a_directory_named_test_is_not_all_fixtures(project: Pa (root / "src" / "agent.py").write_text('api_key = "b7Kq2ZmVx9Lp4Rt6Wn3Jc"\n', encoding="utf-8") findings = Scanner(Config()).scan(root).findings assert [f.severity for f in findings] == [Severity.CRITICAL] + + +# --- credential class ------------------------------------------------------------------- + + +def test_publishable_key_is_capped_at_low(project: Path) -> None: + """A PostHog project key ships in client JavaScript. Committing it is not a compromise. + + Measured as a Critical false positive in browser-use and langgraph. + """ + (project / "telemetry.py").write_text( + "POSTHOG_PROJECT_API_KEY = 'phc_F8JMNjW1i2KbGUTaW1unnDdLSPCoyc52SGRU0Jeca'\n", + encoding="utf-8", + ) + findings = [f for f in Scanner(Config()).scan(project).findings if f.rule_id == "AG001"] + assert findings, "a publishable key is still worth reporting" + assert findings[0].severity == Severity.LOW + assert findings[0].metadata["credential_class"] == "public" + assert "publishable by design" in findings[0].explanation + + +def test_public_by_name_is_capped_at_low(project: Path) -> None: + (project / "constants.py").write_text( + 'SUPABASE_PUBLIC_API_KEY = "eyJhbGciOiJIUzI1NiJ9.b7Kq2ZmVx9Lp4Rt6Wn3Jc.sig"\n', + encoding="utf-8", + ) + findings = [f for f in Scanner(Config()).scan(project).findings if f.rule_id == "AG001"] + assert findings and findings[0].severity == Severity.LOW + + +def test_an_ordinary_secret_is_not_capped(project: Path) -> None: + """The cap must not swallow the case the rule exists for.""" + (project / "config.py").write_text('DATABASE_PASSWORD = "b7Kq2ZmVx9Lp4Rt6Wn3Jc"\n', encoding="utf-8") + findings = [f for f in Scanner(Config()).scan(project).findings if f.rule_id == "AG001"] + assert findings[0].severity == Severity.CRITICAL + assert findings[0].metadata["credential_class"] == "secret" + + +# --- vendored paths --------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "relative", + ["vendor/dep.py", "third_party/dep.py", "lib/site-packages/dep.py", "bundled/dep.py"], +) +def test_vendored_paths_are_recognised(project: Path, relative: str) -> None: + path = project / relative + assert SourceFile(path, project, "", "python").is_vendored + + +def test_findings_in_vendored_code_are_downgraded(project: Path) -> None: + """A finding in someone else's release is not actionable where it is reported.""" + vendored = project / "vendor" / "dep" + vendored.mkdir(parents=True) + (vendored / "runner.py").write_text("import os\n\n\ndef r(c):\n os.system(c)\n", encoding="utf-8") + assert "AG002" not in _rules(Scanner(Config()).scan(project).findings) + + +def test_first_party_code_is_unaffected_by_the_vendored_rule(project: Path) -> None: + src = project / "src" + src.mkdir() + (src / "runner.py").write_text("import os\n\n\ndef r(c):\n os.system(c)\n", encoding="utf-8") + assert "AG002" in _rules(Scanner(Config()).scan(project).findings) + + +# --- shell=True with an argument list --------------------------------------------------- + + +def test_shell_true_with_a_list_describes_the_portability_bug(project: Path) -> None: + """POSIX passes only argv[0] to the shell. That is the defect, not injection.""" + (project / "cli.py").write_text( + 'import subprocess\n\n\ndef v(cmd):\n subprocess.run([cmd, "--version"], shell=True)\n', + encoding="utf-8", + ) + findings = [f for f in Scanner(Config()).scan(project).findings if f.rule_id == "AG002"] + assert findings, "still reported - it is a real defect" + assert "argument list" in findings[0].explanation + assert "silently discarded" in findings[0].risk + assert findings[0].metadata["defect_class"] == "portability" + + +def test_shell_true_with_a_string_is_still_an_injection_finding(project: Path) -> None: + (project / "cli.py").write_text( + "import subprocess\n\n\ndef v(cmd):\n subprocess.run(cmd, shell=True)\n", encoding="utf-8" + ) + findings = [f for f in Scanner(Config()).scan(project).findings if f.rule_id == "AG002"] + assert findings and "no enforceable command boundary" in findings[0].explanation + + +# --- dispositions of the five corpus failures ------------------------------------------- + + +def test_ag003_no_longer_flags_a_local_named_function_map(project: Path) -> None: + """FIXED. The clause matched any variable of that name, and where it did hit + AutoGen's real parameter it flagged a function map's existence, not its breadth.""" + content = "def build(tools):\n function_map = {t.name: t for t in tools}\n return function_map\n" + assert "AG003" not in _rules(_findings(project, "mod.py", content)) + + +def test_ag003_still_flags_an_explicit_capability_grant(project: Path) -> None: + content = "agent = Agent(role='ops', allow_dangerous_code=True)\n" + assert "AG003" in _rules(_findings(project, "mod.py", content)) + + +def test_ag005_no_longer_flags_a_path_normaliser(project: Path) -> None: + """FIXED. `path = "/" + path` prefixes a separator; it does not grant a root.""" + content = 'def n(path):\n if not path.startswith("/"):\n path = "/" + path\n return path\n' + assert "AG005" not in _rules(_findings(project, "mod.py", content)) + + +def test_ag005_still_flags_a_genuinely_broad_root(project: Path) -> None: + assert "AG005" in _rules(_findings(project, "mod.py", 'workspace = "/"\n')) + + +def test_ag006_no_longer_flags_a_variable_merely_named_url(project: Path) -> None: + """FIXED. Every AG006 field finding was a compile-time host over TLS with a timeout.""" + content = ( + "import requests\n\n\n" + "def get(ds):\n" + ' url = f"https://api.example.invalid/v1/{ds}"\n' + " return requests.get(url, timeout=30)\n" + ) + assert "AG006" not in _rules(_findings(project, "mod.py", content)) + + +def test_ag006_still_flags_an_actually_untrusted_source(project: Path) -> None: + content = "import requests\n\n\ndef get(tool_input):\n return requests.get(tool_input, timeout=5)\n" + assert "AG006" in _rules(_findings(project, "mod.py", content)) + + +def test_ag006_still_flags_plaintext_http(project: Path) -> None: + content = 'import requests\n\n\ndef get():\n return requests.get("http://x.invalid/a")\n' + assert "AG006" in _rules(_findings(project, "mod.py", content)) + + +def test_ag002_no_longer_flags_exec_of_a_module_constant(project: Path) -> None: + """FIXED. The executed argument is a name bound once to a literal at module scope.""" + content = ( + '_IMPORTS = "import os\\n"\n\n\ndef build():\n ns = {}\n exec(_IMPORTS, ns)\n return ns\n' + ) + assert "AG002" not in _rules(_findings(project, "mod.py", content)) + + +def test_ag002_still_flags_exec_of_a_reassigned_name(project: Path) -> None: + """ "Bound exactly once" is load-bearing: a rebindable name is not a constant.""" + content = ( + '_IMPORTS = "import os\\n"\n\n\n' + "def override(v):\n" + " global _IMPORTS\n" + " _IMPORTS = v\n\n\n" + "def build():\n exec(_IMPORTS, {})\n" + ) + assert "AG002" in _rules(_findings(project, "mod.py", content)) + + +def test_ag002_still_flags_exec_of_a_parameter(project: Path) -> None: + assert "AG002" in _rules(_findings(project, "mod.py", "def run(code):\n exec(code, {})\n")) + + +@pytest.mark.xfail( + reason=( + "ACCEPTED PRECISION LIMIT, not a bug to be fixed silently. AG008 cannot tell a " + "tool deleting a caller-supplied path from a function deleting a temp file it " + "created itself; that needs data flow the rule does not have. Recorded in " + "docs/DECISIONS.md. This xfail is the marker - if it ever XPASSes, the limit has " + "been closed and the disposition should be revisited." + ), + strict=True, +) +def test_ag008_internal_cleanup_is_a_known_limit(project: Path) -> None: + content = ( + "def append(sandbox, temp_path, code):\n" + " if code != 0:\n" + " sandbox.fs.delete_file(temp_path)\n" + ) + assert "AG008" not in _rules(_findings(project, "mod.py", content)) diff --git a/tests/test_rules.py b/tests/test_rules.py index 4f214e0..770372c 100644 --- a/tests/test_rules.py +++ b/tests/test_rules.py @@ -24,7 +24,11 @@ ("requirements.txt", "langchain", "AG010"), ("agent.ts", "execSync(command)", "AG002"), ("agent.ts", "const prompt = `Read ${web_content}`", "AG004"), - ("agent.ts", "fetch(url)", "AG006"), + # `fetch(url)` used to be asserted here. It was a wrong expectation defending a + # false positive, and it is now a corpus true negative + # (true_negatives/js_fetch_local_url.js) so the rule is measured on it rather than + # asserted about. This case keeps the positive direction covered. + ("agent.ts", "fetch(user_input)", "AG006"), ("agent.ts", "tool({ name: 'lookup', execute: run })", "AG007"), ], ) diff --git a/tools/bench.py b/tools/bench.py index 49dcf0e..eb6a0e7 100644 --- a/tools/bench.py +++ b/tools/bench.py @@ -29,6 +29,12 @@ CORPUS = Path(__file__).resolve().parent.parent / "tests" / "corpus" SECTIONS = ("true_positives", "true_negatives") +#: Where a corpus case came from. `field` means it reproduces a false positive or missed +#: finding observed by scanning a real project; `written` means it was composed to cover a +#: case someone thought of. The distinction matters because a corpus whose negatives were +#: all selected from observed failures is biased towards passing, so the two are scored +#: separately rather than blended into one flattering number. +ORIGINS = frozenset({"field", "written"}) @dataclass @@ -50,9 +56,11 @@ def recall(self) -> float | None: return None if denominator == 0 else self.true_positives / denominator -def load_manifest() -> dict[Path, set[str]]: +def load_manifest() -> tuple[dict[Path, set[str]], dict[Path, str]]: + """Return (expectations, origins). Both keyed by path.""" raw = yaml.safe_load((CORPUS / "manifest.yml").read_text(encoding="utf-8")) labels: dict[Path, set[str]] = {} + origins: dict[Path, str] = {} problems: list[str] = [] for section in SECTIONS: for name, entry in (raw.get(section) or {}).items(): @@ -62,6 +70,12 @@ def load_manifest() -> dict[Path, set[str]]: continue if not (entry or {}).get("why", "").strip(): problems.append(f"no `why` given for {section}/{name}") + origin = (entry or {}).get("origin") + if origin not in ORIGINS: + problems.append( + f"{section}/{name}: `origin` must be one of {', '.join(sorted(ORIGINS))}, got {origin!r}" + ) + origins[path] = origin if origin in ORIGINS else "written" labels[path] = set(entry.get("expect") or []) on_disk = {path for section in SECTIONS for path in (CORPUS / section).rglob("*") if path.is_file()} @@ -72,16 +86,19 @@ def load_manifest() -> dict[Path, set[str]]: for problem in problems: print(f"corpus error: {problem}", file=sys.stderr) raise SystemExit(2) - return labels + return labels, origins -def measure(labels: dict[Path, set[str]]) -> tuple[dict[str, Tally], list[str], list[str]]: +def measure( + labels: dict[Path, set[str]], +) -> tuple[dict[str, Tally], list[str], list[str], list[str]]: tallies: dict[str, Tally] = defaultdict(Tally) for rule in BUILTIN_RULES: tallies[rule.metadata.id] = Tally() scan_errors: list[str] = [] undiscovered: list[str] = [] + misbehaving: list[str] = [] for path, expected in sorted(labels.items()): result = Scanner(Config()).scan(path) scan_errors.extend(result.errors) @@ -95,6 +112,8 @@ def measure(labels: dict[Path, set[str]]) -> tuple[dict[str, Tally], list[str], fired[finding.rule_id].append(finding.location.line) rel = path.relative_to(CORPUS).as_posix() + if set(fired) != expected: + misbehaving.append(rel) for rule_id, lines in fired.items(): if rule_id in expected: tallies[rule_id].true_positives += 1 @@ -104,15 +123,23 @@ def measure(labels: dict[Path, set[str]]) -> tuple[dict[str, Tally], list[str], for rule_id in expected - set(fired): tallies[rule_id].false_negatives += 1 tallies[rule_id].fn_locations.append(rel) - return dict(tallies), scan_errors, undiscovered + return dict(tallies), scan_errors, undiscovered, misbehaving def _pct(value: float | None) -> str: return " — " if value is None else f"{value * 100:5.1f}%" -def render(tallies: dict[str, Tally], scan_errors: list[str], undiscovered: list[str]) -> None: - print(f"AgentGuard detection benchmark — {len(BUILTIN_RULES)} rules\n") +def render( + tallies: dict[str, Tally], + scan_errors: list[str], + undiscovered: list[str], + total: int, + subset: bool, + misbehaving: list[str], +) -> None: + scope = "field-derived cases only" if subset else f"{total} labelled cases" + print(f"AgentGuard detection benchmark — {len(BUILTIN_RULES)} rules, {scope}\n") print("| Rule | TP | FP | FN | Precision | Recall |") print("|-------|----:|----:|----:|----------:|-------:|") totals = Tally() @@ -139,6 +166,14 @@ def render(tallies: dict[str, Tally], scan_errors: list[str], undiscovered: list if any(t.fp_locations or t.fn_locations for t in tallies.values()): print() + # Precision over a subset selected from observed failures is structurally skewed - the + # field-derived cases are nearly all negatives, because a field false positive becomes a + # true negative here. How many behave as labelled is the statistic that survives that. + behaving = total - len(misbehaving) + print(f"\n{behaving} of {total} cases behave as labelled.") + for name in misbehaving: + print(f" does not: {name}") + if undiscovered: print( f"\n{len(undiscovered)} corpus file(s) never opened by file discovery. These measure" @@ -156,9 +191,17 @@ def render(tallies: dict[str, Tally], scan_errors: list[str], undiscovered: list def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--json", action="store_true", help="emit JSON instead of a table") + parser.add_argument( + "--field-only", + action="store_true", + help="score only cases derived from scanning real projects, excluding composed ones", + ) args = parser.parse_args() - tallies, scan_errors, undiscovered = measure(load_manifest()) + labels, origins = load_manifest() + if args.field_only: + labels = {path: expected for path, expected in labels.items() if origins[path] == "field"} + tallies, scan_errors, undiscovered, misbehaving = measure(labels) if args.json: print( json.dumps( @@ -175,6 +218,8 @@ def main() -> int: } for rule_id, t in sorted(tallies.items()) }, + "cases": len(labels), + "misbehaving": misbehaving, "scan_errors": scan_errors, "undiscovered": undiscovered, }, @@ -182,7 +227,7 @@ def main() -> int: ) ) else: - render(tallies, scan_errors, undiscovered) + render(tallies, scan_errors, undiscovered, len(labels), args.field_only, misbehaving) return 0 diff --git a/tools/measure_linearity.py b/tools/measure_linearity.py new file mode 100644 index 0000000..e8eebc5 --- /dev/null +++ b/tools/measure_linearity.py @@ -0,0 +1,182 @@ +"""Measure how each rule's regexes grow with input length, and gate on it. + +A rule may only declare ``max_line_length=UNBOUNDED`` if its patterns are linear. An +unbounded non-linear pattern is a denial-of-service vector: AG004 was cubic and took 34 +seconds on 28 KB of a single line, and the file-size limit permits 1 MB. + +This discovers every rule declaring ``UNBOUNDED`` and every compiled pattern on it, so a +contributor adding a pattern is covered without editing this file. Run with ``--check`` +in CI: it exits non-zero if any unbounded rule owns a pattern that is not linear. + +The harness validates itself first against a pattern known to be cubic. If that control +does not come back non-linear, the measurement is not trustworthy and nothing is +reported. A harness that cannot fail is not a harness — and this one is what stands +between a contributor's regex and an unbounded read. + + python -m tools.measure_linearity # table + python -m tools.measure_linearity --check # exit 1 if any unbounded rule is unsafe +""" + +from __future__ import annotations + +import argparse +import math +import re +import statistics +import time +from collections.abc import Callable, Iterator + +from agentguard.context import UNBOUNDED +from agentguard.rules import BUILTIN_RULES +from agentguard.rules.base import Rule + +SIZES = (2000, 4000, 8000, 16000, 32000) +LINEAR_CEILING = 1.35 +MAX_FILE_BYTES = 1024 * 1024 + +_SRC = ( + r"(?:request|req|input|user_input|user_message|web_content|document|page|result" + r"|tool_output|message\.content)" +) +#: AG004 as it stood before the two-step rewrite. Known cubic. Control only. +_KNOWN_CUBIC = re.compile( + rf"(?i)(?:prompt|system_message|instructions?)\s*=.*" + rf"(?:f['\"].*\{{{_SRC}\}}|\.format\([^)]*{_SRC}|\+\s*{_SRC})" +) + +#: Literal runs inside a regex source, used to build inputs that almost match. +_LITERAL_RUN = re.compile(r"[A-Za-z0-9_/:.-]{3,}") + + +def _time_once(pattern: re.Pattern[str], text: str) -> float: + start = time.perf_counter() + pattern.search(text) + return time.perf_counter() - start + + +def growth( + pattern: re.Pattern[str], build: Callable[[int], str], sizes: tuple[int, ...] = SIZES +) -> tuple[float, float]: + """Return (exponent, seconds at the largest size). Exponent ~1 is linear.""" + times = [min(_time_once(pattern, build(n)) for _ in range(3)) for n in sizes] + ratios = [ + math.log(times[i] / times[i - 1]) / math.log(sizes[i] / sizes[i - 1]) + for i in range(1, len(sizes)) + if times[i] > 0 and times[i - 1] > 0 + ] + return (statistics.median(ratios) if ratios else float("nan")), times[-1] + + +def stress_inputs(pattern: re.Pattern[str]) -> Iterator[tuple[str, Callable[[int], str]]]: + """Adversarial builders derived from the pattern itself. + + Backtracking blows up on input that *nearly* matches, so the useful inputs are built + from the pattern's own literal runs, repeated, with the match denied at the end. + Generic by construction: a pattern added tomorrow is stressed the same way. + """ + yield "filler", lambda n: "a" * n + yield "quotes", lambda n: "'\"" * (n // 2) + literals = sorted(set(_LITERAL_RUN.findall(pattern.pattern)), key=len, reverse=True)[:3] + for literal in literals: + yield f"repeat {literal!r}", lambda n, lit=literal: (lit * (n // len(lit) + 1))[:n] + yield ( + f"repeat {literal!r}+sep", + lambda n, lit=literal: ((lit + " ") * (n // (len(lit) + 1) + 1))[:n], + ) + yield ( + f"near-miss {literal!r}", + lambda n, lit=literal: ((lit + '="') * (n // (len(lit) + 2) + 1))[:n], + ) + + +def worst_growth(pattern: re.Pattern[str]) -> tuple[float, float, str]: + """Worst (exponent, time, label) over every stress input for this pattern.""" + worst = (0.0, 0.0, "none") + for label, build in stress_inputs(pattern): + exponent, elapsed = growth(pattern, build) + if not math.isnan(exponent) and exponent > worst[0]: + worst = (exponent, elapsed, label) + return worst + + +def patterns_of(rule: type[Rule]) -> Iterator[tuple[str, re.Pattern[str]]]: + """Every compiled regex reachable as a class attribute, including inside tuples. + + Introspection rather than a registry, so a contributor does not have to remember to + declare a new pattern in order for it to be gated. + """ + for name, value in vars(rule).items(): + if isinstance(value, re.Pattern): + yield name, value + elif isinstance(value, tuple | list): + for index, item in enumerate(value): + if isinstance(item, re.Pattern): + yield f"{name}[{index}]", item + elif isinstance(item, tuple | list): + for sub in item: + if isinstance(sub, re.Pattern): + yield f"{name}[{index}]", sub + + +def _verdict(exponent: float) -> str: + if exponent < LINEAR_CEILING: + return "linear" + return "SUPER-LINEAR" if exponent < 2.5 else "NON-LINEAR" + + +def main() -> int: + parser = argparse.ArgumentParser(description="Measure regex growth for unbounded rules.") + parser.add_argument( + "--check", + action="store_true", + help="exit 1 if any rule declaring UNBOUNDED owns a non-linear pattern", + ) + args = parser.parse_args() + + exponent, _ = growth(_KNOWN_CUBIC, lambda n: "prompt=f'" * (n // 9) + "{req", SIZES[:4]) + print(f"control (known-cubic AG004): exponent {exponent:.2f} — {_verdict(exponent)}") + if exponent < 1.5: + print("HARNESS BROKEN: the control did not register as non-linear. Not reporting.") + return 2 + print("control registers. Measuring.\n") + + unbounded = [r for r in BUILTIN_RULES if r.metadata.max_line_length == UNBOUNDED] + if not unbounded: + print("No rule declares UNBOUNDED. Nothing to gate.") + return 0 + + header = f"{'rule':7s} {'pattern':22s} {'exp':>5s} {'32KB':>9s} {'1MB':>10s} worst input" + print(header) + print("-" * len(header)) + failures: list[str] = [] + for rule in unbounded: + for name, pattern in patterns_of(rule): + exponent, elapsed, label = worst_growth(pattern) + projected = elapsed * (MAX_FILE_BYTES / SIZES[-1]) ** exponent + print( + f"{rule.metadata.id:7s} {name:22s} {exponent:5.2f} {elapsed * 1000:8.2f}ms " + f"{projected * 1000:9.1f}ms {label} [{_verdict(exponent)}]" + ) + if exponent >= LINEAR_CEILING: + failures.append( + f"{rule.metadata.id}.{name} — {_verdict(exponent)}, exponent {exponent:.2f}, " + f"worst on {label}" + ) + + print() + if failures: + print("Declared UNBOUNDED but not linear:") + for failure in failures: + print(f" {failure}") + print( + "\nEither rewrite the pattern, or give the rule a finite max_line_length until it " + "is rewritten.\nAn unbounded non-linear pattern is a denial-of-service vector." + ) + return 1 if args.check else 0 + checked = sum(1 for r in unbounded for _ in patterns_of(r)) + print(f"All linear: {checked} pattern(s) across {len(unbounded)} unbounded rule(s).") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main())