mirror of
https://github.com/trailofbits/skills.git
synced 2026-09-14 14:28:48 +08:00
main
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 iflov
|
7e7a9b2a5d | supply-chain-risk-auditor: discard undecodable cache entries (#291) | ||
|
kz-tob
|
7dee682744 |
supply-chain-risk-auditor: don't crash on Windows in the cache owner check (#284)
* supply-chain-risk-auditor: don't crash on Windows in the cache owner check Fixes #273. Http.__init__ called os.getuid() unconditionally, which does not exist on Windows, so collect.py raised AttributeError before it collected anything. Reproduced the reporter's grep: this was the only POSIX-only call in the package, and the rest is stdlib-only, so nothing else stood in the way of a Windows run. The check is a real control, not a formality — the collector trusts the cache for registry and advisory responses, and a cache another user can write turns a compromised package into a clean "no advisories" verdict. So this takes the issue's first option rather than a bare hasattr guard: on a platform with no os.getuid the check is skipped and returns a caveat naming what went unverified, which collect() appends to the report notes. It surfaces under "Method and caveats", the same channel the report already uses to separate assessed-clean from unassessable-with-a-reason. A silently dropped control would let the report imply a cache it never checked. The POSIX path is unchanged: a foreign owner still aborts. Not implementing the SID comparison the issue offers as option 2. It needs pywin32, and the package declares dependencies = [] — a third-party dependency to harden a fallback is a poor trade, and the caveat keeps the gap visible either way. Three tests, each mutation-checked. Restoring the original unguarded call reproduces the exact AttributeError from the issue; replacing the caveat with a bare `return None` — the shortcut the reporter warned against — fails the test that requires a reason; dropping the SystemExit fails the foreign-owner test. Left alone: the reporter also notes gh_token() has no GH_TOKEN/ GITHUB_TOKEN fallback and offered to file it separately. That is a different defect and belongs in its own issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * supply-chain-risk-auditor: name the encoding on every text read and write Review P2 on #284: the PR claimed the getuid fix was the last thing standing between this plugin and a Windows run. Not true. No read_text or write_text passed encoding=, so on Windows they decode with the ANSI code page — cp1252, not UTF-8. A package.json with a non-ASCII author either raised UnicodeDecodeError, which is a ValueError but not a JSONDecodeError and so escaped _read_json's guards as a traceback, or where the bytes happened to be cp1252-decodable was silently mojibaked into the report. The codebase already knew this trap: the .git/HEAD comment says "UnicodeDecodeError is a ValueError, which the original OSError guard missed", but the lesson stopped there. encoding="utf-8" on all twelve sites. The review named six; a grep found five more, including both ends of the HTTP cache round-trip in sources.py and the JSON artifact write, all of which carry registry text where non-ASCII is routine. A thirteenth turned up only when the new test ran: subprocess(text=True) decodes with the locale encoding too, so gh_token and the pip-audit call needed it as well. _read_json and _read_toml now catch UnicodeDecodeError explicitly. JSON is UTF-8 by RFC 8259 and TOML by its own spec, so a file that will not decode is malformed input and earns the same refusal as bad syntax rather than a traceback. Two tests, deliberately paired. The behavioural one runs the real collector paths in a subprocess under -X warn_default_encoding, where CPython raises EncodingWarning on any omitted encoding= (PEP 597) — that is what caught the subprocess sites, which no grep of mine had found. The static one covers what the driver cannot reach, render.py's two sites among them. Reverting any of the four representative sites fails one of them; reverting the UnicodeDecodeError guard fails the third. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * supply-chain-risk-auditor: make the encoding guard actually guard Two P2s from review on #284. The static half of the encoding check could not fail. text=True lines went into `offenders` with a "check encoding= nearby" marker, then the assertion filtered exactly those out and nothing ever looked at the soft list — so deleting encoding= from either subprocess call passed both halves of the pair. Confirmed by doing it: four green tests with the regression present. That is the failure mode AGENTS.md calls the most expensive class of bug here, written into the very test meant to prevent it. Rewritten with ast instead of line matching, so a call is seen whole however it is wrapped. Only the builtin open() is flagged, not x.open(): the first version flagged sources.py's urllib self.opener.open(request), where an encoding would be nonsense. Binary modes are skipped. Modules are globbed rather than listed, so a new one in scripts/ is covered when it lands, and the discovery asserts it found something. test_the_encoding_check_detects_a_missing_encoding pins both shapes the checker must catch, since its predecessor passed while a real regression was live. Second: the plain-text readers had no UnicodeDecodeError guard, unlike the JSON and TOML readers the last commit fixed, and main() catches only ReconciliationError. PowerShell 5.1 redirection writes UTF-16LE with a BOM, so a requirements file generated with `>` on a stock Windows box starts with two bytes that are invalid UTF-8 and the run died with a raw traceback — on the platform this PR exists to support. requirements*.txt and both go.mod reads now go through _read_text, which refuses with a message naming the likely cause. Three tests, one per site, each verified to fail when its own site is reverted. The third earns its place: parse_go short-circuits at the first go.mod read, so reverting _go_indirect's read alone left all 103 tests green, and the static check cannot see it because what is missing there is the guard, not the encoding. The fixture needed correcting mid-way: encode("utf-16-le") produces valid UTF-8 — nulls are legal — so it decoded without error and the test did not reproduce anything. encode("utf-16") carries the 0xFF 0xFE BOM the review named, which is what actually fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Eric Quintero
|
e6066e7db1 |
Rebuild supply-chain-risk-auditor around a deterministic collector (#227)
* Rebuild supply-chain-risk-auditor around a deterministic collector Replace the gh-only audit method with two stdlib-only Python scripts bundled with the skill: collect.py queries OSV, the npm and PyPI registries, the Go module proxy, deps.dev, OpenSSF Scorecard, and GitHub, and emits a JSON artifact; render.py turns it into a Markdown report of facts only. The model's job is the judgment layer on top — remediation, replacement candidates, narrative — written in report register and labeled as judgment. The old method could not deliver its own criteria: repository contributors are not registry publish rights, and gh sees no download counts or ecosystem-keyed advisories. What it measures, for npm, PyPI, and Go: - Version-matched advisories for direct dependencies, resolved from the lockfile, manifest pins, or labeled fallbacks — and for the full lockfile-resolved transitive tree (package-lock.json, uv.lock, go 1.17+ go.mod), advisories only. - Abandoned or archived upstreams, deprecated and yanked releases, npm publisher concentration, install-time script execution, and the two OpenSSF Scorecard checks that name a concrete mechanism (Dangerous-Workflow, Binary-Artifacts). Download volume, publish provenance, and security policy are reported as context, never flagged. The structure enforces its honesty rules rather than documenting them: - Every criterion resolves to assessed-clean, assessed-flagged, or unassessable-with-a-reason. Unavailable data is never evidence of risk, and every claim is bounded by a coverage table. - An empty advisory answer counts as clean only for a package proven to exist: a registry document for npm and PyPI, a module-proxy answer for Go, and for transitive lockfile entries a registry integrity hash or registry source. Everything else — private registries, git dependencies, vendored directories — is named as unverifiable with its reason, never counted clean. - Coverage must reconcile, a run that measures nothing exits non-zero instead of reporting that nothing is wrong, the renderer refuses an artifact whose flags and coverage disagree, and third-party text is escaped before it reaches a Markdown table. 85 offline tests exercise the invariants through the collector's own cache format, and the suite is mutation-checked. evals/ ships three fixtures with graded expectations; against a no-skill baseline the skill passed ~92% of skill-agnostic assertions vs ~60%, at half the wall clock, with its edge in reproducibility — the report regenerates byte-for-byte from the artifact — and self-consistency. Version 1.0.1 -> 2.0.0: method replacement. CODEOWNERS moves to @e-q. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Establish registry identity at the parse boundary; reconcile the sweep against the raw lockfile Review findings on the PR clustered at one boundary: a non-registry or malformed identity reaching the registry-keyed pipeline. Close the class, not the instances: - Dependency gains non_registry_reason, set by the parsers (npm file:/ workspace:/git/shorthand specs; PyPI direct deps whose uv.lock source is git/directory/path). One choke point in collect() marks every criterion unassessable with that reason and excludes such deps from all lookups — a same-named public package's advisories, publishers, and deprecation belong to code the project never installs. The deps stay in the report and its coverage. - Versions extracted from requirements text pass a PEP 440-shaped gate; pip-compile continuation/hash debris ("2.19.0 \") becomes unresolved instead of a version-matched claim. Measured: OSV compares garbage versions lexically, so the debris did not fail — it matched the wrong advisory ranges. - The transitive sweep excludes direct dependencies by (ecosystem, name, version), never by name: a nested copy of a direct dependency pinned at another version is this sweep's responsibility, and the name-keyed exclusion silently dropped it (measured on axios: two recovered entries, 620 -> 622). The artifact now carries a ledger counted from the raw lockfile before any exclusion — checked + unverifiable + excluded_direct must equal it, so a dropped triple fails validation instead of vanishing while the counts balance. The checked==0 guard hole is closed: zero reconciles like any number unless a reason is stated. - pip-audit runs with --no-deps --disable-pip. Measured: --no-deps alone still audited a pip-resolved transitive set, so pip was still fetching and potentially building untrusted distributions, against the tool's no-execution promise; with both flags it audits exactly the listed pins from registry metadata. Names on both sides of the cross-check are PEP 503-normalised. - Scorecard check thresholds move to model.SCORECARD_CHECKS as the single source of truth; the renderer derives its never-flags set from threshold-is-None, ending the clean-run contradiction that described the two flagging checks as "not flagged — poor precision". - Duplicate requirements prefer the runtime declaration (the dev file sorts first, so first-seen-wins reclassified production pins as build-time at the dev version); _git_commit confines refs to .git and degrades to None on undecodable content instead of crashing the run; recognised-but-unread lockfiles (yarn.lock, pnpm-lock.yaml, poetry.lock) produce a note and the docs name exactly which lockfiles are read; third-party text cannot inject links; the runtime estimate is honest. 88 tests; the four new guards (triple-keyed exclusion, non-registry choke, lockfile ledger, zero-checked reconciliation) are each mutation-checked against the exact reviewed bug. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Mark PEP 508 direct references as non-registry at the parse boundary `flask @ git+https://...` in pyproject.toml or requirements.txt was stripped to a bare name — `@` is a name terminator in _REQ_SPLIT, so the URL was silently discarded — and the dependency was looked up on PyPI, attributing the public package's advisories and metadata to a fork that may exist precisely to fix them. The npm path guards this at spec parsing and the PyPI path guarded it only via uv.lock's source table, so any pip-managed project walked past the choke point. Direct references are now detected in the requirement text itself and carry non_registry_reason with the URL; the existing choke point does the rest. Assert folded into the requirements parsing test and mutation-checked against the reproduction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Escape third-party text everywhere in the report, not only in tables The Method-and-caveats notes and the report header interpolated untrusted strings unescaped. Reproduced end to end: a package.json dependency key containing newlines (JSON permits them) with a file: spec produced a note that wrote a `## Summary` heading and a forged "No known advisory affects any of the 12 direct dependencies" bullet into report.md — a deliverable meant to survive being pasted into a client report. The 12-byte commit field read from the target's .git/HEAD had the same reach. Whitespace collapsing is the half that matters: it confines hostile text to the line it was interpolated into, where the worst available is inline emphasis rather than forged block structure. The mechanism was already right, so this is the missing calls plus a rename — _cell is now _safe_text, since a table-shaped name is what invited skipping it off-table. The existing table test asserted on the row it expected, which is why it never noticed these paths; the new test asserts structurally that every heading and bullet in the report came from the renderer. Both escaping calls are mutation-checked. Normal reports are byte-identical: the note templates carry no pipes or brackets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Accept v-prefixed and epoch versions; stop escaping inside code spans A regression review comparing the branch tip against its first commit found that two of the earlier review fixes had costs worth paying back. The PEP 440 version gate required a leading digit, so the legal pin `django==v3.2.0` became an unresolved version: advisories were matched against the latest release and 62 real ones for that version read as assessed_clean. PEP 440 permits the prefix, pip accepts it, and OSV matches it. The gate now accepts and strips it, so the reported version is canonical. Probing that also surfaced a longer-standing defect in the same path: extraction split on `!`, which is there for `!=` and truncated a PEP 440 epoch, so `1!2.0` was reported as the pin `1`. Extraction now ends the version at whitespace, a comma, or a semicolon, which additionally recovers the real pin from pip-compile hash lines that previously fell back to unresolved. Markdown does not process backslash escapes inside a code span, so escaping there wrote the backslashes out literally: the report title and the `Scanned:` path came out as `/tmp/pkg \[v2] \| beta`, which is not a path a reader can copy. Values inside backticks now go through _safe_code, which collapses whitespace and neutralises the one character that matters there — a backtick, which would close the span early — and leaves the rest alone. Prose and table cells keep _safe_text, so link forgery and cell escapes are unchanged. One first-party note lost its literal brackets rather than being escaped around them. Non-registry dependencies now share one unassessable reason, with the specific source in the signal value and the Method note. Embedding the source in the reason gave each dependency a unique string, which defeated the report's grouping: a 7-workspace-package fixture produced 91 near-identical bullets across 13 criteria, and the Not-assessable section went from 56 lines to 132. It is back to 56. Also: a collector-level fixture now proves the transitive ledger is sourced independently of the buckets it checks — deriving it from them made the equation true by construction and left a dropped package undetected while all tests passed. The docstring no longer claims the ledger is counted from the raw lockfile, which overstated its reach, and _locked_beyond_direct's return annotation matches its five values again. 91 tests; all four fixes mutation-checked. Real reports are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Keep the pipe escape for code spans inside table cells Splitting the escaping into prose and code-span variants dropped the pipe escape from both, but a GFM table row is split on pipes before inline spans are parsed, so a pipe inside a code span still ends the cell. A dependency named `evil|forged` put six boundaries in a five-column row: the name truncated to a bare backtick and `evil`, and every later value shifted one column right, so "none known" rendered under Other findings. Verified against a CommonMark+GFM parser that this is genuinely a third context rather than a reason to revert: inside a table cell `\|` renders as a literal pipe, while outside one the backslash survives into the output — which is the corrupted `Scanned:` path the split fixed. The three table paths now use _safe_code_cell; the bullet and header paths stay on _safe_code. The new test asserts column parity across every table in the document rather than one row in one table, counting the pipes GFM actually splits on so an escaped pipe reads as content. Per-path assertions are what let this reach three call sites at once, and what missed the notes path two commits earlier. Each of the three sites is mutation-checked independently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Check the assembled document instead of trusting every escape site Escaping was applied per interpolation site, and three successive commits each fixed one set of sites correctly while leaving siblings unguarded: the notes and header fields, then three table paths, and now the download-volume line and the informational sample. Coverage depended on whoever wrote or reviewed the diff noticing every interpolation, which is the wrong thing to rest a property on when the failure mode is a client deliverable carrying a forged all-clear. render() assembles a list of lines and joins them, and every legitimate line is appended as its own element, so two invariants are precise and cannot be violated by legitimate content: no assembled line contains a newline, and every table row carries its header's unescaped-pipe count. check_no_forged_lines enforces both immediately before the join. Block forgery needs a newline to open a new block, so the first invariant catches any site that leaks, including sites not yet written; verified by reverting each of the two newly-escaped sites, which now fails seven existing tests rather than none. The two open sites are escaped as well rather than left to the invariant: with escaping a hostile name renders harmlessly and the audit completes, while the invariant alone would let any audited repository deny its own audit by naming a dependency with a newline in it. The shared test fixture now carries a newline, a backtick, and a pipe in its default name, so every render test drives adversarial input through every path it touches. Its informational criteria carry real booleans too: they were ints, and informational_section sorts on `is True` / `is False`, so the path that interpolates names into a Without: sample had never executed in any test. That combination is why the misses kept recurring. 93 tests; both invariant loops mutation-checked. Real reports are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Lixin2026
|
d5fe2e6a78 |
feat(codex): add UI metadata for skills (#175)
* feat(codex): add skill UI metadata * Use official Trail of Bits logo * fix: resolve code review findings for PR #175 Codex silently drops the icons as authored: its loader (codex-rs/core-skills resolve_asset_path) requires icon paths containing '..' to resolve under <plugin_root>/assets/, and the repo-root .codex/assets location fails that containment check. Verified empirically via codex app-server plugin/read: every iconSmall/iconLarge came back null; only brand_color applied. P1 fixed: - Vendor trail-of-bits-mark.svg into plugins/<name>/assets/ for all 38 plugins with skills and point every openai.yaml at ../../assets/trail-of-bits-mark.svg (the supported plugin-level shared asset pattern). Icons now resolve for marketplace installs too, since nothing escapes the plugin root. - Drop the .codex/ additions: .codex/skills/gh-cli/agents/ openai.yaml resolved nowhere (.codex/skills is not a Codex discovery root) and PR #173 removes the whole .codex/ tree P2 fixed: - Patch-bump all 38 touched plugins in plugin.json and marketplace.json so installed clients pick up the metadata Verified: - Static check replicating Codex's resolution algorithm: all 73 yaml files resolve under their plugin assets/ and exist - Live codex app-server probe: 71/72 loadable skills report resolved iconSmall/iconLarge and brand_color #D83A34 (claude-in-chrome-troubleshooting fails to load on main due to a pre-existing 64-char qualified-name limit, fixed by #173's rename; zeroize-audit's manifest mcpServers object is likewise a pre-existing Codex incompatibility fixed by #173) - validate_codex_skills.py, validate_plugin_metadata.py, prek all pass Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(codex): use skill-local icon assets --------- Co-authored-by: Dan Guido <dan@trailofbits.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Jonathan Hefner
|
debfb29c8e |
Fix allowed-tools to use spec-compliant space-delimited strings (#139)
* Fix `allowed-tools` to use spec-compliant space-delimited strings Per the agentskills.io specification, `allowed-tools` must be a single string of space-delimited patterns, not a YAML list. Converted all 23 SKILL.md files from the `- Item` list format to the correct `"Item1 Item2"` string format. Also updated the frontmatter examples in CLAUDE.md and the workflow-skill-design skill template to match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix remaining allowed-tools format in firebase-apk-scanner and workflow-skill-design docs - Convert firebase-apk-scanner from comma-separated to space-delimited - Update anti-patterns.md and tool-assignment-guide.md examples from YAML lists to space-delimited strings - Remove unnecessary quotes from SKILL.md template placeholder Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Cover commands, new SKILL.md files, and fix template placeholder Extends the previous spec-compliance fixes: * Convert command frontmatter (commands/*.md) — per Claude Code docs, command files use the same frontmatter as skills, so the same space-delimited rule applies. * Convert three SKILL.md files added since the original PR: mutation-testing, trailmark-structural, trailmark-summary. * Fix the placeholder in the workflow-skill-design template. The previous "[minimum tools needed, space-delimited]" was YAML flow-sequence syntax, which parses as a list — the opposite of what the placeholder claims. Replaced with a concrete-looking space-delimited example plus a comment. Zeroize-audit agent files still use `allowed-tools:` in YAML list form. They are intentionally excluded: per the project's own docs (workflow-skill-design references), agents declare tools with `tools:` (not `allowed-tools:`). Fixing those requires changing the field name as well as the format and is out of scope for this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * zeroize-audit agents: switch allowed-tools to tools Subagents declare their tool allowlist via `tools:` (comma-separated), not `allowed-tools:` — see Claude Code's subagent docs and this repo's own designing-workflow-skills/SKILL.md:47: > Skills use `allowed-tools:` in frontmatter. Agents use `tools:` > in frontmatter. Before this change, the zeroize-audit agents declared their tool list under `allowed-tools:`, which Claude Code does not read for subagents. The field was effectively a no-op; the spawned agents had no tool restriction enforced. Renames the field on all 11 agents to `tools:` and reformats the YAML list as comma-separated to match the documented format and existing agents elsewhere in the repo (e.g. function-analyzer.md, spec-compliance-checker.md). Tool sets are unchanged. Behavior change: tools now actually constrain what each spawned agent can call. The lists are the ones the original author intended. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * skill-improver: convert command allowed-tools to space-delimited The two command files in plugins/skill-improver/commands/ still used the JSON flow-array format (`allowed-tools: ["..."]`), which the rest of this PR converted everywhere else. Convert them to the spec-compliant space-delimited string form for consistency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Dan Guido <dan@trailofbits.com> |
||
|
Dan Guido
|
ed41cd7ceb |
Import five plugins from skills-internal (#108)
* Import four plugins from skills-internal and clean up README Import seatbelt-sandboxer, supply-chain-risk-auditor, zeroize-audit, and let-fate-decide from skills-internal. Remove dead humanizer and skill-extractor directories. Fold "About Trail of Bits" into the license line. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix CI: shellcheck SC2317 and let-fate-decide description mismatch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Import agentic-actions-auditor from skills-internal Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix review issues across 5 imported plugins - Fix empty array expansion crash under set -u on macOS bash 3.2 (emit_rust_mir.sh, emit_asm.sh, emit_ir.sh) - Fix copy-paste error in seatbelt-sandboxer README ("variant analysis") - Remove references to non-existent leak-hunter skill - Update agentic-actions-auditor license to match repo CC-BY-SA-4.0 - Add PEP 723 metadata to check_rust_asm_{aarch64,x86}.py - Replace unsafe xargs trim with parameter expansion in track_dataflow.sh - Add json_escape function to analyze_asm.sh, analyze_heap.sh, track_dataflow.sh for safe JSON construction with backslashes - Add allowed-tools frontmatter to seatbelt-sandboxer and supply-chain-risk-auditor SKILL.md - Add minimum finding count assertions to run_smoke.sh - Fix double period typo in supply-chain-risk-auditor SKILL.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> |