diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 7e575d2..b65029b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -135,8 +135,13 @@ jobs: name: Create GitHub Release needs: [build-release] runs-on: ubuntu-latest + # Least privilege, scoped to this job only: + # contents: write -> create the GitHub Release and upload assets + # id-token: write -> mint the GitHub OIDC token cosign exchanges for a + # short-lived Sigstore (Fulcio) signing certificate (#591) permissions: contents: write + id-token: write steps: - uses: actions/checkout@v6 @@ -156,6 +161,36 @@ jobs: sha256sum * > checksums-sha256.txt cat checksums-sha256.txt + # Keyless signing (#591). Ordering matters: sign BEFORE creating the + # release. If signing fails the job stops here and no release is + # published, so we never ship a release that lacks its signature. + # One signature over the checksums file covers every archive, because + # install.sh / `cora upgrade` verify each archive against that file. + # sigstore/cosign-installer v3.10.1, pinned by commit SHA. + - name: Install cosign + uses: sigstore/cosign-installer@7e8b541eb2e61bf99390e1afd4be13a184e9ebc5 + with: + cosign-release: 'v2.6.5' + + - name: Sign checksums with cosign (keyless) + working-directory: release-artifacts + run: | + cosign sign-blob --yes \ + --bundle checksums-sha256.txt.sigstore.json \ + checksums-sha256.txt + test -s checksums-sha256.txt.sigstore.json + + # Fail the job (before publishing) if the bundle does not verify against + # this exact workflow identity. + - name: Verify the fresh signature + working-directory: release-artifacts + run: | + cosign verify-blob \ + --bundle checksums-sha256.txt.sigstore.json \ + --certificate-identity "https://github.com/${GITHUB_REPOSITORY}/.github/workflows/release.yml@${GITHUB_REF}" \ + --certificate-oidc-issuer https://token.actions.githubusercontent.com \ + checksums-sha256.txt + - name: Extract version from tag id: version run: | diff --git a/AGENT.md b/AGENT.md index b548e50..6edaa60 100644 --- a/AGENT.md +++ b/AGENT.md @@ -404,6 +404,7 @@ After release completes, verify: - [ ] GitHub Release page shows vX.Y.Z with correct changelog - [ ] 4 platform binaries attached to release - [ ] SHA256 checksums file included +- [ ] `checksums-sha256.txt.sigstore.json` asset exists and verifies (`cosign verify-blob` command in `docs/installation.md#verify-release-signatures`); `CORA_REQUIRE_SIGNATURE=1 CORA_VERSION=vX.Y.Z sh install.sh` succeeds - [ ] `crates.io` shows new version: `cargo search cora-code` - [ ] `codecora.dev` reflects new docs - [ ] Marketplace action still works (test on a test PR) diff --git a/CHANGELOG.md b/CHANGELOG.md index c30cede..e38d247 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.18.0] - 2026-10-09 + +### Security + +- **Release checksums are signed with cosign keyless and verified by `install.sh` and `cora upgrade` (#591).** `release.yml` now signs `checksums-sha256.txt` (which covers every archive) via Sigstore/GitHub OIDC before creating the release and uploads `checksums-sha256.txt.sigstore.json`; a signing failure aborts the job so no unsigned release is published. `install.sh` and `cora upgrade` verify the bundle with `cosign verify-blob` (identity: this repo's `release.yml` on a `v*` tag, issuer `token.actions.githubusercontent.com`) when `cosign` is on `PATH`; an invalid signature aborts. Without `cosign`, or for releases without a bundle, they print a notice and fall back to checksum-only. `CORA_REQUIRE_SIGNATURE=1` makes a missing `cosign` or bundle fatal. See `docs/installation.md`. + +### Changed + +- **Documented what the hardcoded-secret check flags and its one known limit** (all-letter unquoted YAML values), and closed that item as not planned (#625, #635). + +- **Scanners take file lines instead of diffs (#610).** New `engine::scan_input::ScanFile` (`path`, `language`, `(line number, text)` pairs) is the one input shape for `secrets_scanner::scan_secrets_in`, `security_scanner::scan_security_in`, `rules::run_rules_in` and `inline_suppress::apply_lines`. Diff chunks and whole files are thin adapters (`ScanFile::added_lines`, `ScanFile::post_image`, `ScanFile::from_entry`); `scan_secrets`/`scan_security`/`run_rules`/`inline_suppress::apply` keep their signatures. The `files_as_chunks` workaround and the MCP fake diff are gone. Findings are byte-identical for diffs and scans. Unit tests that open an index session now use a per-test temp database instead of the shared global one. + +### Fixed + +- **A `rules_engine:` section without `enabled: true` no longer disables the built-in rules.** `enabled` now defaults to `true` when the section is present, so `rules_engine: { max_findings: 0 }` only changes the cap; set `enabled: false` to turn the engine off (#638). + +- **`cora.check_snippet` (MCP) now honors `ignore.rules`, inline `cora-ignore:` markers and the false-positive filters, and `rules_engine.max_findings` (#610).** The tool used to run the secrets and security scanners on a fake diff with a fixed cap of 10 per scanner and no filtering, so a snippet could report findings that `cora scan` suppresses. It now goes through the shared post-processing pipeline as a synthetic `snippet.` file (so path-aware classification applies), loads the project config like the other MCP tools (and fails with the same "Failed to load config" error), and mentions how many findings the cap dropped. Output format is unchanged. + +- **Secret-detection edge cases (#625, #628, #629, #630).** A string literal inside a constructor wrapper is now a literal: Java `new String("..")`, `Cow::Owned`, `Box::from`, `Secret::new`, Python `str("..")` (plain calls such as `get_password()` stay quiet) (#630). A literal default in the default position of an env read is flagged in code files: `os.environ.get("X", "lit")`, `os.getenv`, `process.env.X || "lit"` / `?? "lit"`, Rust `env::var("X").unwrap_or("lit".into())` / `unwrap_or_else(|_| "lit".to_string())`, Ruby `ENV.fetch`, PHP `getenv() ?:`; the env var name and name-only reads stay quiet (#628). Shell `${VAR:-literal}` / `${VAR:=literal}` is flagged, while `${VAR}`, `${VAR:-}` and `${VAR:?msg}` stay quiet (#629). The unquoted-YAML rule no longer applies to source code (`secret: Secret1Type` in `.ts`/`.py`), YAML flow-style `{ password: hunter2hunter2 }` is flagged, unspaced `password:=os.Getenv("X")` is no longer matched, and SQL `PASSWORD "..."` with double quotes is detected (#625). All-alphabetic unquoted YAML values (`password: correcthorsebattery`) remain undetected, tracked in #635. The LLM secret cross-check uses the same classification. + +- **`rules_engine.max_findings` is applied after suppression and truncation is reported (#624).** The cap (default 5) used to be applied inside each scanner, before `ignore.rules` / `cora-ignore`, so suppressed findings consumed slots and pushed real findings out, and the rest were dropped silently. Scanners now run uncapped and the cap is enforced once in post-processing on deterministic findings only (LLM issues are never capped), highest severity first. `cora scan` and `cora review` print `⚠ N more deterministic findings not shown (...)` on stderr when findings are cut, and the `cora scan` summary mentions it; stdout formats are unchanged. `max_findings: 0` or `null` now means unlimited (previously `0` hid every finding). + ## [0.17.2] - 2026-10-09 ### Fixed @@ -986,7 +1008,8 @@ Benchmarked on the cora-code repository (1,864 symbols, 115 Rust files, x86_64): - **Cross-platform** — Linux (x86_64, ARM64), macOS (Apple Silicon), Windows (x86_64) - **MIT License** — fully open source -[Unreleased]: https://github.com/codecoradev/cora-code/compare/v0.17.2...develop +[Unreleased]: https://github.com/codecoradev/cora-code/compare/v0.18.0...develop +[0.18.0]: https://github.com/codecoradev/cora-code/compare/v0.17.2...v0.18.0 [0.17.2]: https://github.com/codecoradev/cora-code/compare/v0.17.1...v0.17.2 [0.17.1]: https://github.com/codecoradev/cora-code/compare/v0.17.0...v0.17.1 [0.17.0]: https://github.com/codecoradev/cora-code/compare/v0.16.1...v0.17.0 diff --git a/Cargo.lock b/Cargo.lock index 9287b14..91402f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -303,7 +303,7 @@ dependencies = [ [[package]] name = "cora-code" -version = "0.17.2" +version = "0.18.0" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index c6048c9..ea391c1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cora-code" -version = "0.17.2" +version = "0.18.0" edition = "2024" description = "CLI-first AI code review — BYOK, diff/scan/branch, pre-commit hooks" license = "Apache-2.0" diff --git a/README.md b/README.md index 63b0d9e..ad4cbd4 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,7 @@ cargo install --git https://github.com/codecoradev/cora-code > Pin a version: `CORA_VERSION=v0.6.1 curl -fsSL ... | sh` -**Upgrading:** run `cora upgrade` (downloads the latest release, verifies its SHA-256 checksum, replaces the binary) or `cora upgrade --check` to just see if one is available. If you installed via `cargo install --path .`, re-run that instead. +**Upgrading:** run `cora upgrade` (downloads the latest release, verifies its SHA-256 checksum and, when `cosign` is installed, its Sigstore signature, replaces the binary) or `cora upgrade --check` to just see if one is available. If you installed via `cargo install --path .`, re-run that instead. **Verify which `cora` you're running** — `which -a cora` will reveal stale copies from other channels: diff --git a/SECURITY.md b/SECURITY.md index 0a447df..196addf 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -45,6 +45,15 @@ Security-related areas of the codebase: - `src/hook/` — Pre-commit hook integration - `src/index/` — File access and SQLite storage +## Release Artifact Signing + +Release checksums are signed with cosign keyless (Sigstore, GitHub OIDC) by the +`release.yml` workflow, and `install.sh` / `cora upgrade` verify the signature +when `cosign` is installed (`CORA_REQUIRE_SIGNATURE=1` makes it mandatory). +Manual verification, the exact `cosign verify-blob` flags, and the scope and +limits of this guarantee are documented in +[docs/installation.md](docs/installation.md#verify-release-signatures). + ## Threat Model: Adversarial Source-Code Comments (ALIBI) LLM-based reviewers are vulnerable to adversarial comments in the code under diff --git a/docs/changelog.md b/docs/changelog.md index 4a28f1d..4ab4579 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,6 +7,146 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`rules_engine.max_findings` is applied after suppression and truncation is reported (#624).** The cap (default 5) used to be applied inside each scanner, before `ignore.rules` / `cora-ignore`, so suppressed findings consumed slots and pushed real findings out, and the rest were dropped silently. Scanners now run uncapped and the cap is enforced once in post-processing on deterministic findings only (LLM issues are never capped), highest severity first. `cora scan` and `cora review` print `⚠ N more deterministic findings not shown (...)` on stderr when findings are cut, and the `cora scan` summary mentions it; stdout formats are unchanged. `max_findings: 0` or `null` now means unlimited (previously `0` hid every finding). + +## [0.17.2] - 2026-10-09 + +### Fixed + +- **Env reads, call expressions and shell interpolation are no longer reported as hardcoded secrets.** `process.env.X`, `os.environ["X"]`, `os.getenv("X")`, `std::env::var("X")?`, `System.getenv("X")`, `get_password_from_vault()`, `String::new()` and bare identifiers/member accesses (`const password = userInput;`) are expressions, not literals, in source files (#616). `DB_PASSWORD=${VAR}`, `db.password=${DB_PASSWORD}`, `export X="$VAULT_X"` and `$(cmd)` are references (#617). Real values stay flagged: quoted or concatenated literals in code, and bare tokens in `.env`/`.properties`/`.ini`/`.toml`/shell/YAML files (`DB_PASSWORD=hunter2hunter2xx`). The LLM secret cross-check uses the same classification. + +## [0.17.1] - 2026-10-09 + +### Fixed + +- **Hardcoded secrets in Go short declarations, YAML/JSON keys and SQL `PASSWORD` literals are now detected.** Go `password := "..."` (with or without a trailing comment) and `var apiKey string = "..."` (#618); unquoted YAML `password: hunter2hunter2` and JSON quoted keys `"password": "..."` / `"apiKey": "..."` (#619); SQL `CREATE USER app WITH PASSWORD ''` and `IDENTIFIED BY ''` (#620). References stay quiet: `${VAR}`/`$VAR`, `!tag`, `*alias`, `&anchor`, paths, `null`/`required`-style bare words, and SQL placeholders (`PASSWORD '%s'`). The LLM secret cross-check uses the same pattern, so LLM findings on these shapes are no longer dropped. + +### Changed + +- **One shared post-processing module for `cora review` and `cora scan`.** The finding filter pipeline (LLM secret false-positive cross-check, Markdown code blocks, `ignore.rules`, inline `cora-ignore:`, context-line filter, scanner merge for scans) now lives in `engine::postprocess`, so a fix lands in one place. Internal refactor with no CLI or output change (#610). + +- **`ReviewIssue::new` constructor and table-driven hardcoded-secret tests.** `ReviewIssue` is now built through `ReviewIssue::new(..)` plus `with_*` setters, so adding a field touches one place instead of every construction site. One end-to-end table covers the hardcoded-secret heuristics across languages; known false positives/negatives are pinned in a separate table. Internal change with no CLI or output change (#610). + +## [0.17.0] - 2026-10-09 + +### Fixed + +- **A `cora-ignore:` naming a scanner rule could hide an unrelated LLM finding on the same line.** A scanner finding is now merged into an LLM issue on the same line only when they share a topic word (the secret family — password, key, token, credential — counts as one topic); otherwise both are reported and the marker suppresses only the finding it names (#609). + +### Fixed + +- **Real secrets in typed declarations were missed.** `const password: string = "..."` (TypeScript), `password: str = "..."` (Python) and `let password: &str = "..."` (Rust) matched none of the secret patterns (they required the value right after `password =`/`password:`), and `cora review` also dropped the LLM's finding on them (and on single-quoted literals) as a false positive. The scanner, the built-in rule and the LLM cross-check now allow an optional type annotation, and the cross-check accepts single-quoted literals. The `bare identifier` check in the object-shorthand filter, which was always true, now rejects values containing a string literal (#607). + +### Fixed + +- **Real secrets were missed when the line had a trailing comment containing `:`.** The `hardcoded-secret` false-positive filter treated the first `:` anywhere (even inside a comment or string) as object shorthand, so `password = "..."; // note: fix later` was not flagged, and no scanner finding existed on lines carrying a `cora-ignore: ` marker. The filter now judges only the code, ignoring trailing comments and quoted colons (#603). +- **Concurrent index opens could fail with `UNIQUE constraint failed: schema_version.version`.** Schema migrations are now serialised per process, which also removes a flaky test (#604). + +### Added + +- **Findings now carry a `rule_id`.** Deterministic scanner findings (rules, secrets, security, index) keep their rule id through the merge; it appears in pretty/compact output, as `rule_id` in JSON (omitted when absent, so older JSON still deserializes), and as `properties.coraRuleId` in SARIF (#597). + +### Changed + +- **`cora-ignore:` and `ignore.rules` match rule ids.** `cora-ignore: sec-hardcoded-secret` suppresses by id (exact, case-insensitive) as well as title; `ignore.rules` additionally matches ids exactly. A marker naming a scanner rule now also suppresses the LLM finding that displaced that scanner finding on the same line (#597). + +## [0.16.1] - 2026-10-08 + +### Fixed + +- **`cora scan` no longer reports "No issues found" when the LLM fails.** It now runs the deterministic secrets and security scanners on every scanned file, merges them with LLM findings, and applies `ignore.rules` and inline `cora-ignore:` markers (read from file contents), so a hardcoded secret is reported even on an LLM 502 (#595). +- **The LLM no longer flags `cora-ignore:` markers themselves** (e.g. "Security scanner findings suppressed instead of remediated"). The hardened system prompt now explains the marker is Cora's own suppression syntax (#596). + +## [0.16.0] - 2026-10-08 + +### Added + +- **Rule-scoped inline suppression with `cora-ignore:`.** A source comment such as `// cora-ignore: Hardcoded password or secret in variable` (also `#`, `--`, `/* */`, ``; comma-separated rules) suppresses findings with that exact title (case-insensitive) on the same line, or on the next line when the marker line holds only a comment. Applies to both static-scanner and LLM findings; other rules and other lines stay visible, and a bare `cora-ignore` without rules suppresses nothing. Coexists with `ignore.rules` / `ignore.files` (#554). + +### Security + +- **Project `.cora.yaml` can no longer redirect your API key.** `provider.base_url` from a discovered project config is ignored unless `CORA_TRUST_PROJECT_CONFIG=1`; `base_url` must be `https` (plain `http` only for loopback); LLM error bodies echoed to the terminal are length-capped (#563). +- **Hardened `cora upgrade` and `install.sh`.** Checksums are matched by exact filename; downloads go to a random 0700 temp dir; only the single `cora` binary entry is extracted (symlink/hardlink entries are rejected); every request has a timeout and a size cap; the version probe does not follow redirects and the tag is validated. `CORA_UPGRADE_SKIP_CHECKSUM` now also requires `CORA_UPGRADE_I_UNDERSTAND=1`. `install.sh` fails closed when the checksums file or entry is missing (opt out with `CORA_SKIP_CHECKSUM=1`) and falls back to `shasum -a 256` (#572). +- **`install.sh` requests now have timeouts and size caps.** Every `curl` call sets `--connect-timeout`, `--max-time` and `--max-filesize` (256 MiB archive, 1 MiB checksums/API JSON), matching `cora upgrade`, so a stalled or hostile server cannot hang or flood the installer (#580). +- **Hardened LLM response handling.** Review and scan system prompts tell the model to treat diff content as untrusted data; the diff fence is longer than any backtick run in the diff; the SSE stream errors on a line over 1 MiB or more than 16 MiB of content (#573). +- **Secrets in test and doc files are no longer invisible to the static security scanner.** Test and doc paths still skip the noisy general rules but now run high-confidence checks (AWS keys, private-key headers, GitHub/Slack/Stripe live tokens; values containing `EXAMPLE` are ignored) (#573). + +### Fixed + +- **`cora watch` reindexed every source file on each cycle.** Change detection now reindexes only files that changed, and `--filter` restricts what is indexed instead of only gating the trigger (#578, #584). +- **Secrets in test, fixture and example files were skipped entirely by the secrets scanner.** High-confidence secrets are now reported in those paths too (#579, #583). +- **`cora findings list --severity` never matched, and severity colouring never applied.** Both are now case-insensitive (#586, #588). +- **Unit tests wrote to the real data directory and could block on the global vector index lock.** Tests are isolated from the real data dir, and the lock wait is bounded (#587, #589). +- **`cora affected` never matched naming-convention tests.** The CLI took the file extension (`rs`) as the file stem. CLI and MCP now share one query, with escaped `LIKE` wildcards, batched queries and one deduplicated pattern list (`{stem}_test`, `test_{stem}`, `{stem}.test`, `{stem}.spec`, `tests/{stem}`, `__tests__/{stem}`). MCP dead-code now honors `analysis.entry_point_patterns` like the CLI (#575). +- **`cora --config serve` ignored `--config`.** The global option is now passed through (#576). +- **MCP `brain_search` ignored the configured embedding backend.** It now resolves the backend from the project config (#576). +- **Review and indexing disagreed about which files to skip.** Review-time scanners now use the same patterns as the indexer (`ignore.files` + `index.skip_files`) (#576). +- **Review scanners could resolve a different project than indexing** when run from a subdirectory or workspace member. Every entry point now resolves the project root the same way, and review no longer creates an empty `cora.db` when no index exists (#574). +- **Files from different projects overwrote each other's fingerprints** (perpetual reindex). Files are keyed by `(project_id, path)` (schema v8). `callers`/`callees`/`trace` match names exactly instead of by substring (`run` no longer matches `rerun`), and index runs prune stale files and their edges (#565). +- **MCP server robustness.** Stdin is framed as bytes and decoded as UTF-8; garbage input yields a `-32700` parse error; notifications get no response and `notifications/cancelled` no longer stops the server; `tools/call` without a name returns `-32602`; `limit`/`depth`/`min_lines` are clamped; `cora.install` requires `confirm: true` (#564). +- **Chunked review printed "No issues found" while reporting issues** when chunks returned empty summaries (#562). +- **CI:** the CLA check works for fork PRs (#553); `rustls` pinned to 0.23.45 for RUSTSEC-2026-0285 and a deprecated `f32` import removed so clippy passes on rustc 1.99 (#561). + +### Changed + +- **Ignore patterns now use a single matcher** for the index walk, review scanners, `cora scan --include/--exclude` and `watch --filter`. Behavior changes you may notice in `ignore.files` and related options: + - `**/*.test.ts` no longer matches `footest.ts`, and `vite.config.*` no longer matches `vite.configx`. + - Patterns with a wildcard in the middle (`src/*.rs`, `*.gen.*`) now match; they were silently ignored before, so files you thought were excluded may now actually be excluded. + - Patterns without a `/` also match by basename, in `cora scan --include/--exclude` and `watch --filter` too. + - An invalid glob matches literally instead of being dropped (#577). +- **`cora index --watch` now runs the same watcher as `cora watch`.** It checks every 500 ms (was 2 s), skips hidden directories, and prints `Reindexed: ...` (#576). +- **Internal refactors with no CLI change:** one seam for opening the index (`IndexBridge`, #574), one index-session module (#576), and review split into a deterministic stage testable without an LLM (#577), LLM parse/repair/retry policy unified behind one `Transport` seam (#582), and review-history SQL owned by a single `review_store` module (#585). + +## [0.15.0] - 2026-08-31 + +### Added + +- **Opt-in `vecq` vector store for Brain Mode.** Set `brain.vector_store: vecq` in `.cora.yaml` to replace the usearch HNSW index with a vecq quantized scan (pure Rust, deterministic, ~5x smaller). Keyed persistence included: symbol ids survive reload, so a fresh process serves the index as-is and `cora index` no longer re-embeds unchanged projects (#542, #547). +- **`brain.vector_bits` quantization-width knob.** `residual` (default) | `4` | `5` | `6` — 4-bit base codes with second-pass residual rescoring, or plain Lloyd-Max widths. The default is residual: best recall@10 at 4-bit scan speed in a recall study on cora's own embeddings, ahead of plain 5-bit at 1k/5k/13k-symbol scales. Changing the width rebuilds the index once on the next `cora index` instead of silently serving the old width; unknown values fall back to `residual`. + +### Fixed + +- **Vector signal never fired in a fresh process.** `cora brain` and MCP `brain_search` only saw the vector index if the same process had run the embed — otherwise results silently degraded to FTS-only. The search path now lazy-loads the on-disk index once per process, with a dimension guard against backend switches (#545). +- **Stale embed fingerprints after a global vector-index rebuild.** The vector index is a single file shared by all projects; rebuilding it (width/dims change, legacy file, corruption) wiped every project's vectors while their fingerprints still said "embedded" — the incremental path would skip those symbols forever. A rebuild now clears fingerprints for all projects, and the usearch dims-mismatch path (which deleted the index without clearing) joins the same heal. + +### Changed + +- **vecq-core dependency 0.2.0 → 0.3.0.** Picks up the 4-bit+residual mode, plain 5/6-bit widths, runtime-detected AVX2 scoring, and file formats v1.3–v1.5 with the keyed-slot table. Pre-0.3.0 `.vecq` files carry no key table and rebuild once with a warning, then upgrade to the keyed format. + +## [0.14.0] - 2026-08-28 + +### Fixed + +- **Empty LLM responses from reasoning models.** Models like GLM can spend the entire `max_tokens` budget on chain-of-thought and return `content: ""` with `finish_reason: "length"`, which previously surfaced as a misleading `EOF while parsing` error. Cora now reads `finish_reason`/`reasoning_content`, automatically retries with a doubled budget (up to 32768), salvages JSON from reasoning text as a last resort, and reports an explicit "EMPTY response" error when nothing is recoverable (#536). + +- **Dead-code false positives from cross-crate method calls.** Method calls inside Rust `impl` blocks were never walked for call edges, and call targets stored raw AST text (`self.export_full`) that could not join against symbol names — 557 false positives on a 5-crate workspace (#519). +- **Index root mismatch between CLI and MCP.** Running `cora index` inside a workspace member crate created a separate project row from the one MCP resolved, so `index_status` reported 0 symbols despite a populated DB. Root resolution now prefers a `[workspace]` Cargo.toml and never climbs past a `.git` boundary (#522). +- **`ignore.files` was not honored by the index.** Skip patterns only invalidated fingerprints; matched files were still indexed and surfaced in dead-code/review findings. They are now excluded from indexing entirely (#521). + +### Changed + +- **Default `max_tokens` raised from 4096 to 8192** to give reasoning models headroom above their chain-of-thought (#536). + +### Changed + +- **Default `max_tokens` raised from 4096 to 8192** to give reasoning models headroom above their chain-of-thought (#536). +- **Dead-code now skips public API surface by default** (`pub`/`export` items) — new `--include-pub` flag and MCP `include_pub_api` parameter opt back in (#520). +- **Review prompts include enclosing control-flow scope.** Hunks touching branching constructs get the enclosing function from the post-image (120-line cap), plus an always-on guardrail against unverified reachability claims (#523). +- **Incremental re-index reports honestly.** No-op runs print "Index up to date" with stored totals instead of "Indexed 0 symbols"; MCP `index_status` carries a root-mismatch hint (#522). +- **Relicensed from MIT to Apache-2.0.** All 18 CodeCoraDev repositories now + standardize on Apache-2.0 for patent grant protection and open-core model + compatibility. Added CLA (Individual + Corporate) for contributor copyright + and patent grants. +- **Added Contributor License Agreement (CLA).** Individual and Corporate CLA + documents added (`CLA_INDIVIDUAL.md`, `CLA_CORPORATE.md`). CLA includes + SIAC arbitration, patent retaliation, moral rights acknowledgment, and + no-compensation clause. +- **Updated CONTRIBUTING.md.** Added Contribution Terms section with + no-compensation notice, CLA requirement, and Apache-2.0 license reference. +- **Updated README license badge** from MIT to Apache-2.0. + ## [0.13.0] ### Added @@ -850,7 +990,14 @@ Benchmarked on the cora-code repository (1,864 symbols, 115 Rust files, x86_64): - **Cross-platform** — Linux (x86_64, ARM64), macOS (Apple Silicon), Windows (x86_64) - **MIT License** — fully open source -[Unreleased]: https://github.com/codecoradev/cora-code/compare/v0.13.0...develop +[Unreleased]: https://github.com/codecoradev/cora-code/compare/v0.17.2...develop +[0.17.2]: https://github.com/codecoradev/cora-code/compare/v0.17.1...v0.17.2 +[0.17.1]: https://github.com/codecoradev/cora-code/compare/v0.17.0...v0.17.1 +[0.17.0]: https://github.com/codecoradev/cora-code/compare/v0.16.1...v0.17.0 +[0.16.1]: https://github.com/codecoradev/cora-code/compare/v0.16.0...v0.16.1 +[0.16.0]: https://github.com/codecoradev/cora-code/compare/v0.15.0...v0.16.0 +[0.15.0]: https://github.com/codecoradev/cora-code/compare/v0.14.0...v0.15.0 +[0.14.0]: https://github.com/codecoradev/cora-code/compare/v0.13.0...v0.14.0 [0.13.0]: https://github.com/codecoradev/cora-code/compare/v0.12.0...v0.13.0 [0.12.0]: https://github.com/codecoradev/cora-code/compare/v0.11.1...v0.12.0 [0.11.1]: https://github.com/codecoradev/cora-code/compare/v0.11.0...v0.11.1 diff --git a/docs/configuration.md b/docs/configuration.md index 4a314b6..759a934 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -371,6 +371,14 @@ cora runs a static security scan on added lines before the AI review. 11 built-i Test files are automatically skipped. Findings are injected into the LLM prompt as additional context. +#### What the hardcoded-secret check does (and does not) flag + +The check looks for a secret-named key (`password`, `secret`, `token`, `api_key`, ...) assigned a **literal** value, and ignores values that are references: + +- **Flagged:** quoted literals (`password = "..."`, typed declarations, Go `:=`, JSON `"password": "..."`), SQL `PASSWORD '...'`, literal defaults inside env reads (`os.getenv("X", "literal")`, `process.env.X || "literal"`), shell `${VAR:-literal}`, bare values in `.env` / `.properties` / `.ini` / shell / YAML files when they look like a secret. +- **Not flagged:** env reads and other expressions (`process.env.X`, `os.getenv("X")`), identifiers and member accesses, `${VAR}` / `$VAR` interpolation, empty strings, UI bindings, placeholders such as `%s` / `$1` / `{{ x }}`. +- **Known limit:** an unquoted YAML value made only of letters (`password: correcthorsebattery`) is not flagged. It cannot be told apart from a reference to another key (`secret: kubernetes`, `token: optional`) without flooding Kubernetes/Compose files with false positives. Quote the value, or give it a digit or symbol, and it is detected. Use `cora-ignore: ` (see below) for a finding you accept. + ## Language-Specific Analyzers cora detects the languages in your diff and injects tailored review guidance: @@ -453,9 +461,18 @@ rules_engine: | Field | Type | Default | Description | |-------|------|---------|-------------| | `enabled` | `bool` | `true` | Enable/disable the rule engine | -| `max_findings` | `int` | `5` | Max findings per scan | +| `max_findings` | `int` | `5` | Max deterministic findings shown per run (`0` or `null` = unlimited). See below | | `index_skip_files` | `[string]` | *(see below)* | Glob patterns for files to skip during index scanning | +#### What `max_findings` caps + +`max_findings` caps the **deterministic** findings only (rule engine, secrets, security, and index scanners; the findings that carry a `rule_id`). LLM issues are never capped. + +- The cap is applied **after** suppression (`ignore.rules` and inline `cora-ignore:` markers), so suppressed findings never use up slots and hide real ones. +- Findings are kept highest severity first (Critical, Major, Minor, Info); ties keep their original order. Because the most severe findings survive, the cap never changes the blocking decision for a cap of 1 or more. +- When findings are cut, `cora scan` and `cora review` print one line on **stderr** (`⚠ N more deterministic findings not shown (rules.max_findings = 5; raise it or set 0/null to show all)`), and the `cora scan` summary mentions it. JSON, SARIF and compact output on stdout are unchanged. +- `0` or `null` means unlimited. For large diffs that `cora review` splits into chunks, the cap applies per chunk. + Default `index_skip_files` patterns (bundled with cora): - `*.config.ts`, `*.config.js`, `*.config.mjs` — bundler/build config files diff --git a/docs/installation.md b/docs/installation.md index cd07858..bbc0b28 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -88,6 +88,55 @@ $ echo 'source ~/.cora/completion.zsh' >> ~/.zshrc $ cora completion fish > ~/.config/fish/completions/cora.fish ``` +## Verify Release Signatures + +Every release (from the first signed release onward) publishes +`checksums-sha256.txt.sigstore.json`, a [Sigstore](https://www.sigstore.dev/) +bundle produced by the release workflow with **cosign keyless signing** (GitHub +OIDC, no long-lived key). One signature over `checksums-sha256.txt` covers every +archive, because each archive is verified against that file. + +Verify manually (requires [cosign](https://docs.sigstore.dev/cosign/system_config/installation/)): + +```bash +TAG=vX.Y.Z +BASE=https://github.com/codecoradev/cora-code/releases/download/$TAG +curl -fsSLO $BASE/checksums-sha256.txt +curl -fsSLO $BASE/checksums-sha256.txt.sigstore.json +cosign verify-blob \ + --bundle checksums-sha256.txt.sigstore.json \ + --certificate-identity-regexp '^https://github.com/codecoradev/cora-code/\.github/workflows/release\.yml@refs/tags/v' \ + --certificate-oidc-issuer https://token.actions.githubusercontent.com \ + checksums-sha256.txt +sha256sum -c --ignore-missing checksums-sha256.txt # shasum -a 256 -c on macOS +``` + +`install.sh` and `cora upgrade` perform the same check automatically when +`cosign` is on your `PATH`: + +| Situation | Default | `CORA_REQUIRE_SIGNATURE=1` | +|-----------|---------|----------------------------| +| Bundle present, `cosign` installed, signature valid | install | install | +| Bundle present, `cosign` installed, signature invalid | **abort** | **abort** | +| Bundle present, `cosign` not installed | install, prints a "checksum-only" notice | **abort** | +| No bundle published (release predates signing) | install, prints a notice | **abort** | +| Bundle download fails (not a 404) | **abort** | **abort** | + +`CORA_REQUIRE_SIGNATURE=1` also conflicts with `CORA_SKIP_CHECKSUM=1` +(`CORA_UPGRADE_SKIP_CHECKSUM` for `cora upgrade`): skipping the checksum file +would silently skip the signature, so the combination is rejected. + +**What this covers:** the checksums file (and therefore the archives) was +produced by this repository's `release.yml` workflow on a `v*` tag, and is +recorded in the Sigstore transparency log. A compromised release page or CDN +cannot forge it without also compromising the workflow. + +**What it does not cover:** a compromised repository or workflow itself, +releases published before signing was introduced, and installs where `cosign` +is absent and `CORA_REQUIRE_SIGNATURE` is not set (those fall back to +checksum-only, which protects against corruption but not a tampered release). +`cargo install` is verified by crates.io, not by this mechanism. + ## Verify Installation Confirm cora is installed correctly: diff --git a/docs/package.json b/docs/package.json index 2b14601..5efe4bd 100644 --- a/docs/package.json +++ b/docs/package.json @@ -4,7 +4,9 @@ "scripts": { "docs:dev": "vitepress dev", "docs:build": "vitepress build", - "docs:preview": "vitepress preview" + "docs:preview": "vitepress preview", + "predocs:build": "cp ../CHANGELOG.md changelog.md", + "predocs:dev": "cp ../CHANGELOG.md changelog.md" }, "devDependencies": { "@codecora/theme": "github:codecoradev/codecora-theme#c6a6aa654f686a8012a8442310222b8a6e879aa9", diff --git a/install.sh b/install.sh index c83ac67..e7b6542 100755 --- a/install.sh +++ b/install.sh @@ -89,6 +89,63 @@ get_target() { esac } +# Keyless cosign (Sigstore) identity that must have signed the release (#591). +COSIGN_IDENTITY_REGEXP="^https://github.com/${REPO}/\\.github/workflows/release\\.yml@refs/tags/v" +COSIGN_OIDC_ISSUER="https://token.actions.githubusercontent.com" + +# Is CORA_REQUIRE_SIGNATURE on? Sets REQUIRE_SIG=1|0. Unknown values are an +# error rather than a silent "off" so a requested verification is never lost. +parse_require_signature() { + case "${CORA_REQUIRE_SIGNATURE:-}" in + ""|0|false) REQUIRE_SIG=0;; + 1|true) REQUIRE_SIG=1;; + *) error "CORA_REQUIRE_SIGNATURE must be 1 or 0 (got '${CORA_REQUIRE_SIGNATURE}')";; + esac +} + +# Verify the cosign keyless signature over the checksums file. +# Policy (see docs/installation.md): +# bundle present + cosign present -> verify; failure is fatal (fail closed) +# cosign missing -> notice (checksum-only); fatal if REQUIRE_SIG=1 +# bundle absent (HTTP 404) -> notice (older release); fatal if REQUIRE_SIG=1 +# any other bundle download error -> fatal +verify_signature() { + BUNDLE_URL="${CHECKSUMS_URL}.sigstore.json" + BUNDLE_FILE="${CHECKSUM_FILE}.sigstore.json" + + HTTP_CODE=$(curl -sSL --connect-timeout 10 --max-time 30 --max-filesize 1048576 \ + -w '%{http_code}' "$BUNDLE_URL" -o "$BUNDLE_FILE") || HTTP_CODE="000" + case "$HTTP_CODE" in + 200) ;; + 404) + if [ "$REQUIRE_SIG" = "1" ]; then + error "No signature bundle published for ${VERSION} but CORA_REQUIRE_SIGNATURE=1. Refusing to install." + fi + warn "No cosign signature published for ${VERSION}; verified by SHA-256 checksum only." + return 0 + ;; + *) error "Failed to download signature bundle (HTTP ${HTTP_CODE}). Refusing to install (set CORA_SKIP_CHECKSUM=1 to bypass all verification, unsafe).";; + esac + + if ! command -v cosign >/dev/null 2>&1; then + if [ "$REQUIRE_SIG" = "1" ]; then + error "cosign not found but CORA_REQUIRE_SIGNATURE=1. Install cosign: https://docs.sigstore.dev/cosign/system_config/installation/" + fi + warn "Signature NOT verified (checksum-only): cosign not found. Install it to verify: https://docs.sigstore.dev/cosign/system_config/installation/" + return 0 + fi + + info "Verifying cosign signature..." + if ! cosign verify-blob \ + --bundle "$BUNDLE_FILE" \ + --certificate-identity-regexp "$COSIGN_IDENTITY_REGEXP" \ + --certificate-oidc-issuer "$COSIGN_OIDC_ISSUER" \ + "$CHECKSUM_FILE" >/dev/null 2>&1; then + error "cosign signature verification FAILED for checksums of ${VERSION}. Refusing to install." + fi + info "Signature verified (cosign keyless, GitHub Actions release workflow)" +} + # Download and install install() { info "Detected: $OS $ARCH" @@ -111,13 +168,18 @@ install() { # Verify SHA256 checksum (prevents MITM / corrupted download). # Mandatory: a missing checksums file or entry is fatal unless the user # explicitly opts out with CORA_SKIP_CHECKSUM=1. + parse_require_signature if [ "${CORA_SKIP_CHECKSUM:-}" = "1" ]; then + if [ "$REQUIRE_SIG" = "1" ]; then + error "CORA_SKIP_CHECKSUM=1 conflicts with CORA_REQUIRE_SIGNATURE=1 (the signature covers the checksums file). Unset one." + fi warn "CORA_SKIP_CHECKSUM=1 set - checksum verification DISABLED. The binary is NOT verified." else info "Downloading checksums..." if ! curl -fsSL --connect-timeout 10 --max-time 30 --max-filesize 1048576 "$CHECKSUMS_URL" -o "$CHECKSUM_FILE"; then error "Failed to download checksums. Refusing to install an unverified binary (set CORA_SKIP_CHECKSUM=1 to override, unsafe)." fi + verify_signature info "Verifying SHA256 checksum..." # Exact filename match (optionally prefixed with '*' or './'). EXPECTED=$(awk -v n="$ARCHIVE_NAME" '{f=$2; sub(/^\*/, "", f); sub(/^\.\//, "", f); if (f == n) {print $1; exit}}' "$CHECKSUM_FILE") diff --git a/scripts/test_install_signature.sh b/scripts/test_install_signature.sh new file mode 100644 index 0000000..760267c --- /dev/null +++ b/scripts/test_install_signature.sh @@ -0,0 +1,78 @@ +#!/bin/sh +# Decision-matrix test for install.sh verify_signature() (#591). +# No network: uses a stub curl and a stub cosign. Usage: sh scripts/test_install_signature.sh +set -u +ROOT=$(cd "$(dirname "$0")/.." && pwd) +T=$(mktemp -d) +trap 'rm -rf "$T"' EXIT +mkdir "$T/with" "$T/without" + +cat > "$T/with/curl" <<'STUB' +#!/bin/sh +out=""; w="" +while [ $# -gt 0 ]; do case "$1" in -o) out="$2"; shift;; -w) w="$2"; shift;; esac; shift; done +[ "$FAKE_CODE" = "ERR" ] && exit 28 +[ "$FAKE_CODE" = "200" ] && echo bundle > "$out" +[ -n "$w" ] && printf '%s' "$FAKE_CODE" +exit 0 +STUB +cat > "$T/with/cosign" <<'STUB' +#!/bin/sh +echo "$@" > "$COSIGN_ARGS" +exit "${FAKE_COSIGN_RC:-0}" +STUB +chmod +x "$T/with/curl" "$T/with/cosign" +cp "$T/with/curl" "$T/without/curl" +# install.sh without its trailing `main` call +sed '$d' "$ROOT/install.sh" > "$T/lib.sh" + +FAIL=0 +# check +check() { + name=$1; code=$2; has=$3; crc=$4; req=$5; want=$6; sub=$7 + if [ "$has" = 1 ]; then P="$T/with"; else P="$T/without"; fi + out=$( ( + FAKE_CODE=$code FAKE_COSIGN_RC=$crc CORA_REQUIRE_SIGNATURE=$req COSIGN_ARGS="$T/args" + export FAKE_CODE FAKE_COSIGN_RC CORA_REQUIRE_SIGNATURE COSIGN_ARGS + PATH="$P:/usr/bin:/bin" + # shellcheck disable=SC1091 + . "$T/lib.sh" + VERSION=v9.9.9 + CHECKSUMS_URL=http://x/checksums-sha256.txt + CHECKSUM_FILE="$T/checksums-sha256.txt" + : > "$CHECKSUM_FILE" + parse_require_signature + verify_signature + ) 2>&1 ) + rc=$? + case "$out" in *"$sub"*) okout=1;; *) okout=0;; esac + if [ "$rc" = "$want" ] && [ "$okout" = 1 ]; then + echo "PASS $name" + else + echo "FAIL $name (rc=$rc want=$want) $out" + FAIL=1 + fi +} + +check "verified" 200 1 0 "" 0 "Signature verified" +check "cosign rejects -> fatal" 200 1 1 "" 1 "FAILED" +check "cosign missing -> notice" 200 0 0 "" 0 "NOT verified" +check "cosign missing + REQUIRE" 200 0 0 1 1 "cosign not found" +check "no bundle -> notice" 404 1 0 "" 0 "checksum only" +check "no bundle + REQUIRE" 404 1 0 1 1 "CORA_REQUIRE_SIGNATURE=1" +check "HTTP 500 -> fatal" 500 1 0 "" 1 "Failed to download signature" +check "network error -> fatal" ERR 1 0 "" 1 "Failed to download signature" +check "invalid env -> fatal" 200 1 0 yes 1 "must be 1 or 0" + +# Re-run the success case so $T/args holds the verified invocation, then check flags. +check "verified (args)" 200 1 0 "" 0 "Signature verified" +if grep -q -- "--certificate-oidc-issuer https://token.actions.githubusercontent.com" "$T/args" \ + && grep -q -- 'release\\.yml@refs/tags/v' "$T/args" \ + && grep -q -- "--bundle" "$T/args"; then + echo "PASS cosign args" +else + echo "FAIL cosign args: $(cat "$T/args")" + FAIL=1 +fi + +exit "$FAIL" diff --git a/src/commands/review.rs b/src/commands/review.rs index 7a76bbe..f8572d1 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -254,6 +254,8 @@ pub async fn execute_review( } }; + warn_dropped(response.dropped_findings, config, opts.quiet); + // 6. Filter by severity if specified let min_severity = if let Some(ref sev) = opts.severity { Severity::from_str_lossy(sev) @@ -505,6 +507,7 @@ async fn execute_chunked_review( let mut any_error = false; let mut any_success = false; let mut should_block = false; + let mut dropped_findings = 0usize; for chunk in &chunks { if !opts.quiet { @@ -565,6 +568,7 @@ async fn execute_chunked_review( )); } + dropped_findings += resp.dropped_findings; all_issues.extend(resp.issues); if resp.should_block { should_block = true; @@ -629,11 +633,14 @@ async fn execute_chunked_review( // Build merged response let merged_summary = merged_chunk_summary(&summaries, all_issues.len(), any_error); + warn_dropped(dropped_findings, config, opts.quiet); + let merged_response = ReviewResponse { issues: all_issues, summary: merged_summary, tokens_used: Some(total_tokens), should_block, + dropped_findings, }; // 6. Filter by severity if specified @@ -784,6 +791,16 @@ fn compute_exit_code( } } +/// Print the one-line "findings not shown" warning to stderr (#624). +fn warn_dropped(dropped: usize, config: &Config, quiet: bool) { + if quiet { + return; + } + if let Some(w) = crate::engine::postprocess::dropped_warning(dropped, config) { + eprintln!("{}", w.yellow()); + } +} + #[cfg(test)] mod tests { use super::*; @@ -803,6 +820,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block, + dropped_findings: 0, } } diff --git a/src/commands/scan.rs b/src/commands/scan.rs index b0e5d49..de17856 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -6,7 +6,7 @@ use tracing::debug; use crate::config::schema::Config; use crate::engine::ReviewIssue; -use crate::engine::postprocess::{Source, postprocess}; +use crate::engine::postprocess::{Source, dropped_warning, postprocess_report}; use crate::engine::review_store; use crate::engine::scanner::{FileEntry, batch_files, format_batch_for_prompt, walk_project}; use crate::engine::types::TokenUsage; @@ -127,7 +127,8 @@ pub async fn execute_scan( let index_findings = crate::engine::index_scanner::scan_project_index( &index_bridge, &files, - config.rules_config.max_findings, + // Uncapped: the cap is enforced after suppression in `postprocess` (#624). + usize::MAX, &index_skip, ); if !index_findings.is_empty() { @@ -253,7 +254,10 @@ pub async fn execute_scan( // apply ignore.rules and inline cora-ignore markers. Deterministic // findings survive an LLM failure (#595). all_issues.extend(index_findings); - let all_issues = finalize_issues(config, &files, all_issues); + let (all_issues, dropped) = finalize_issues(config, &files, all_issues); + if let Some(w) = dropped_warning(dropped, config) { + eprintln!("{}", w.yellow()); + } let issue_count = all_issues.len(); let min_severity = config.hook.min_severity_level(); // Ord order is Critical(0) < Major(1) < Minor(2) < Info(3), so "at or above @@ -265,10 +269,17 @@ pub async fn execute_scan( files_scanned: files.len(), lines_scanned: total_lines, summary: format!( - "Scanned {} files ({} lines), found {} issues.", + "Scanned {} files ({} lines), found {} issues{}.", files.len(), total_lines, - issue_count + issue_count, + if dropped > 0 { + format!( + " ({dropped} more deterministic findings not shown, rules.max_findings cap)" + ) + } else { + String::new() + } ), tokens_used: total_tokens, should_block, @@ -342,8 +353,9 @@ fn finalize_issues( config: &Config, files: &[FileEntry], issues: Vec, -) -> Vec { - postprocess(issues, &Source::Files(files), config) +) -> (Vec, usize) { + let report = postprocess_report(issues, &Source::Files(files), config); + (report.issues, report.dropped) } /// Compute a short SHA256 hash of a file's content for incremental scanning. @@ -428,7 +440,7 @@ mod tests { } fn secret_title() -> String { - let out = finalize_issues(&Config::default(), &[secret_file("")], Vec::new()); + let out = finalize_issues(&Config::default(), &[secret_file("")], Vec::new()).0; out.iter() .find(|i| i.line == Some(2)) .unwrap_or_else(|| panic!("no finding on line 2: {:?}", titles(&out))) @@ -438,7 +450,7 @@ mod tests { #[test] fn finds_hardcoded_secret_without_llm() { - let out = finalize_issues(&Config::default(), &[secret_file("")], Vec::new()); + let out = finalize_issues(&Config::default(), &[secret_file("")], Vec::new()).0; assert!( out.iter() .any(|i| i.file == "src/app.py" && i.line == Some(2)), @@ -451,7 +463,7 @@ mod tests { fn inline_marker_suppresses_secret() { let title = secret_title(); let file = secret_file(&format!(" # cora-ignore: {title}")); - let out = finalize_issues(&Config::default(), &[file], Vec::new()); + let out = finalize_issues(&Config::default(), &[file], Vec::new()).0; assert!( !out.iter().any(|i| i.line == Some(2)), "got {:?}", @@ -463,7 +475,7 @@ mod tests { fn ignore_rules_suppress_secret() { let mut config = Config::default(); config.ignore.rules = vec![secret_title()]; - let out = finalize_issues(&config, &[secret_file("")], Vec::new()); + let out = finalize_issues(&config, &[secret_file("")], Vec::new()).0; assert!( !out.iter().any(|i| i.line == Some(2)), "got {:?}", diff --git a/src/commands/upgrade.rs b/src/commands/upgrade.rs index 4694ff8..9a530b4 100644 --- a/src/commands/upgrade.rs +++ b/src/commands/upgrade.rs @@ -22,6 +22,8 @@ const MAX_ARCHIVE_BYTES: u64 = 256 * 1024 * 1024; const MAX_SMALL_BYTES: u64 = 1024 * 1024; /// Maximum accepted size for the extracted binary. const MAX_BINARY_BYTES: u64 = 512 * 1024 * 1024; +/// OIDC issuer that must appear in the signing certificate (GitHub Actions). +const COSIGN_OIDC_ISSUER: &str = "https://token.actions.githubusercontent.com"; const CONNECT_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(15); const REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(300); @@ -150,6 +152,22 @@ pub async fn run(yes: bool, check_only: bool) -> anyhow::Result { } }; + let require_sig = + match parse_require_signature(std::env::var("CORA_REQUIRE_SIGNATURE").ok().as_deref()) { + Ok(v) => v, + Err(msg) => { + eprintln!("{} {msg}", "[ERROR]".red()); + return Ok(1); + } + }; + if skip_checksum && require_sig { + eprintln!( + "{} CORA_UPGRADE_SKIP_CHECKSUM conflicts with CORA_REQUIRE_SIGNATURE=1 (the signature covers the checksums file). Unset one.", + "[ERROR]".red() + ); + return Ok(1); + } + if skip_checksum { eprintln!( "{} !!! CHECKSUM VERIFICATION DISABLED (CORA_UPGRADE_SKIP_CHECKSUM + CORA_UPGRADE_I_UNDERSTAND) !!!", @@ -160,8 +178,8 @@ pub async fn run(yes: bool, check_only: bool) -> anyhow::Result { "[WARN]".yellow().bold() ); } else { - let checksums_text = match download_async(&checksums_url, MAX_SMALL_BYTES).await { - Ok(b) => String::from_utf8_lossy(&b).to_string(), + let checksums_bytes = match download_async(&checksums_url, MAX_SMALL_BYTES).await { + Ok(b) => b, Err(e) => { eprintln!("{} Failed to download checksums: {e}", "[ERROR]".red()); eprintln!( @@ -171,6 +189,35 @@ pub async fn run(yes: bool, check_only: bool) -> anyhow::Result { } }; + let checksums_text = String::from_utf8_lossy(&checksums_bytes).to_string(); + + // Keyless cosign signature over the checksums file (#591). + let checksums_path = temp_dir.join("checksums-sha256.txt"); + fs::write(&checksums_path, &checksums_bytes) + .map_err(|e| anyhow::anyhow!("Failed to write checksums: {e}"))?; + let bundle_state = + match download_optional_async(&bundle_url(&latest_version), MAX_SMALL_BYTES).await { + Ok(Some(b)) => BundleState::Present(b), + Ok(None) => BundleState::Absent, + Err(e) => BundleState::Error(e.to_string()), + }; + match verify_signature_policy( + require_sig, + bundle_state, + &SystemCosign, + &temp_dir, + &checksums_path, + ) { + Ok(SigOutcome::Verified) => { + println!("{} Signature verified (cosign keyless)", "[INFO]".green()) + } + Ok(SigOutcome::Unverified(note)) => eprintln!("{} {note}", "[WARN]".yellow()), + Err(msg) => { + eprintln!("{} {msg}", "[ERROR]".red()); + return Ok(1); + } + } + let expected = match parse_checksum(&checksums_text, &archive_name) { Some(h) => h, None => { @@ -272,12 +319,22 @@ fn http_client(follow_redirects: bool) -> anyhow::Result { /// Download a URL with timeouts and a hard cap on the body size. async fn download_async(url: &str, max_bytes: u64) -> anyhow::Result> { + download_optional_async(url, max_bytes) + .await? + .ok_or_else(|| anyhow::anyhow!("HTTP 404 Not Found")) +} + +/// Like [`download_async`] but maps HTTP 404 to `Ok(None)`. +async fn download_optional_async(url: &str, max_bytes: u64) -> anyhow::Result>> { let mut resp = http_client(true)? .get(url) .send() .await .map_err(|e| anyhow::anyhow!("HTTP request failed: {e}"))?; + if resp.status() == reqwest::StatusCode::NOT_FOUND { + return Ok(None); + } if !resp.status().is_success() { anyhow::bail!("HTTP {}", resp.status()); } @@ -298,7 +355,130 @@ async fn download_async(url: &str, max_bytes: u64) -> anyhow::Result> { } buf.extend_from_slice(&chunk); } - Ok(buf) + Ok(Some(buf)) +} + +/// URL of the cosign bundle published next to the checksums file. +fn bundle_url(tag: &str) -> String { + format!("https://github.com/{REPO}/releases/download/{tag}/checksums-sha256.txt.sigstore.json") +} + +/// Regexp the signing certificate identity must match: the release workflow of +/// this repository, run from a `v*` tag. (`.` is escaped so only the real file matches.) +fn cosign_identity_regexp() -> String { + format!("^https://github.com/{REPO}/\\.github/workflows/release\\.yml@refs/tags/v") +} + +/// Parse `CORA_REQUIRE_SIGNATURE`. Unknown values are an error, never a silent "off". +fn parse_require_signature(v: Option<&str>) -> Result { + match v { + None | Some("") | Some("0") | Some("false") => Ok(false), + Some("1") | Some("true") => Ok(true), + Some(o) => Err(format!("CORA_REQUIRE_SIGNATURE must be 1 or 0 (got '{o}')")), + } +} + +/// Result of fetching the signature bundle. +enum BundleState { + Present(Vec), + /// HTTP 404: release predates signing. + Absent, + Error(String), +} + +#[derive(Debug, PartialEq, Eq)] +enum SigOutcome { + Verified, + /// Proceeding on checksum-only; the string is the notice to show. + Unverified(String), +} + +/// Seam over the external `cosign` binary so the policy is unit-testable. +trait CosignRunner { + fn available(&self) -> bool; + fn verify(&self, bundle: &std::path::Path, blob: &std::path::Path) -> Result<(), String>; +} + +struct SystemCosign; + +impl CosignRunner for SystemCosign { + fn available(&self) -> bool { + std::process::Command::new("cosign") + .arg("version") + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .is_ok() + } + + fn verify(&self, bundle: &std::path::Path, blob: &std::path::Path) -> Result<(), String> { + let out = std::process::Command::new("cosign") + .arg("verify-blob") + .arg("--bundle") + .arg(bundle) + .arg("--certificate-identity-regexp") + .arg(cosign_identity_regexp()) + .arg("--certificate-oidc-issuer") + .arg(COSIGN_OIDC_ISSUER) + .arg(blob) + .output() + .map_err(|e| format!("failed to run cosign: {e}"))?; + if out.status.success() { + Ok(()) + } else { + Err(String::from_utf8_lossy(&out.stderr).trim().to_string()) + } + } +} + +const COSIGN_INSTALL_HINT: &str = "https://docs.sigstore.dev/cosign/system_config/installation/"; + +/// Signature policy (same as install.sh): +/// bundle + cosign => verify, failure fatal; cosign missing or bundle absent => +/// notice, fatal when `require`; any other bundle fetch error => fatal. +fn verify_signature_policy( + require: bool, + bundle: BundleState, + runner: &dyn CosignRunner, + dir: &std::path::Path, + checksums_path: &std::path::Path, +) -> Result { + let bytes = match bundle { + BundleState::Present(b) => b, + BundleState::Absent if require => { + return Err( + "No signature bundle published for this release but CORA_REQUIRE_SIGNATURE=1. Refusing to upgrade." + .into(), + ) + } + BundleState::Absent => { + return Ok(SigOutcome::Unverified( + "No cosign signature published for this release; verified by SHA-256 checksum only." + .into(), + )) + } + BundleState::Error(e) => { + return Err(format!( + "Failed to download signature bundle: {e}. Refusing to upgrade." + )) + } + }; + if !runner.available() { + if require { + return Err(format!( + "cosign not found but CORA_REQUIRE_SIGNATURE=1. Install cosign: {COSIGN_INSTALL_HINT}" + )); + } + return Ok(SigOutcome::Unverified(format!( + "Signature NOT verified (checksum-only): cosign not found. Install it to verify: {COSIGN_INSTALL_HINT}" + ))); + } + let bundle_path = dir.join("checksums-sha256.txt.sigstore.json"); + fs::write(&bundle_path, bytes).map_err(|e| format!("Failed to write bundle: {e}"))?; + runner + .verify(&bundle_path, checksums_path) + .map(|()| SigOutcome::Verified) + .map_err(|e| format!("cosign signature verification FAILED: {e}. Refusing to upgrade.")) } /// Decide whether checksum verification may be skipped. @@ -606,4 +786,106 @@ mod tests { let a = write_archive(d.path(), &tgz); assert!(extract_binary(&a, out.path()).is_err()); } + + struct FakeCosign { + available: bool, + ok: bool, + calls: std::cell::Cell, + } + + impl CosignRunner for FakeCosign { + fn available(&self) -> bool { + self.available + } + fn verify(&self, bundle: &std::path::Path, _blob: &std::path::Path) -> Result<(), String> { + self.calls.set(self.calls.get() + 1); + assert!(bundle.exists()); + if self.ok { + Ok(()) + } else { + Err("bad sig".into()) + } + } + } + + fn policy( + require: bool, + b: BundleState, + available: bool, + ok: bool, + ) -> (Result, u32) { + let d = tempfile::tempdir().unwrap(); + let blob = d.path().join("checksums-sha256.txt"); + fs::write(&blob, b"x").unwrap(); + let f = FakeCosign { + available, + ok, + calls: std::cell::Cell::new(0), + }; + let r = verify_signature_policy(require, b, &f, d.path(), &blob); + (r, f.calls.get()) + } + + #[test] + fn signature_policy_matrix() { + let present = || BundleState::Present(b"{}".to_vec()); + assert_eq!( + policy(false, present(), true, true), + (Ok(SigOutcome::Verified), 1) + ); + assert_eq!( + policy(true, present(), true, true), + (Ok(SigOutcome::Verified), 1) + ); + // cosign rejects => fatal regardless of require + for req in [false, true] { + let (r, calls) = policy(req, present(), true, false); + assert!(r.unwrap_err().contains("FAILED")); + assert_eq!(calls, 1); + } + // cosign missing + let (r, calls) = policy(false, present(), false, true); + assert!(matches!(r, Ok(SigOutcome::Unverified(ref m)) if m.contains("cosign not found"))); + assert_eq!(calls, 0); + assert!(policy(true, present(), false, true).0.is_err()); + // bundle absent (older release) + assert!(matches!( + policy(false, BundleState::Absent, true, true).0, + Ok(SigOutcome::Unverified(_)) + )); + assert!(policy(true, BundleState::Absent, true, true).0.is_err()); + // other fetch errors are always fatal + for req in [false, true] { + assert!( + policy(req, BundleState::Error("HTTP 500".into()), true, true) + .0 + .is_err() + ); + } + } + + #[test] + fn require_signature_parsing() { + assert_eq!(parse_require_signature(None), Ok(false)); + assert_eq!(parse_require_signature(Some("0")), Ok(false)); + assert_eq!(parse_require_signature(Some("1")), Ok(true)); + assert_eq!(parse_require_signature(Some("true")), Ok(true)); + assert!(parse_require_signature(Some("yes")).is_err()); + } + + #[test] + fn signature_constants() { + assert_eq!( + bundle_url("v1.2.3"), + "https://github.com/codecoradev/cora-code/releases/download/v1.2.3/checksums-sha256.txt.sigstore.json" + ); + assert_eq!( + cosign_identity_regexp(), + "^https://github.com/codecoradev/cora-code/\\.github/workflows/release\\.yml@refs/tags/v" + ); + assert_eq!( + COSIGN_OIDC_ISSUER, + "https://token.actions.githubusercontent.com" + ); + } } diff --git a/src/commands/watch.rs b/src/commands/watch.rs index a68aecb..6e131e1 100644 --- a/src/commands/watch.rs +++ b/src/commands/watch.rs @@ -476,7 +476,14 @@ mod tests { fs::create_dir_all(root.join("src")).unwrap(); fs::write(root.join("src/a.rs"), "pub fn alpha() {}\n").unwrap(); fs::write(root.join("src/b.rs"), "pub fn beta() {}\n").unwrap(); - let session = IndexSession::open_at(root, ConfigSource::ProjectOnlyAt(root)).unwrap(); + // Per-test DB: never touches the shared global index. + let db_dir = TempDir::new().unwrap(); + let session = IndexSession::open_at_db( + &db_dir.path().join("cora.db"), + root, + ConfigSource::ProjectOnlyAt(root), + ) + .unwrap(); session.index(false).unwrap(); let pat = PathPattern::new("a.rs").unwrap(); let s = || scan_files(root, &None, Some(&pat), None).unwrap(); diff --git a/src/config/schema.rs b/src/config/schema.rs index 40c9d40..b9cb6da 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -609,17 +609,36 @@ pub struct LlmSection { pub cache_ttl: Option, } +fn default_rules_enabled() -> bool { + true +} + fn default_max_findings() -> usize { 5 } +/// `rules_engine.max_findings`: an explicit `null` means unlimited (stored as 0). +fn deserialize_max_findings<'de, D>(d: D) -> std::result::Result +where + D: serde::Deserializer<'de>, +{ + Ok(Option::::deserialize(d)?.unwrap_or(0)) +} + /// Rule engine configuration section for `.cora.yaml`. #[derive(Debug, Clone, Serialize, Deserialize, Default)] #[serde(deny_unknown_fields)] pub struct RulesSection { - #[serde(default, skip_serializing_if = "is_default")] + /// Built-in rule engine switch. Defaults to `true` (the same as having no + /// `rules_engine` section at all), so tuning another key such as + /// `max_findings` does not silently turn the built-in rules off (#638). + #[serde(default = "default_rules_enabled", skip_serializing_if = "is_true")] pub enabled: bool, - #[serde(default = "default_max_findings")] + /// Cap on deterministic findings shown per run. `0` or `null` = unlimited (#624). + #[serde( + default = "default_max_findings", + deserialize_with = "deserialize_max_findings" + )] pub max_findings: usize, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub custom: Vec, @@ -1124,6 +1143,32 @@ provider: zai assert_eq!(nm_count, 1, "duplicate entry should be deduplicated"); } + #[test] + fn rules_engine_section_without_enabled_keeps_rules_on() { + // #638: `rules_engine: { max_findings: 0 }` must not disable built-in rules. + let cora: CoraFile = serde_yaml_ng::from_str("rules_engine:\n max_findings: 0\n").unwrap(); + let mut cfg = Config::default(); + cora.merge_into(&mut cfg).unwrap(); + assert!(cfg.rules_config.enabled); + assert_eq!(cfg.rules_config.max_findings, 0); + } + + #[test] + fn rules_engine_enabled_false_still_disables_and_round_trips() { + let cora: CoraFile = + serde_yaml_ng::from_str("rules_engine:\n enabled: false\n max_findings: 7\n") + .unwrap(); + let mut cfg = Config::default(); + cora.merge_into(&mut cfg).unwrap(); + assert!(!cfg.rules_config.enabled); + let out = serde_yaml_ng::to_string(&cora).unwrap(); + let again: CoraFile = serde_yaml_ng::from_str(&out).unwrap(); + assert!( + !again.rules_engine.unwrap().enabled, + "false must survive a round trip" + ); + } + #[test] fn merge_index_skip_files_preserves_defaults() { let mut cfg = Config::default(); diff --git a/src/engine/cache.rs b/src/engine/cache.rs index 042e6cd..61a061d 100644 --- a/src/engine/cache.rs +++ b/src/engine/cache.rs @@ -195,6 +195,7 @@ mod tests { summary: "Found 1 issue.".to_string(), tokens_used: None, should_block: false, + dropped_findings: 0, } } diff --git a/src/engine/deterministic.rs b/src/engine/deterministic.rs index d75bf4e..90bfab1 100644 --- a/src/engine/deterministic.rs +++ b/src/engine/deterministic.rs @@ -23,6 +23,9 @@ use crate::engine::index_bridge::IndexBridge; use crate::engine::rules::{self, types::RuleFinding}; use crate::engine::{index_scanner, secrets_scanner, security_scanner}; +/// Max findings per family listed in the LLM prompt context. +const PROMPT_FINDINGS_PER_FAMILY: usize = 20; + /// Findings of every deterministic scanner family, in contract order. #[derive(Debug, Default)] pub struct DeterministicReport { @@ -45,11 +48,18 @@ pub fn skip_patterns(config: &Config) -> Vec { /// Run every deterministic check on a parsed diff. pub fn run(chunks: &[FileChunk], config: &Config, bridge: &IndexBridge) -> DeterministicReport { - let max = config.rules_config.max_findings; + // Scanners run uncapped: `rules_engine.max_findings` is enforced by + // `postprocess` AFTER suppression, so suppressed findings never consume + // slots and truncation can be reported (#624). + let max = usize::MAX; let skip = skip_patterns(config); + let uncapped_rules = crate::engine::rules::types::RulesConfig { + max_findings: 0, + ..config.rules_config.clone() + }; DeterministicReport { - rules: rules::run_rules(chunks, &config.rules_config), + rules: rules::run_rules(chunks, &uncapped_rules), secrets: secrets_scanner::scan_secrets(chunks, max), security: security_scanner::scan_security(chunks, max), index_unused: index_scanner::scan_unused_imports(bridge, chunks, max, &skip), @@ -95,8 +105,18 @@ impl DeterministicReport { parts.push(warning); } for family in self.families() { - let text = rules::format_rule_context(family); + // Scanners run uncapped (the user-facing cap is applied after + // suppression in `postprocess`), so bound what is sent to the model: + // families are severity-sorted, keep the worst PROMPT_FINDINGS_PER_FAMILY. + let shown = &family[..family.len().min(PROMPT_FINDINGS_PER_FAMILY)]; + let mut text = rules::format_rule_context(shown); if !text.is_empty() { + if family.len() > shown.len() { + text.push_str(&format!( + "({} more findings of this kind not listed here)\n", + family.len() - shown.len() + )); + } parts.push(text); } } @@ -207,6 +227,26 @@ diff --git a/src/api.js b/src/api.js ); } + #[test] + fn prompt_context_is_bounded_per_family() { + let report = DeterministicReport { + secrets: (1..=(PROMPT_FINDINGS_PER_FAMILY as u32 + 7)) + .map(|line| RuleFinding { + rule_id: "secrets/x".to_string(), + file: "a.rs".to_string(), + line, + severity: crate::engine::Severity::Critical, + title: "t".to_string(), + body: "b".to_string(), + }) + .collect(), + ..Default::default() + }; + let ctx = report.context(None).expect("context"); + assert_eq!(ctx.matches("secrets/x").count(), PROMPT_FINDINGS_PER_FAMILY); + assert!(ctx.contains("(7 more findings of this kind not listed here)")); + } + #[test] fn context_is_ordered_and_optional() { let chunks = parse_diff(DIFF); diff --git a/src/engine/index_bridge.rs b/src/engine/index_bridge.rs index 934abb8..1e293cf 100644 --- a/src/engine/index_bridge.rs +++ b/src/engine/index_bridge.rs @@ -108,6 +108,14 @@ impl IndexBridge { Self::from_connection(conn, start) } + /// [`Self::open_or_create`] against an explicit database file instead of + /// the process-global data dir (tests: a per-test temp DB, #610). + #[cfg(test)] + pub fn open_or_create_at(db_path: &Path, start: &Path) -> anyhow::Result { + let conn = crate::index::open_index_at(db_path)?; + Self::from_connection(conn, start) + } + /// [`Self::open_or_create`] for the current working directory. pub fn open_or_create_cwd() -> anyhow::Result { Self::open_or_create(&std::env::current_dir()?) diff --git a/src/engine/inline_suppress.rs b/src/engine/inline_suppress.rs index a7b46ee..a98b00f 100644 --- a/src/engine/inline_suppress.rs +++ b/src/engine/inline_suppress.rs @@ -20,7 +20,8 @@ //! the diff (more than the context window away from the finding) is not seen. use crate::engine::ReviewIssue; -use crate::engine::diff_parser::{DiffLineType, FileChunk}; +use crate::engine::diff_parser::FileChunk; +use crate::engine::scan_input::ScanFile; use std::collections::{HashMap, HashSet}; use tracing::debug; @@ -72,43 +73,41 @@ fn matches_any(issue: &ReviewIssue, rules: &HashSet) -> bool { || issue.also_matches.iter().any(|a| hit(a)) } -/// Drop findings suppressed by an inline `cora-ignore:` marker. -pub fn apply(mut issues: Vec, chunks: &[FileChunk]) -> Vec { +/// Drop findings suppressed by an inline `cora-ignore:` marker, reading the +/// post-change lines of a diff (diff adapter over [`apply_lines`]). +pub fn apply(issues: Vec, chunks: &[FileChunk]) -> Vec { + let files: Vec> = chunks.iter().map(ScanFile::post_image).collect(); + apply_lines(issues, &files) +} + +/// Drop findings suppressed by an inline `cora-ignore:` marker found in +/// `files`' lines. +pub fn apply_lines(mut issues: Vec, files: &[ScanFile<'_>]) -> Vec { // (file, line) -> lowercased rules suppressed there. let mut suppressed: HashMap<(String, u32), HashSet> = HashMap::new(); - for chunk in chunks { - let path = chunk - .new_path - .as_deref() - .or(chunk.old_path.as_deref()) - .unwrap_or(""); - for hunk in &chunk.chunks { - for line in &hunk.lines { - if line.line_type == DiffLineType::Remove { - continue; - } - let Some(ln) = line.new_line_no else { continue }; - let rules = parse_rules(&line.content); - if rules.is_empty() { - if line.content.to_lowercase().contains("cora-ignore") { - debug!( - file = path, - line = ln, - "bare cora-ignore ignored: rule list required" - ); - } - continue; + for file in files { + let path = file.path; + for &(ln, content) in &file.lines { + let rules = parse_rules(content); + if rules.is_empty() { + if content.to_lowercase().contains("cora-ignore") { + debug!( + file = path, + line = ln, + "bare cora-ignore ignored: rule list required" + ); } + continue; + } + suppressed + .entry((path.to_string(), ln)) + .or_default() + .extend(rules.iter().cloned()); + if is_comment_only(content) { suppressed - .entry((path.to_string(), ln)) + .entry((path.to_string(), ln + 1)) .or_default() - .extend(rules.iter().cloned()); - if is_comment_only(&line.content) { - suppressed - .entry((path.to_string(), ln + 1)) - .or_default() - .extend(rules); - } + .extend(rules); } } } diff --git a/src/engine/llm/mod.rs b/src/engine/llm/mod.rs index 5e2921c..7dd4797 100644 --- a/src/engine/llm/mod.rs +++ b/src/engine/llm/mod.rs @@ -145,6 +145,7 @@ fn into_review_response(findings: Findings, usage: Option<&transport::Usage>) -> summary: findings.summary.unwrap_or_default(), tokens_used: usage.map(usage_to_token_usage), should_block: false, + dropped_findings: 0, } } diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 63ea8e5..599348f 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -21,6 +21,7 @@ pub mod quality_gate; pub mod review; pub mod review_store; pub mod rules; +pub mod scan_input; pub mod scanner; #[cfg(test)] mod secret_heuristics_tests; diff --git a/src/engine/postprocess.rs b/src/engine/postprocess.rs index 85d44d7..22e620b 100644 --- a/src/engine/postprocess.rs +++ b/src/engine/postprocess.rs @@ -25,6 +25,11 @@ //! | 5 | inline `cora-ignore:` markers | yes | yes | //! | 6 | context-line severity filter | yes | no | //! +//! After the filters, `rules_engine.max_findings` caps the deterministic +//! findings (those with a `rule_id`; LLM issues are never capped), highest +//! severity first. `0` or `null` = unlimited. The dropped count is returned by +//! [`postprocess_report`] so the CLI can warn on stderr (#624). +//! //! Filters 2, 3 and 6 reason about added vs unchanged diff lines (or LLM //! mistakes about added lines), which a full-file scan does not have: every //! line of a scanned file is "added", and scan findings come from the @@ -34,7 +39,8 @@ use tracing::debug; use crate::config::schema::Config; use crate::engine::diff_parser::FileChunk; -use crate::engine::scanner::{FileEntry, files_as_chunks}; +use crate::engine::scan_input; +use crate::engine::scanner::FileEntry; use crate::engine::types::{ReviewIssue, Severity}; use crate::engine::{inline_suppress, rules, secrets_scanner, security_scanner}; @@ -46,14 +52,35 @@ pub enum Source<'a> { Files(&'a [FileEntry]), } +/// Result of [`postprocess_report`]. +#[derive(Debug, Default)] +pub struct PostprocessReport { + /// Final, user-visible findings. + pub issues: Vec, + /// Deterministic findings cut by `rules_engine.max_findings` (0 = none). + pub dropped: usize, +} + /// Run the shared post-processing pipeline over `issues`. See the module docs -/// for the per-source filter table. +/// for the per-source filter table. Drops the truncation count; use +/// [`postprocess_report`] when the caller should surface it. +#[cfg_attr(not(test), allow(dead_code))] // callers use postprocess_report; kept for tests/API pub fn postprocess( issues: Vec, source: &Source<'_>, config: &Config, ) -> Vec { - match source { + postprocess_report(issues, source, config).issues +} + +/// Like [`postprocess`], but also reports how many deterministic findings the +/// `rules_engine.max_findings` cap removed (#624). +pub fn postprocess_report( + issues: Vec, + source: &Source<'_>, + config: &Config, +) -> PostprocessReport { + let issues = match source { Source::Diff(chunks) => { let issues = apply_llm_secret_fp_filter(issues, chunks); let issues = apply_markdown_code_block_filter(issues, chunks); @@ -65,21 +92,65 @@ pub fn postprocess( apply_context_line_filter(issues, chunks) } Source::Files(files) => { - let chunks = files_as_chunks(files); - let max = config.rules_config.max_findings; + let scan_files = scan_input::from_entries(files); + // Scanners run uncapped; the cap is enforced below, after + // suppression (#624). let mut merged = issues; for family in [ - secrets_scanner::scan_secrets(&chunks, max), - security_scanner::scan_security(&chunks, max), + secrets_scanner::scan_secrets_in(&scan_files, usize::MAX), + security_scanner::scan_security_in(&scan_files, usize::MAX), ] { if !family.is_empty() { merged = rules::merge_rule_findings(merged, family); } } let merged = apply_ignore_rules(merged, &config.ignore.rules); - inline_suppress::apply(merged, &chunks) + inline_suppress::apply_lines(merged, &scan_files) } + }; + let (issues, dropped) = cap_deterministic(issues, config.rules_config.finding_cap()); + PostprocessReport { issues, dropped } +} + +/// Keep at most `cap` deterministic findings (those carrying a `rule_id`), +/// highest severity first; ties keep their existing order (stable). LLM issues +/// are never counted or dropped, and every kept issue keeps its position. +/// +/// Because the most severe findings survive, the blocking decision +/// (`any(severity <= min_severity)`) is unchanged by the cap for any `cap >= 1`. +/// Returns the kept issues and the number of deterministic findings dropped. +fn cap_deterministic(issues: Vec, cap: usize) -> (Vec, usize) { + let mut det: Vec = issues + .iter() + .enumerate() + .filter(|(_, i)| i.rule_id.is_some()) + .map(|(idx, _)| idx) + .collect(); + if det.len() <= cap { + return (issues, 0); } + // Stable: equal severities stay in their original relative order. + det.sort_by_key(|&idx| issues[idx].severity); + let drop: std::collections::HashSet = det[cap..].iter().copied().collect(); + let dropped = drop.len(); + let kept = issues + .into_iter() + .enumerate() + .filter(|(idx, _)| !drop.contains(idx)) + .map(|(_, i)| i) + .collect(); + (kept, dropped) +} + +/// One-line stderr warning for truncated deterministic findings, or `None` +/// when nothing was dropped. +pub fn dropped_warning(dropped: usize, config: &Config) -> Option { + (dropped > 0).then(|| { + format!( + "⚠ {dropped} more deterministic findings not shown (rules.max_findings = {}; raise it or set 0/null to show all)", + config.rules_config.max_findings + ) + }) } /// Filter out LLM findings about hardcoded secrets/passwords that point to @@ -345,6 +416,190 @@ fn apply_context_line_filter( mod tests { use super::*; + // ─── max_findings cap (#624) ─── + + fn det(file: &str, line: u32, sev: Severity) -> ReviewIssue { + ReviewIssue::new(file, Some(line), sev, format!("t{line}")).with_rule_id("r") + } + + fn llm(line: u32, sev: Severity) -> ReviewIssue { + ReviewIssue::new("a.rs", Some(line), sev, format!("llm{line}")) + } + + #[test] + fn cap_keeps_highest_severity_stably_and_counts_dropped() { + let issues = vec![ + det("a", 1, Severity::Info), + det("a", 2, Severity::Major), + det("a", 3, Severity::Critical), + det("a", 4, Severity::Major), + det("a", 5, Severity::Info), + ]; + let (kept, dropped) = cap_deterministic(issues, 3); + assert_eq!(dropped, 2); + // Critical + both Majors kept, original relative order preserved. + let lines: Vec<_> = kept.iter().map(|i| i.line.unwrap()).collect(); + assert_eq!(lines, vec![2, 3, 4]); + } + + #[test] + fn cap_never_counts_or_drops_llm_issues() { + let mut issues = vec![llm(1, Severity::Info), llm(2, Severity::Info)]; + issues.extend((10..16).map(|l| det("a", l, Severity::Major))); + issues.push(llm(3, Severity::Minor)); + let (kept, dropped) = cap_deterministic(issues, 2); + assert_eq!(dropped, 4); + assert_eq!(kept.iter().filter(|i| i.rule_id.is_none()).count(), 3); + assert_eq!(kept.iter().filter(|i| i.rule_id.is_some()).count(), 2); + } + + #[test] + fn cap_under_limit_is_a_no_op() { + let issues = vec![det("a", 1, Severity::Info), llm(2, Severity::Info)]; + let (kept, dropped) = cap_deterministic(issues, 5); + assert_eq!((kept.len(), dropped), (2, 0)); + } + + #[test] + fn zero_and_unlimited_semantics() { + let mut cfg = Config::default(); + assert_eq!(cfg.rules_config.finding_cap(), 5, "default cap"); + cfg.rules_config.max_findings = 0; + assert_eq!(cfg.rules_config.finding_cap(), usize::MAX, "0 = unlimited"); + let parsed: crate::config::schema::RulesSection = + serde_yaml_ng::from_str("enabled: true\nmax_findings: null\n").unwrap(); + assert_eq!(parsed.max_findings, 0, "null = 0"); + let parsed: crate::config::schema::RulesSection = + serde_yaml_ng::from_str("enabled: true\n").unwrap(); + assert_eq!(parsed.max_findings, 5, "absent = default"); + } + + #[test] + fn dropped_warning_text() { + let cfg = Config::default(); + assert!(dropped_warning(0, &cfg).is_none()); + let w = dropped_warning(3, &cfg).unwrap(); + assert!(w.contains("3 more deterministic findings")); + assert!(w.contains("rules.max_findings = 5")); + } + + /// Inline marker naming every finding a probe secret line produces, so + /// the marked lines are suppressed regardless of how many rules fire. + fn ignore_marker() -> String { + let files = secret_files_with(1, 0, ""); + let r = postprocess_report(vec![], &Source::Files(&files), &Config::default()); + let ids: Vec<_> = r.issues.iter().filter_map(|i| i.rule_id.clone()).collect(); + assert!(!ids.is_empty(), "probe must produce a finding"); + format!(" # cora-ignore: {}", ids.join(", ")) + } + + fn secret_files(n: usize, extra_on_first: usize) -> Vec { + secret_files_with(n, extra_on_first, &ignore_marker()) + } + + fn secret_files_with(n: usize, extra_on_first: usize, marker: &str) -> Vec { + (0..n) + .map(|i| { + let marker = if i < extra_on_first { marker } else { "" }; + let content = + format!("import os\npassword = \"hunter2hunter{i}\"{marker}\nprint(1)\n"); + FileEntry { + path: format!("src/app{i}.py"), + lines: content.lines().count(), + content, + } + }) + .collect() + } + + fn secret_count(issues: &[ReviewIssue]) -> usize { + issues.iter().filter(|i| i.rule_id.is_some()).count() + } + + #[test] + fn files_source_reports_truncation_with_default_cap() { + let files = secret_files(8, 0); + let r = postprocess_report(vec![], &Source::Files(&files), &Config::default()); + assert_eq!(secret_count(&r.issues), 5); + assert_eq!(r.dropped, 3); + } + + #[test] + fn files_source_unlimited_when_cap_is_zero() { + let files = secret_files(8, 0); + let mut cfg = Config::default(); + cfg.rules_config.max_findings = 0; + let r = postprocess_report(vec![], &Source::Files(&files), &cfg); + assert_eq!(secret_count(&r.issues), 8); + assert_eq!(r.dropped, 0); + } + + #[test] + fn files_source_suppressed_findings_do_not_consume_cap_slots() { + // 5 inline-suppressed secrets + 1 real one: the real one must survive. + let files = secret_files(6, 5); + let r = postprocess_report(vec![], &Source::Files(&files), &Config::default()); + assert_eq!(r.dropped, 0); + assert_eq!(secret_count(&r.issues), 1); + assert_eq!(r.issues[0].file, "src/app5.py"); + } + + #[test] + fn files_source_llm_issues_are_never_capped() { + let files = secret_files(8, 0); + let llm_issues: Vec<_> = (1..=7).map(|l| llm(l, Severity::Info)).collect(); + let r = postprocess_report(llm_issues, &Source::Files(&files), &Config::default()); + assert_eq!(r.issues.iter().filter(|i| i.rule_id.is_none()).count(), 7); + assert_eq!(secret_count(&r.issues), 5); + assert_eq!(r.dropped, 3); + } + + fn secret_diff(n: usize, ignored: usize) -> String { + let marker_text = ignore_marker(); + let mut d = String::new(); + for i in 0..n { + let marker = if i < ignored { + marker_text.as_str() + } else { + "" + }; + d.push_str(&format!( + "diff --git a/src/f{i}.py b/src/f{i}.py\n--- a/src/f{i}.py\n+++ b/src/f{i}.py\n@@ -1,1 +1,2 @@\n import os\n+password = \"hunter2hunter{i}\"{marker}\n" + )); + } + d + } + + fn diff_findings(diff: &str) -> (Vec, Vec) { + let chunks = crate::engine::diff_parser::parse_diff(diff); + let cfg = Config::default(); + // Isolated: a db path inside a fresh temp dir that does not exist, so + // the real data dir is never read. + let tmp = tempfile::tempdir().unwrap(); + let bridge = crate::engine::index_bridge::IndexBridge::open_tolerant_at( + &tmp.path().join("none.db"), + tmp.path(), + ); + let det = crate::engine::deterministic::run(&chunks, &cfg, &bridge); + (det.merge_into(vec![]), chunks) + } + + #[test] + fn diff_source_caps_after_suppression_and_reports_dropped() { + let (issues, chunks) = diff_findings(&secret_diff(8, 0)); + assert!(secret_count(&issues) >= 8, "scanners must run uncapped"); + let r = postprocess_report(issues, &Source::Diff(&chunks), &Config::default()); + assert_eq!(secret_count(&r.issues), 5); + assert_eq!(r.dropped, 3); + + // 5 suppressed + 1 real: nothing dropped, the real one is reported. + let (issues, chunks) = diff_findings(&secret_diff(6, 5)); + let r = postprocess_report(issues, &Source::Diff(&chunks), &Config::default()); + assert_eq!(r.dropped, 0); + assert!(r.issues.iter().all(|i| i.file == "src/f5.py")); + assert!(!r.issues.is_empty()); + } + #[test] fn secret_fp_filter_removes_struct_field_declarations() { use crate::engine::diff_parser::*; diff --git a/src/engine/review.rs b/src/engine/review.rs index d6f4045..6b76a88 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -3,7 +3,7 @@ use tracing::{debug, instrument}; use crate::config::schema::Config; use crate::engine::llm; -use crate::engine::postprocess::{Source, postprocess}; +use crate::engine::postprocess::{Source, postprocess_report}; use crate::engine::types::{LLMConfig, ReviewResponse}; /// Load a custom system prompt from a file path. @@ -97,6 +97,7 @@ async fn review_diff_inner( summary: "No changes to review.".to_string(), tokens_used: None, should_block: false, + dropped_findings: 0, }); } @@ -308,8 +309,11 @@ async fn review_diff_inner( ), tokens_used: None, should_block: false, + dropped_findings: 0, }; - fallback.issues = postprocess(fallback.issues, &Source::Diff(&diff_chunks), config); + let pp = postprocess_report(fallback.issues, &Source::Diff(&diff_chunks), config); + fallback.issues = pp.issues; + fallback.dropped_findings = pp.dropped; let min_sev = config.hook.min_severity_level(); fallback.should_block = fallback .issues @@ -343,7 +347,9 @@ async fn review_diff_inner( // Shared post-processing (LLM secret FP cross-check, Markdown code blocks, // ignore.rules, inline `cora-ignore:`, context-line filter), see // `engine::postprocess`. - response.issues = postprocess(response.issues, &Source::Diff(&diff_chunks), config); + let pp = postprocess_report(response.issues, &Source::Diff(&diff_chunks), config); + response.issues = pp.issues; + response.dropped_findings = pp.dropped; // Calculate should_block based on min_severity let min_severity = config.hook.min_severity_level(); diff --git a/src/engine/rules/builtin.rs b/src/engine/rules/builtin.rs index c722962..0208efa 100644 --- a/src/engine/rules/builtin.rs +++ b/src/engine/rules/builtin.rs @@ -143,7 +143,9 @@ pub(crate) fn secret_literal_pattern() -> String { } /// SQL password literal without `=`: `... WITH PASSWORD 'x'`, `IDENTIFIED BY 'x'`. -const SQL_PASSWORD_PATTERN: &str = r"(?i)\b(?:password|identified\s+by)\s+'([^']+)'"; +/// The literal may be single- or double-quoted (#625), optionally backslash +/// escaped when the SQL sits inside a host-language string (`\"pw\"`). +const SQL_PASSWORD_PATTERN: &str = r#"(?i)\b(?:password|identified\s+by)\s+\\?["']([^"']+)["']"#; static SQL_PASSWORD_RE: LazyLock = LazyLock::new(|| Regex::new(SQL_PASSWORD_PATTERN).expect("sql password regex must compile")); @@ -164,13 +166,18 @@ pub(crate) fn has_secret_literal(line: &str, path: &str) -> bool { if quoted_placeholder_value(line) || non_literal_rhs(strip_trailing_comment(line), path) { return false; } - SECRET_LITERAL_RE.is_match(line) || plain_scalar_secret(strip_trailing_comment(line)) + let code = strip_trailing_comment(line); + SECRET_LITERAL_RE.is_match(line) + || plain_scalar_secret(code, path) + || secret_assign_rhs(code).is_some_and(|rhs| { + embedded_literal(rhs.trim_end_matches([';', ',']).trim_end()) == Some(true) + }) } /// A SQL literal that is a bind/format placeholder, not a password: /// `%s`, `%v`, `$1`, `?`, `:name`, `@name`, `{}` / `{pw}` / `${PW}`, ``. fn is_sql_placeholder(lit: &str) -> bool { - let lit = lit.trim(); + let lit = lit.trim().trim_end_matches('\\'); lit.starts_with(['$', ':', '@', '?']) || lit.contains('%') || (lit.contains('{') && lit.contains('}')) @@ -199,10 +206,49 @@ fn secret_kv_value(line: &str) -> Option<&str> { /// (`required`, `formPassword`, `vault_db_password`) stay "references". Never /// a YAML tag/anchor/alias/block indicator, `${..}`/`$VAR`, a path, or a /// keyword (`true`, `null`, ...). -fn plain_scalar_secret(line: &str) -> bool { - let Some(v) = secret_kv_value(line) else { +/// +/// Only applies outside source code (#625): `secret: Secret1Type` in a `.ts` +/// or `.py` file is a type/identifier, not a YAML scalar. Flow-style entries +/// (`{ password: hunter2hunter2 }`) are only recognised in `.yaml`/`.yml`, as +/// in code they are object shorthand with an identifier value. All-alphabetic +/// values (`password: correcthorsebattery`) are deliberately NOT flagged: they +/// cannot be told apart from references (`secret: kubernetes`, `token: optional`). +fn plain_scalar_secret(line: &str, path: &str) -> bool { + if rhs_style(path) == RhsStyle::Code { return false; - }; + } + if let Some(v) = secret_kv_value(line) { + // compose-style `password: ${DB_PASSWORD:-hunter2hunter2}` (#629) + if !v.starts_with(['"', '\'']) && !v.contains("$(") && shell_secret_default(v) { + return true; + } + if plain_scalar_value_ok(v) { + return true; + } + } + is_yaml_path(path) && flow_kv_values(line).any(plain_scalar_value_ok) +} + +fn is_yaml_path(path: &str) -> bool { + path.rsplit_once('.') + .is_some_and(|(_, e)| e.eq_ignore_ascii_case("yaml") || e.eq_ignore_ascii_case("yml")) +} + +/// Values of flow-style mapping entries on the line whose key ends in a secret +/// word: `{ password: v }`, `db: { user: a, token: v }`. +fn flow_kv_values(line: &str) -> impl Iterator { + static FLOW_RE: LazyLock = LazyLock::new(|| { + Regex::new( + r#"(?i)[{,]\s*["']?[\w.\-]*(?:password|passwd|pwd|secret|api_?key|token)["']?\s*:\s+([^\s,{}\[\]]+)\s*(?:[,}]|$)"#, + ) + .expect("flow kv regex must compile") + }); + FLOW_RE + .captures_iter(line) + .filter_map(|c| c.get(1).map(|m| m.as_str())) +} + +fn plain_scalar_value_ok(v: &str) -> bool { let lower = v.to_ascii_lowercase(); v.len() >= 8 && !v.contains(char::is_whitespace) @@ -238,7 +284,7 @@ fn quoted_placeholder_value(line: &str) -> bool { let v = &cap[1]; let inner = &v[1..v.len() - 1]; let placeholder = inner.is_empty() - || (inner.starts_with("${") && inner.ends_with('}')) + || (inner.starts_with("${") && inner.ends_with('}') && !shell_secret_default(inner)) || (inner.starts_with("{{") && inner.ends_with("}}")) || (inner.starts_with('$') && inner.len() > 1 @@ -317,7 +363,10 @@ fn secret_assign_rhs(line: &str) -> Option<&str> { /// A quoted value that is nothing but a reference: `${X}`, `$X`, `$(cmd)`. fn is_pure_reference(inner: &str) -> bool { - (inner.starts_with("${") && inner.ends_with('}') && inner.matches("${").count() == 1) + (inner.starts_with("${") + && inner.ends_with('}') + && inner.matches("${").count() == 1 + && !shell_secret_default(inner)) || (inner.starts_with("$(") && inner.ends_with(')')) || (inner.starts_with('$') && inner.len() > 1 @@ -327,17 +376,90 @@ fn is_pure_reference(inner: &str) -> bool { } /// Does the RHS start with a quoted literal (`"x"`, `'x'`, `` `x` ``, `b"x"`, -/// `r#"x"#`, `f'x'`) or a literal wrapped by a constructor (`String::from("x")`)? +/// `r#"x"#`, `f'x'`)? Constructor-wrapped literals are handled by +/// [`embedded_literal`]. fn starts_with_literal(rhs: &str) -> bool { static LIT_RE: LazyLock = LazyLock::new(|| { - Regex::new( - r#"^(?:[A-Za-z]{0,2}#*["'`]|(?:String::from|Some|Secret(?:String)?::(?:new|from)|new\s+String)\s*\(\s*[A-Za-z]{0,2}#*["'`])"#, - ) - .expect("literal-start regex must compile") + Regex::new(r#"^[A-Za-z]{0,2}#*["'`]"#).expect("literal-start regex must compile") }); LIT_RE.is_match(rhs) } +/// Constructors that merely wrap a string literal: the literal is still the +/// value (#630). Plain calls (`get_password()`) are not in this list. +const WRAPPER: &str = r"(?:new\s+String|String::from|Some|Secret(?:String|Str)?::(?:new|from)|Cow::(?:Owned|Borrowed)|Box::from|Arc::from|Rc::from|str|bytes|String)"; + +/// Env reads with a literal fallback in the DEFAULT position (#628). Each +/// alternative ends on the opening quote of the default, so the env var NAME +/// string (`os.getenv("DB_PASSWORD", ..)`) is never mistaken for the secret. +const ENV_DEFAULT: &str = concat!( + // Python: os.environ.get("X", "lit"), os.getenv("X", "lit") + r#"(?:(?:os\.)?(?:environ\.get|getenv)\s*\(\s*(?:"[^"]*"|'[^']*'|[\w.]+)\s*,\s*(?:default\s*=\s*)?[A-Za-z]{0,2}["']"#, + // JS/TS: process.env.X || "lit", process.env["X"] ?? "lit" + r#"|(?:process\.env|Deno\.env|import\.meta\.env)(?:\.\w+|\[[^\]]*\]|\.get\([^)]*\))\s*(?:\|\||\?\?)\s*["'`]"#, + // Rust: env::var("X").unwrap_or("lit".into()), .unwrap_or_else(|_| "lit".to_string()) + r#"|(?:std::)?env::var(?:_os)?\s*\([^)]*\)\s*(?:\.ok\(\))?\s*\.unwrap_or(?:_else)?\s*\(\s*(?:\|[^|]*\|\s*)?(?:String::from\s*\(\s*)?["']"#, + // Ruby: ENV.fetch("X", "lit"), ENV["X"] || "lit" + r#"|ENV\.fetch\s*\(\s*(?:"[^"]*"|'[^']*')\s*,\s*["']|ENV\[[^\]]*\]\s*\|\|\s*["']"#, + // PHP: getenv('X') ?: 'lit', $_ENV['X'] ?? 'lit' + r#"|(?:getenv\s*\([^)]*\)|\$_(?:ENV|SERVER)\[[^\]]*\])\s*(?:\?:|\?\?|\|\|)\s*["'])"#, +); + +/// Inner text of the quoted literal starting at `rest` (which begins with the quote). +fn literal_after(rest: &str) -> &str { + let mut chars = rest.chars(); + let Some(q) = chars.next() else { return "" }; + let body = &rest[q.len_utf8()..]; + body.find(q).map_or(body, |end| &body[..end]) +} + +/// A literal that is empty or itself a reference/template (`${X}`, `$X`, `{{ x }}`). +fn is_placeholder_literal(inner: &str) -> bool { + let t = inner.trim(); + t.is_empty() || is_pure_reference(t) || (t.starts_with("{{") && t.ends_with("}}")) +} + +/// Does the text hold a shell `${VAR:-default}` / `${VAR:=default}` whose default +/// is a secret-looking literal (#629)? Empty defaults, nested references, paths, +/// URLs and bare keywords are not. `${VAR}` / `${VAR:?msg}` never match. +fn shell_secret_default(s: &str) -> bool { + static RE: LazyLock = LazyLock::new(|| { + Regex::new(r"\$\{[A-Za-z_][A-Za-z0-9_]*:[-=]([^}]*)\}").expect("shell default regex") + }); + RE.captures_iter(s).any(|c| { + let d = c[1].trim(); + !d.is_empty() + && !d.contains(['$', '`']) + && !d.starts_with(['/', '.', '~']) + && !d.contains("://") + && !matches!( + d.to_ascii_lowercase().as_str(), + "true" | "false" | "null" | "none" | "nil" | "yes" | "no" | "required" + ) + }) +} + +/// A literal embedded in an expression on the RHS: inside a constructor wrapper +/// (`new String("x")`, `Cow::Owned("x")`, `str("x")`), in the default position of +/// an env read, or as a shell `${VAR:-default}` default. `Some(true)` = a real +/// secret literal, `Some(false)` = such a shape but the literal is empty or a +/// placeholder, `None` = not one of these shapes. +fn embedded_literal(rhs: &str) -> Option { + static WRAP_RE: LazyLock = LazyLock::new(|| { + Regex::new(&format!(r#"^(?:{WRAPPER}\s*\(\s*)+[A-Za-z]{{0,2}}#*["'`]"#)) + .expect("wrapper regex must compile") + }); + static ENV_RE: LazyLock = LazyLock::new(|| { + Regex::new(&format!("^{ENV_DEFAULT}")).expect("env default regex must compile") + }); + for re in [&*WRAP_RE, &*ENV_RE] { + if let Some(m) = re.find(rhs) { + return Some(!is_placeholder_literal(literal_after(&rhs[m.end() - 1..]))); + } + } + (!rhs.contains("$(") && shell_secret_default(rhs)).then_some(true) +} + /// Is the right-hand side of a secret-named assignment provably NOT a literal /// value (#616, #617)? /// @@ -368,6 +490,11 @@ fn non_literal_rhs(line: &str, path: &str) -> bool { if starts_with_literal(rhs) { return false; } + // A literal inside a constructor wrapper, an env-read default or a shell + // `${VAR:-default}` is judged on the literal itself (#628, #629, #630). + if let Some(is_literal) = embedded_literal(rhs) { + return !is_literal; + } if rhs.starts_with('$') || rhs.contains("${") || rhs.contains("$(") || rhs.contains('`') { return true; } @@ -565,7 +692,7 @@ fn is_false_positive_secret(line: &str, path: &str) -> bool { } // Unquoted YAML `password: hunter2hunter2` is a real secret, not shorthand (#619) - if plain_scalar_secret(line) { + if plain_scalar_secret(line, path) { return false; } @@ -1228,4 +1355,36 @@ mod tests { "No wildcard in this line" )); } + + // ─── #625, #628, #629, #630: the LLM cross-check agrees with the scanners ─── + + #[test] + fn llm_cross_check_keeps_embedded_literals() { + for (line, path) in [ + ( + r#"String password = new String("hunter2hunter2xx");"#, + "A.java", + ), + ( + r#"password = os.getenv("DB_PASSWORD", "hunter2hunter2xx")"#, + "a.py", + ), + ( + r#"const password = process.env.X || "hunter2hunter2xx";"#, + "a.js", + ), + ("DB_PASSWORD=${DB_PASSWORD:-hunter2hunter2xx}", "a.sh"), + ("{ password: hunter2hunter2 }", "a.yaml"), + ] { + assert!(has_secret_literal(line, path), "{line}"); + } + for (line, path) in [ + (r#"password = os.getenv("DB_PASSWORD")"#, "a.py"), + (r#"password = os.getenv("DB_PASSWORD", "")"#, "a.py"), + ("DB_PASSWORD=${DB_PASSWORD:-}", "a.sh"), + ("secret: Secret1Type", "a.ts"), + ] { + assert!(!has_secret_literal(line, path), "{line}"); + } + } } diff --git a/src/engine/rules/mod.rs b/src/engine/rules/mod.rs index d2898d6..63228c5 100644 --- a/src/engine/rules/mod.rs +++ b/src/engine/rules/mod.rs @@ -6,8 +6,9 @@ pub mod types; use tracing::debug; -use crate::engine::diff_parser::{DiffLineType, FileChunk, parse_diff}; +use crate::engine::diff_parser::{FileChunk, parse_diff}; use crate::engine::rules::types::{RuleFinding, RulesConfig}; +use crate::engine::scan_input::{self, ScanFile}; use crate::engine::types::{ReviewIssue, Severity}; /// Map severity to a numeric rank for sorting (Critical=4, Major=3, Minor=2, Info=1). @@ -20,10 +21,17 @@ fn severity_rank(sev: Severity) -> u8 { } } -/// Run all rules (built-in + custom) against parsed diff chunks. +/// Run all rules (built-in + custom) against the added lines of parsed diff +/// chunks (diff adapter over [`run_rules_in`]). /// /// Returns findings capped by `config.max_findings`. pub fn run_rules(chunks: &[FileChunk], config: &RulesConfig) -> Vec { + run_rules_in(&scan_input::from_chunks(chunks), config) +} + +/// Run all rules (built-in + custom) against file lines. The one implementation +/// behind both the diff and the whole-file entry points. +pub fn run_rules_in(files: &[ScanFile<'_>], config: &RulesConfig) -> Vec { if !config.enabled { debug!("rule engine is disabled"); return Vec::new(); @@ -44,52 +52,39 @@ pub fn run_rules(chunks: &[FileChunk], config: &RulesConfig) -> Vec let mut findings = Vec::new(); - for file in chunks { - let file_path = file - .new_path - .as_deref() - .or(file.old_path.as_deref()) - .unwrap_or("unknown"); - - for hunk in &file.chunks { - for line in &hunk.lines { - // Only check added lines (new code being introduced) - if line.line_type != DiffLineType::Add { + for file in files { + let file_path = file.path; + + for &(line_no, content) in &file.lines { + for rule in &all_rules { + // Check language filter + if !matching::matches_language(rule, file.language) { continue; } - for rule in &all_rules { - // Check language filter - if !matching::matches_language(rule, &file.language) { - continue; - } - - // Check exclude filter - if matching::matches_exclude(rule, file_path) { - continue; - } - - // Check pattern match - if !matching::match_rule_against_line(rule, &line.content) { - continue; - } - - // Post-match filter (e.g., allow localhost URLs) - if builtin::post_match_filter_for_path(&rule.id, &line.content, file_path) { - continue; - } - - let line_no = line.new_line_no.unwrap_or(0); - - findings.push(RuleFinding { - rule_id: rule.id.clone(), - file: file_path.to_string(), - line: line_no, - severity: rule.severity, - title: format!("[{}] Rule: {}", rule.id, rule.id), - body: rule.message.clone(), - }); + // Check exclude filter + if matching::matches_exclude(rule, file_path) { + continue; } + + // Check pattern match + if !matching::match_rule_against_line(rule, content) { + continue; + } + + // Post-match filter (e.g., allow localhost URLs) + if builtin::post_match_filter_for_path(&rule.id, content, file_path) { + continue; + } + + findings.push(RuleFinding { + rule_id: rule.id.clone(), + file: file_path.to_string(), + line: line_no, + severity: rule.severity, + title: format!("[{}] Rule: {}", rule.id, rule.id), + body: rule.message.clone(), + }); } } } @@ -108,7 +103,7 @@ pub fn run_rules(chunks: &[FileChunk], config: &RulesConfig) -> Vec findings.dedup_by(|a, b| a.rule_id == b.rule_id && a.file == b.file && a.line == b.line); // Cap at max_findings - let capped = findings.len().min(config.max_findings); + let capped = findings.len().min(config.finding_cap()); if findings.len() > capped { debug!(total = findings.len(), capped, "capping rule findings"); findings.truncate(capped); diff --git a/src/engine/rules/types.rs b/src/engine/rules/types.rs index 7567479..8602326 100644 --- a/src/engine/rules/types.rs +++ b/src/engine/rules/types.rs @@ -8,7 +8,8 @@ use crate::engine::Severity; pub struct RulesConfig { /// Whether the rule engine is enabled. pub enabled: bool, - /// Maximum number of findings to report per review (prevents noisy output). + /// Maximum number of deterministic findings shown per run, applied after + /// suppression (`ignore.rules`, `cora-ignore`). `0` = unlimited (#624). pub max_findings: usize, /// User-defined custom rules, merged with built-in rules. pub custom_rules: Vec, @@ -48,6 +49,17 @@ pub(crate) fn default_index_skip_files() -> Vec { ] } +impl RulesConfig { + /// Effective cap: `0` means unlimited. + pub fn finding_cap(&self) -> usize { + if self.max_findings == 0 { + usize::MAX + } else { + self.max_findings + } + } +} + impl Default for RulesConfig { fn default() -> Self { Self { diff --git a/src/engine/scan_input.rs b/src/engine/scan_input.rs new file mode 100644 index 0000000..c6700e1 --- /dev/null +++ b/src/engine/scan_input.rs @@ -0,0 +1,132 @@ +//! Scanner input seam (#610): "the lines of a file", independent of how they +//! were obtained. +//! +//! The deterministic scanners (`secrets_scanner`, `security_scanner`, +//! `rules::run_rules`) and the inline `cora-ignore:` filter only need a path, a +//! language and `(line number, text)` pairs. [`ScanFile`] is that shape; the +//! diff and whole-file sources are thin adapters over it: +//! +//! - [`ScanFile::added_lines`]: the added lines of a diff chunk (what the +//! scanners looked at before; `cora review`). +//! - [`ScanFile::post_image`]: added + context lines of a diff chunk (what the +//! inline-suppression filter reads). +//! - [`ScanFile::from_entry`]: every line of a scanned file (`cora scan`, MCP +//! snippet check). + +use std::path::Path; + +use crate::engine::diff_parser::{DiffLineType, FileChunk}; +use crate::engine::scanner::FileEntry; + +/// One file's worth of lines to scan. +#[derive(Debug, Clone)] +pub struct ScanFile<'a> { + /// Path used in findings and for path-aware classification. + pub path: &'a str, + /// Language tag (file extension for files, the chunk language for diffs). + pub language: &'a str, + /// `(line number, text)`. Diff lines without a number carry `0`. + pub lines: Vec<(u32, &'a str)>, +} + +impl<'a> ScanFile<'a> { + /// Added lines of a diff chunk, in diff order. A missing path becomes + /// `"unknown"` and a missing new-side line number becomes `0`, exactly as + /// the scanners treated them before this seam existed. + pub fn added_lines(chunk: &'a FileChunk) -> Self { + let lines = chunk + .chunks + .iter() + .flat_map(|h| &h.lines) + .filter(|l| l.line_type == DiffLineType::Add) + .map(|l| (l.new_line_no.unwrap_or(0), l.content.as_str())) + .collect(); + Self { + path: chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or("unknown"), + language: chunk.language.as_str(), + lines, + } + } + + /// Post-change lines (added + context, with a new-side number) of a diff + /// chunk. A missing path becomes `""`. + pub fn post_image(chunk: &'a FileChunk) -> Self { + let lines = chunk + .chunks + .iter() + .flat_map(|h| &h.lines) + .filter(|l| l.line_type != DiffLineType::Remove) + .filter_map(|l| l.new_line_no.map(|n| (n, l.content.as_str()))) + .collect(); + Self { + path: chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or(""), + language: chunk.language.as_str(), + lines, + } + } + + /// Every line of a whole file, numbered from 1. The language is the file + /// extension (`""` when there is none). + pub fn from_entry(entry: &'a FileEntry) -> Self { + Self { + path: entry.path.as_str(), + language: Path::new(&entry.path) + .extension() + .and_then(|e| e.to_str()) + .unwrap_or(""), + lines: entry + .content + .lines() + .zip(1u32..) + .map(|(t, n)| (n, t)) + .collect(), + } + } +} + +/// Added-line view of every chunk (diff adapter). +pub fn from_chunks(chunks: &[FileChunk]) -> Vec> { + chunks.iter().map(ScanFile::added_lines).collect() +} + +/// Whole-file view of every entry (file adapter). +pub fn from_entries(entries: &[FileEntry]) -> Vec> { + entries.iter().map(ScanFile::from_entry).collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::engine::diff_parser::parse_diff; + + #[test] + fn file_entry_is_numbered_from_one_with_extension_language() { + let e = FileEntry { + path: "src/a.py".into(), + content: "x = 1\ny = 2\n".into(), + lines: 2, + }; + let f = ScanFile::from_entry(&e); + assert_eq!(f.path, "src/a.py"); + assert_eq!(f.language, "py"); + assert_eq!(f.lines, vec![(1, "x = 1"), (2, "y = 2")]); + } + + #[test] + fn diff_adapters_split_added_from_post_image() { + let diff = "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -1,2 +1,3 @@\n ctx\n-gone\n+added\n tail\n"; + let chunks = parse_diff(diff); + let added = ScanFile::added_lines(&chunks[0]); + assert_eq!(added.lines, vec![(2, "added")]); + let post = ScanFile::post_image(&chunks[0]); + assert_eq!(post.lines, vec![(1, "ctx"), (2, "added"), (3, "tail")]); + } +} diff --git a/src/engine/scanner.rs b/src/engine/scanner.rs index 5b1c1e3..60a515e 100644 --- a/src/engine/scanner.rs +++ b/src/engine/scanner.rs @@ -2,7 +2,6 @@ use std::collections::BTreeSet; use std::io::IsTerminal; use std::path::Path; -use crate::engine::diff_parser::{DiffHunk, DiffLine, DiffLineType, FileChunk}; use crate::engine::path_match::PathMatcher; use crate::error::CoraError; use ignore::WalkBuilder; @@ -20,51 +19,6 @@ pub struct FileEntry { pub lines: usize, } -/// View scanned files as synthetic "new file" diff chunks (every line added), -/// so the diff-based deterministic scanners and the inline `cora-ignore:` -/// filter can run on a full-file scan unchanged (#595). -pub fn files_as_chunks(files: &[FileEntry]) -> Vec { - files - .iter() - .filter(|f| !f.content.is_empty()) - .map(|f| { - let lines: Vec = f - .content - .lines() - .zip(1u32..) - .map(|(text, n)| DiffLine { - line_type: DiffLineType::Add, - content: text.to_string(), - old_line_no: None, - new_line_no: Some(n), - }) - .collect(); - let count = u32::try_from(lines.len()).unwrap_or(u32::MAX); - let language = Path::new(&f.path) - .extension() - .and_then(|e| e.to_str()) - .unwrap_or("") - .to_string(); - FileChunk { - old_path: None, - new_path: Some(f.path.clone()), - language, - chunks: vec![DiffHunk { - old_start: 0, - old_count: 0, - new_start: 1, - new_count: count, - header: String::new(), - lines, - }], - is_binary: false, - is_deleted: false, - is_new: true, - } - }) - .collect() -} - /// File extensions to include in scans by default (source code). const DEFAULT_EXTENSIONS: &[&str] = &[ "rs", "py", "js", "ts", "tsx", "jsx", "go", "java", "kt", "rb", "c", "cpp", "h", "hpp", "cs", diff --git a/src/engine/secret_heuristics_tests.rs b/src/engine/secret_heuristics_tests.rs index 8476620..f325357 100644 --- a/src/engine/secret_heuristics_tests.rs +++ b/src/engine/secret_heuristics_tests.rs @@ -190,7 +190,197 @@ const CASES: &[Row] = &[ ("yaml", "password_file: /run/secrets/db_password", false), ("yaml", "password: /run/secrets/db_password", false), ("yaml", "secretName: db-credentials-secret", false), - ("ts", " secret: Secret1Type", true), + // #625: the plain-scalar rule is YAML-only; identifiers in code are not scalars + ("ts", " secret: Secret1Type", false), + ("py", "secret: Secret1Type", false), + ("rs", " pub secret: Secret1Type,", false), + ("yaml", "{ password: hunter2hunter2 }", true), + ("yml", "db: { user: app, token: abcd1234efgh5678 }", true), + ("yaml", "{ password: ${DB_PASSWORD} }", false), + ("yaml", "{ password: formPassword }", false), + ("ts", "const body = { password: hunter2hunter2 };", false), + ("yaml", "password: ${DB_PASSWORD:-hunter2hunter2xx}", true), + ( + "yaml", + r#"password: "${DB_PASSWORD:-hunter2hunter2xx}""#, + true, + ), + ("yaml", "password: ${DB_PASSWORD:-}", false), + // #625: unspaced `:=` / `:` never reads as a Go short declaration of a call + ("go", r#"password:=os.Getenv("DB_PASSWORD")"#, false), + ("txt", r#"password:=os.Getenv("DB_PASSWORD")"#, false), + ("go", r#"password:="hunter2hunter2""#, true), + // #625: SQL PASSWORD with double quotes + ( + "sql", + r#"CREATE USER app WITH PASSWORD "hunter2hunter2";"#, + true, + ), + ( + "sql", + r#"CREATE USER app IDENTIFIED BY "hunter2hunter2";"#, + true, + ), + ("sql", r#"CREATE USER app WITH PASSWORD "";"#, false), + ("sql", r#"CREATE USER app WITH PASSWORD "%s";"#, false), + ("sql", r#"CREATE USER app WITH PASSWORD "$1";"#, false), + ( + "sql", + r#"CREATE USER app WITH PASSWORD "{password}";"#, + false, + ), + ("go", r#"q := "CREATE USER app WITH PASSWORD "%s"""#, false), + ( + "go", + r#"q := "CREATE USER app WITH PASSWORD "hunter2hunter2"""#, + true, + ), + // #630: constructor wrappers around a literal + ( + "java", + r#"String password = new String("hunter2hunter2xx");"#, + true, + ), + ("java", r#"String password = new String("");"#, false), + ( + "rs", + r#"let password = Cow::Owned("hunter2hunter2xx".to_string());"#, + true, + ), + ( + "rs", + r#"let password: Cow = Cow::Borrowed("hunter2hunter2xx");"#, + true, + ), + ( + "rs", + r#"let password: Box = Box::from("hunter2hunter2xx");"#, + true, + ), + ( + "rs", + r#"let password = Secret::new("hunter2hunter2xx".to_string());"#, + true, + ), + ("py", r#"password = str("hunter2hunter2xx")"#, true), + ("py", r#"password = str("")"#, false), + ("py", r#"password = str("${DB_PASSWORD}")"#, false), + ("py", "password = str(raw_value)", false), + ("rs", r#"let password = Cow::Owned(String::new());"#, false), + ("py", "password = get_password()", false), + ("java", "String password = getPassword();", false), + ("java", r#"String password = lookup("DB_PASSWORD");"#, false), + // #628: literal default inside an env read (default position only) + ( + "py", + r#"password = os.environ.get("DB_PASSWORD", "hunter2hunter2xx")"#, + true, + ), + ( + "py", + r#"password = os.getenv("DB_PASSWORD", "hunter2hunter2xx")"#, + true, + ), + ( + "py", + r#"password = os.getenv("DB_PASSWORD", default="hunter2hunter2xx")"#, + true, + ), + ("py", r#"password = os.getenv("DB_PASSWORD", "")"#, false), + ( + "py", + r#"password = os.getenv("DB_PASSWORD", "${DB_PASSWORD}")"#, + false, + ), + ( + "py", + r#"password = os.getenv("DB_PASSWORD", fallback)"#, + false, + ), + ( + "rs", + r#"let password = std::env::var("DB_PASSWORD").unwrap_or("hunter2hunter2xx".into());"#, + true, + ), + ( + "rs", + r#"let password = std::env::var("DB_PASSWORD").unwrap_or_else(|_| "hunter2hunter2xx".to_string());"#, + true, + ), + ( + "rs", + r#"let password = std::env::var("DB_PASSWORD").unwrap_or("".into());"#, + false, + ), + ( + "rs", + r#"let password = std::env::var("DB_PASSWORD").unwrap_or_else(|_| fallback());"#, + false, + ), + ( + "js", + r#"const password = process.env.DB_PASSWORD || "hunter2hunter2xx";"#, + true, + ), + ( + "js", + r#"const password = process.env.DB_PASSWORD ?? "hunter2hunter2xx";"#, + true, + ), + ( + "ts", + r#"const password = process.env["DB_PASSWORD"] ?? 'hunter2hunter2xx';"#, + true, + ), + ( + "js", + r#"const password = process.env.DB_PASSWORD || "";"#, + false, + ), + ( + "js", + r#"const password = process.env.DB_PASSWORD ?? "${DB_PASSWORD}";"#, + false, + ), + ("js", "const password = process.env.DB_PASSWORD;", false), + ( + "rb", + r#"password = ENV.fetch("DB_PASSWORD", "hunter2hunter2xx")"#, + true, + ), + ("rb", r#"password = ENV.fetch("DB_PASSWORD")"#, false), + ( + "php", + r#"$password = getenv('DB_PASSWORD') ?: 'hunter2hunter2xx';"#, + true, + ), + // #629: shell default literal inside `${VAR:-..}` / `${VAR:=..}` + ("sh", "DB_PASSWORD=${DB_PASSWORD:-hunter2hunter2xx}", true), + ("env", "DB_PASSWORD=${DB_PASSWORD:=hunter2hunter2xx}", true), + ( + "sh", + r#"export DB_PASSWORD="${DB_PASSWORD:-hunter2hunter2xx}""#, + true, + ), + ("sh", "DB_PASSWORD=${DB_PASSWORD}", false), + ("sh", "DB_PASSWORD=${DB_PASSWORD:-}", false), + ("sh", "DB_PASSWORD=${DB_PASSWORD:?must be set}", false), + ("sh", "DB_PASSWORD=${DB_PASSWORD:-$VAULT_PASSWORD}", false), + ( + "sh", + "DB_PASSWORD=${DB_PASSWORD:-${FALLBACK_PASSWORD}}", + false, + ), + ( + "sh", + "DB_PASSWORD_FILE=${DB_PASSWORD_FILE:-/run/secrets/db}", + false, + ), + ( + "sh", + "DB_PASSWORD=${DB_PASSWORD:-$(cat /run/secrets/pw)}", + false, + ), // #619: quoted JSON keys ("json", r#" "password": "hunter2hunter2","#, true), ("json", r#" "apiKey": "abcd1234efgh5678""#, true), @@ -359,30 +549,8 @@ const CASES: &[Row] = &[ /// the ideal result in the trailing comment). Move a row into [`CASES`] with /// the ideal expectation when its issue is fixed. const KNOWN_GAPS: &[Row] = &[ - // KNOWN GAP #628 - FN: an unquoted RHS in source code is treated as an expression, so a real literal default inside a call is missed (ideal: flagged) - ( - "py", - r#"password = os.environ.get("DB_PASSWORD", "hunter2hunter2xx")"#, - false, - ), - ( - "rs", - r#"let password = std::env::var("DB_PASSWORD").unwrap_or("hunter2hunter2xx".into());"#, - false, - ), - ( - "js", - r#"const password = process.env.DB_PASSWORD || "hunter2hunter2xx";"#, - false, - ), - // KNOWN GAP #630 - FN: the scanner regex needs a >= 8 char token after `=`, so `new String("lit")` and other short-prefixed constructors are never matched (ideal: flagged) - ( - "java", - r#"String password = new String("hunter2hunter2xx");"#, - false, - ), - // KNOWN GAP #629 - FN: shell `${VAR:-literal}` default with a real literal is treated as interpolation (ideal: flagged) - ("sh", "DB_PASSWORD=${DB_PASSWORD:-hunter2hunter2xx}", false), + // KNOWN GAP #635 - FN: an all-alphabetic unquoted YAML secret cannot be told apart from a reference (`secret: kubernetes`, `token: optional`) without a value classifier (ideal: flagged) + ("yaml", "password: correcthorsebattery", false), ]; #[test] diff --git a/src/engine/secrets_scanner.rs b/src/engine/secrets_scanner.rs index 4afc955..c29d718 100644 --- a/src/engine/secrets_scanner.rs +++ b/src/engine/secrets_scanner.rs @@ -5,8 +5,9 @@ use std::sync::LazyLock; use tracing::debug; use crate::engine::Severity; -use crate::engine::diff_parser::{DiffLineType, FileChunk}; +use crate::engine::diff_parser::FileChunk; use crate::engine::rules::types::RuleFinding; +use crate::engine::scan_input::{self, ScanFile}; // ─── Built-in secret patterns ─── @@ -107,80 +108,72 @@ static TEST_FIXTURE_RE: LazyLock = LazyLock::new(|| { // ─── Public API ─── -/// Scan diff chunks for known secret patterns. +/// Scan the added lines of diff chunks for known secret patterns (diff adapter +/// over [`scan_secrets_in`]). /// /// Returns findings capped at `max_findings`. Secret values are masked in the /// message body. pub fn scan_secrets(chunks: &[FileChunk], max_findings: usize) -> Vec { + scan_secrets_in(&scan_input::from_chunks(chunks), max_findings) +} + +/// Scan file lines for known secret patterns. The one implementation behind +/// both the diff and the whole-file entry points. +pub fn scan_secrets_in(files: &[ScanFile<'_>], max_findings: usize) -> Vec { let mut findings = Vec::new(); - for file in chunks { - let file_path = file - .new_path - .as_deref() - .or(file.old_path.as_deref()) - .unwrap_or("unknown"); + for file in files { + let file_path = file.path; // Test/spec/fixture/mock/example paths skip the generic rules (noisy // there) but still get the shared high-confidence list, since real // credentials are often committed in exactly those places (#579). let is_fixture = TEST_FIXTURE_RE.is_match(file_path); - for hunk in &file.chunks { - for line in &hunk.lines { - if line.line_type != DiffLineType::Add { - continue; - } - - let line_no = line.new_line_no.unwrap_or(0); - - if is_fixture { - if let Some((pat, matched)) = - crate::engine::secret_patterns::find_high_confidence(&line.content) - { - findings.push(RuleFinding { - rule_id: pat.id.to_string(), - file: file_path.to_string(), - line: line_no, - severity: Severity::Critical, - title: format!("[{}] {}", pat.id, pat.name), - body: format!( - "{} detected in a test/fixture path — verify it is a fake, \ - otherwise mask with environment variable. Matched: {}", - pat.name, - mask_secret(matched) - ), - }); - if findings.len() >= max_findings { - break; - } + for &(line_no, content) in &file.lines { + if is_fixture { + if let Some((pat, matched)) = + crate::engine::secret_patterns::find_high_confidence(content) + { + findings.push(RuleFinding { + rule_id: pat.id.to_string(), + file: file_path.to_string(), + line: line_no, + severity: Severity::Critical, + title: format!("[{}] {}", pat.id, pat.name), + body: format!( + "{} detected in a test/fixture path — verify it is a fake, \ + otherwise mask with environment variable. Matched: {}", + pat.name, + mask_secret(matched) + ), + }); + if findings.len() >= max_findings { + break; } - continue; } + continue; + } - for (re, pat) in COMPILED.iter() { - if let Some(m) = re.find(&line.content) { - let matched = m.as_str(); - findings.push(RuleFinding { - rule_id: pat.id.to_string(), - file: file_path.to_string(), - line: line_no, - severity: pat.severity, - title: format!("[{}] {}", pat.id, pat.name), - body: format!( - "{} detected — mask with environment variable. Matched: {}", - pat.name, - mask_secret(matched) - ), - }); - if findings.len() >= max_findings { - break; - } + for (re, pat) in COMPILED.iter() { + if let Some(m) = re.find(content) { + let matched = m.as_str(); + findings.push(RuleFinding { + rule_id: pat.id.to_string(), + file: file_path.to_string(), + line: line_no, + severity: pat.severity, + title: format!("[{}] {}", pat.id, pat.name), + body: format!( + "{} detected — mask with environment variable. Matched: {}", + pat.name, + mask_secret(matched) + ), + }); + if findings.len() >= max_findings { + break; } } - if findings.len() >= max_findings { - break; - } } if findings.len() >= max_findings { break; @@ -229,7 +222,7 @@ fn floor_boundary(s: &str, target: usize) -> usize { #[cfg(test)] mod tests { use super::*; - use crate::engine::diff_parser::{DiffHunk, DiffLine}; + use crate::engine::diff_parser::{DiffHunk, DiffLine, DiffLineType}; fn make_chunk(file: &str, added_lines: &[&str]) -> FileChunk { FileChunk { diff --git a/src/engine/security_scanner.rs b/src/engine/security_scanner.rs index 42170e3..1622216 100644 --- a/src/engine/security_scanner.rs +++ b/src/engine/security_scanner.rs @@ -9,9 +9,10 @@ use std::sync::LazyLock; use tracing::debug; use crate::engine::Severity; -use crate::engine::diff_parser::{DiffLineType, FileChunk}; +use crate::engine::diff_parser::FileChunk; use crate::engine::rules::builtin::post_match_filter_for_path; use crate::engine::rules::types::RuleFinding; +use crate::engine::scan_input::{self, ScanFile}; // ─── Security patterns ─── @@ -46,10 +47,13 @@ pub static PATTERNS: &[SecurityPattern] = &[ id: "crypto/hardcoded-secret", name: "Hardcoded password or secret in variable", // Alternatives: (1) `name [: type] =|:` value, `:=` only before a quoted - // literal (Go `password := "..."`, never `:= os.Getenv(..)`, #618), key - // may be quoted (JSON, #619); (2) Go `var name type = value`; (3) SQL - // `PASSWORD ''` / `IDENTIFIED BY ''` (#620). - regex: r#"(?i)(?:password|passwd|pwd|secret|api_key|apikey|token)["']?(?:\s*:\s*[&\w<>\[\].?|]+)?\s*(?:[=:]|:=\s*["'`])\s*\S{8,}|(?:password|passwd|pwd|secret|api_key|apikey|token)["']?\s+[&*\w<>\[\].?|]+\s*=\s*["'`]\S{7,}|\b(?:password|identified\s+by)\s+'[^']+'"#, + // literal (Go `password := "..."`, never `:= os.Getenv(..)` and never an + // unspaced `password:=os.Getenv(..)`, #618/#625), key may be quoted (JSON, + // #619), or a quoted literal inside a constructor call such as + // `new String("...")` whose callee is too short for the 8-char token (#630); + // (2) Go `var name type = value`; (3) SQL `PASSWORD ''` / + // `IDENTIFIED BY ""` (#620, #625). + regex: r#"(?i)(?:password|passwd|pwd|secret|api_key|apikey|token)["']?(?:\s*:\s*[&\w<>\[\].?|]+)?\s*(?:(?:=|:=\s*["'`])\s*\S{8,}|:\s*[^=\s]\S{7,}|(?:=|:=)\s*(?:new\s+)?[A-Za-z_][\w:.]*\s*\(\s*["'`][^"'`]+["'`])|(?:password|passwd|pwd|secret|api_key|apikey|token)["']?\s+[&*\w<>\[\].?|]+\s*=\s*["'`]\S{7,}|\b(?:password|identified\s+by)\s+\\?["'][^"']+["']"#, severity: Severity::Critical, }, // ── Injection ── @@ -157,19 +161,22 @@ static COMPILED_PATTERNS: LazyLock> = Laz .collect() }); -/// Run static security scan on diff chunks. +/// Run static security scan on the added lines of diff chunks (diff adapter +/// over [`scan_security_in`]). /// /// Only scans added lines (not removed/context) to reduce false positives. /// Returns findings sorted by severity (worst first). pub fn scan_security(chunks: &[FileChunk], max_findings: usize) -> Vec { + scan_security_in(&scan_input::from_chunks(chunks), max_findings) +} + +/// Run the static security scan over file lines. The one implementation behind +/// both the diff and the whole-file entry points. +pub fn scan_security_in(files: &[ScanFile<'_>], max_findings: usize) -> Vec { let mut findings = Vec::new(); - for chunk in chunks { - let path = chunk - .new_path - .as_deref() - .or(chunk.old_path.as_deref()) - .unwrap_or("unknown"); + for file in files { + let path = file.path; // Skip the noisy general rules for test/spec/fixture/mock/example files // and for documentation (security patterns are designed for source code, @@ -181,7 +188,7 @@ pub fn scan_security(chunks: &[FileChunk], max_findings: usize) -> Vec= max_findings { findings.sort_by_key(|f| std::cmp::Reverse(f.severity)); return findings; @@ -189,52 +196,45 @@ pub fn scan_security(chunks: &[FileChunk], max_findings: usize) -> Vec= max_findings { - findings.sort_by_key(|f| std::cmp::Reverse(f.severity)); - return findings; - } + continue; + } + + debug!( + rule = %rule_id, + file = path, + line = line_no, + "security pattern match" + ); + findings.push(RuleFinding { + rule_id: rule_id.clone(), + file: path.to_string(), + line: line_no, + severity: *severity, + title: name.clone(), + body: format!( + "Static security scanner detected: {} in {}:{}", + name, path, line_no + ), + }); + + if findings.len() >= max_findings { + findings.sort_by_key(|f| std::cmp::Reverse(f.severity)); + return findings; } } } @@ -245,41 +245,35 @@ pub fn scan_security(chunks: &[FileChunk], max_findings: usize) -> Vec, findings: &mut Vec, max_findings: usize, ) { - for hunk in &chunk.chunks { - for line in &hunk.lines { - if line.line_type != DiffLineType::Add { - continue; - } - let line_no = line.new_line_no.unwrap_or(0); - if line_no == 0 { - continue; - } - // Placeholder keys (e.g. AKIAIOSFODNN7EXAMPLE) are ignored inside. - if crate::engine::secret_patterns::find_high_confidence(&line.content).is_none() { - continue; - } - findings.push(RuleFinding { - rule_id: "secrets/high-confidence-in-test-or-doc".to_string(), - file: path.to_string(), - line: line_no, - severity: Severity::Major, - title: "Credential-like secret in test/doc file".to_string(), - body: format!( - "Static security scanner detected a credential-like value in {path}:{line_no}. \ - Test/doc paths are not exempt from secret scanning; verify it is a fake." - ), - }); - if findings.len() >= max_findings { - return; - } + let path = file.path; + for &(line_no, content) in &file.lines { + if line_no == 0 { + continue; + } + // Placeholder keys (e.g. AKIAIOSFODNN7EXAMPLE) are ignored inside. + if crate::engine::secret_patterns::find_high_confidence(content).is_none() { + continue; + } + findings.push(RuleFinding { + rule_id: "secrets/high-confidence-in-test-or-doc".to_string(), + file: path.to_string(), + line: line_no, + severity: Severity::Major, + title: "Credential-like secret in test/doc file".to_string(), + body: format!( + "Static security scanner detected a credential-like value in {path}:{line_no}. \ + Test/doc paths are not exempt from secret scanning; verify it is a fake." + ), + }); + if findings.len() >= max_findings { + return; } } } @@ -339,7 +333,7 @@ mod tests { use super::*; fn make_chunk(file: &str, added_lines: &[&str]) -> FileChunk { - use crate::engine::diff_parser::{DiffHunk, DiffLine}; + use crate::engine::diff_parser::{DiffHunk, DiffLine, DiffLineType}; FileChunk { old_path: None, new_path: Some(file.to_string()), diff --git a/src/engine/types.rs b/src/engine/types.rs index db9e299..bce81a8 100644 --- a/src/engine/types.rs +++ b/src/engine/types.rs @@ -485,6 +485,11 @@ pub struct ReviewResponse { pub summary: String, pub tokens_used: Option, pub should_block: bool, + /// Deterministic findings cut by `rules_engine.max_findings` after + /// suppression (#624). Never serialized, so JSON/SARIF output is + /// unchanged; the CLI reports it on stderr. + #[serde(skip)] + pub dropped_findings: usize, } /// Response from a full project scan diff --git a/src/formatters/compact.rs b/src/formatters/compact.rs index ccacb59..d7f9014 100644 --- a/src/formatters/compact.rs +++ b/src/formatters/compact.rs @@ -99,6 +99,7 @@ mod tests { summary: "Summary".to_string(), tokens_used: None, should_block: false, + dropped_findings: 0, } } @@ -108,6 +109,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: false, + dropped_findings: 0, } } diff --git a/src/formatters/json_fmt.rs b/src/formatters/json_fmt.rs index d5c3546..88798f0 100644 --- a/src/formatters/json_fmt.rs +++ b/src/formatters/json_fmt.rs @@ -65,6 +65,7 @@ mod tests { estimated_cost_usd: 0.005, }), should_block: false, + dropped_findings: 0, } } @@ -74,6 +75,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: false, + dropped_findings: 0, } } diff --git a/src/formatters/pretty.rs b/src/formatters/pretty.rs index a3a5734..87e5cba 100644 --- a/src/formatters/pretty.rs +++ b/src/formatters/pretty.rs @@ -254,6 +254,7 @@ mod tests { summary: "Found 1 critical issue.".to_string(), tokens_used: None, should_block: false, + dropped_findings: 0, } } @@ -263,6 +264,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: false, + dropped_findings: 0, } } @@ -272,6 +274,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: true, + dropped_findings: 0, } } diff --git a/src/formatters/sarif.rs b/src/formatters/sarif.rs index 0fa7570..898d902 100644 --- a/src/formatters/sarif.rs +++ b/src/formatters/sarif.rs @@ -166,6 +166,7 @@ mod tests { summary: "Found 1 critical issue.".to_string(), tokens_used: None, should_block: false, + dropped_findings: 0, } } @@ -175,6 +176,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: false, + dropped_findings: 0, } } @@ -327,6 +329,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: false, + dropped_findings: 0, }; let fmt = SarifFormatter; let output = fmt.format_review(&response).unwrap(); diff --git a/src/index/session.rs b/src/index/session.rs index 804a086..a774237 100644 --- a/src/index/session.rs +++ b/src/index/session.rs @@ -121,6 +121,14 @@ impl IndexSession { Self::from_bridge(bridge, load_config(source).as_ref()) } + /// [`Self::open_at`] against an explicit database file instead of the + /// process-global data dir (tests: a per-test temp DB, #610). + #[cfg(test)] + pub fn open_at_db(db_path: &Path, start: &Path, source: ConfigSource<'_>) -> Result { + let bridge = IndexBridge::open_or_create_at(db_path, start)?; + Self::from_bridge(bridge, load_config(source).as_ref()) + } + /// Build a session from an already-open bridge and loaded config. pub fn from_bridge(bridge: IndexBridge, config: Option<&Config>) -> Result { let (conn, project_id, root) = bridge.into_strict_parts()?; diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index 00250cf..3e9b359 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -4,10 +4,10 @@ use std::sync::LazyLock; -use crate::engine::diff_parser; +use crate::engine::postprocess::{Source, postprocess_report}; use crate::engine::profiles; use crate::engine::rules; -use crate::engine::secrets_scanner; +use crate::engine::scanner::FileEntry; use crate::engine::security_scanner; use super::protocol::{Tool, ToolResult}; @@ -314,44 +314,64 @@ fn handle_check_snippet(params: &serde_json::Value) -> ToolResult { .and_then(|v| v.as_str()) .unwrap_or("unknown"); - // Create a fake diff chunk from the snippet - let fake_diff = format!( - "diff --git a/snippet.{lang} b/snippet.{lang}\n--- a/snippet.{lang}\n+++ b/snippet.{lang}\n@@ -0,0 +1,{count} @@\n{added}", - count = code.lines().count(), - added = code - .lines() - .map(|l| format!("+{l}")) - .collect::>() - .join("\n"), - ); + // Same project config the other tools use, so `ignore.rules` and + // `rules_engine.max_findings` apply exactly as in `cora scan` (#610). + let config = match load_project_config() { + Ok(c) => c, + Err(e) => return ToolResult::error(format!("Failed to load config: {e}")), + }; - let chunks = diff_parser::parse_diff(&fake_diff); + check_snippet_with(code, lang, &config) +} - let mut findings = Vec::new(); +/// Check `code` through the shared post-processing pipeline (scanners, +/// `ignore.rules`, inline `cora-ignore:` markers, finding cap) as a synthetic +/// file `snippet.`, so path-aware classification sees the snippet's +/// language/extension. +fn check_snippet_with( + code: &str, + lang: &str, + config: &crate::config::schema::Config, +) -> ToolResult { + // The language becomes a file extension: keep it a single safe path segment. + let ext: String = lang + .trim_start_matches('.') + .chars() + .map(|c| if c.is_ascii_alphanumeric() { c } else { '_' }) + .collect(); + let entry = FileEntry { + path: format!("snippet.{ext}"), + content: code.to_string(), + lines: code.lines().count(), + }; - // Run secrets scanner - let secrets = secrets_scanner::scan_secrets(&chunks, 10); - findings.extend(secrets); + let report = postprocess_report( + Vec::new(), + &Source::Files(std::slice::from_ref(&entry)), + config, + ); - // Run security scanner - let security = security_scanner::scan_security(&chunks, 10); - findings.extend(security); + if report.issues.is_empty() { + return ToolResult::text("✅ No issues found in snippet by deterministic scanners."); + } - if findings.is_empty() { - ToolResult::text("✅ No issues found in snippet by deterministic scanners.") - } else { - let mut lines = vec![format!("Found {} issue(s):\n", findings.len())]; - for f in &findings { - lines.push(format!( - "- **{}** ({}): {} — {}", - f.rule_id, - f.severity.label(), - f.title, - f.body - )); - } - ToolResult::text(lines.join("\n")) + let mut lines = vec![format!("Found {} issue(s):\n", report.issues.len())]; + for f in &report.issues { + lines.push(format!( + "- **{}** ({}): {} — {}", + f.rule_id.as_deref().unwrap_or(""), + f.severity.label(), + f.title, + f.body + )); } + if report.dropped > 0 { + lines.push(format!( + "\n{} more deterministic finding(s) not shown (rules.max_findings = {}; raise it or set 0/null to show all)", + report.dropped, config.rules_config.max_findings + )); + } + ToolResult::text(lines.join("\n")) } fn handle_get_quality_gate() -> ToolResult { @@ -1148,6 +1168,93 @@ mod tests { assert!(result.content[0].text.contains("issue")); } + // ─── check_snippet through the shared pipeline (#610) ─── + // These call `check_snippet_with` with an explicit Config: no project + // config discovery, no data dir. + + fn snippet_text(code: &str, lang: &str, config: &crate::config::schema::Config) -> String { + let r = check_snippet_with(code, lang, config); + assert!(!r.is_error); + r.content[0].text.clone() + } + + const AWS: &str = "key = 'AKIAIOSFODNN7EXAMPLE'"; + + #[test] + fn snippet_reports_secret_with_rule_id() { + let cfg = crate::config::schema::Config::default(); + let text = snippet_text(AWS, "py", &cfg); + assert!(text.contains("Found 1 issue(s)"), "{text}"); + assert!(text.contains("secrets/aws-access-key"), "{text}"); + } + + #[test] + fn snippet_honors_ignore_rules() { + let mut cfg = crate::config::schema::Config::default(); + cfg.ignore.rules = vec!["secrets/aws-access-key".to_string()]; + let text = snippet_text(AWS, "py", &cfg); + assert!(text.contains("No issues"), "{text}"); + } + + #[test] + fn snippet_honors_inline_cora_ignore() { + let cfg = crate::config::schema::Config::default(); + let same_line = format!("{AWS} # cora-ignore: secrets/aws-access-key"); + assert!(snippet_text(&same_line, "py", &cfg).contains("No issues")); + let next_line = format!("# cora-ignore: secrets/aws-access-key\n{AWS}"); + assert!(snippet_text(&next_line, "py", &cfg).contains("No issues")); + // A marker naming another rule does not hide it. + let other = format!("{AWS} # cora-ignore: crypto/md5-password"); + assert!(snippet_text(&other, "py", &cfg).contains("Found 1 issue(s)")); + } + + #[test] + fn snippet_applies_false_positive_filters() { + let cfg = crate::config::schema::Config::default(); + for line in [ + "password = os.environ[\"DB_PASSWORD\"]", + "const apiKey: string = process.env.API_KEY;", + "password: ${DB_PASSWORD}", + ] { + assert!( + snippet_text(line, "py", &cfg).contains("No issues"), + "{line}" + ); + } + // A literal still fires. + let hit = snippet_text("password = \"hunter2hunter2\"", "py", &cfg); + assert!(hit.contains("Found"), "{hit}"); + } + + #[test] + fn snippet_path_is_derived_from_language() { + let cfg = crate::config::schema::Config::default(); + let text = snippet_text("let p = \"x\";\nlet h = md5(password);", "rs", &cfg); + // The synthetic path carries the extension (shown in scanner bodies). + assert!(text.contains("snippet.rs"), "{text}"); + // Hostile language values cannot escape the single path segment. + let text = snippet_text("let h = md5(password);", "../../etc/x", &cfg); + assert!(!text.contains("../"), "{text}"); + } + + #[test] + fn snippet_respects_max_findings_and_reports_dropped() { + let mut cfg = crate::config::schema::Config::default(); + cfg.rules_config.max_findings = 1; + let code = "a = 'AKIAIOSFODNN7EXAMPLE'\nb = 'AKIAIOSFODNN7EXAMPLF'"; + let text = snippet_text(code, "py", &cfg); + assert!(text.contains("Found 1 issue(s)"), "{text}"); + assert!( + text.contains("1 more deterministic finding(s) not shown"), + "{text}" + ); + + cfg.rules_config.max_findings = 0; // unlimited + let text = snippet_text(code, "py", &cfg); + assert!(text.contains("Found 2 issue(s)"), "{text}"); + assert!(!text.contains("not shown"), "{text}"); + } + #[test] fn handle_check_snippet_missing_code() { let result = handle_tool_call("cora.check_snippet", &serde_json::json!({})); diff --git a/tests/brain_search_process.rs b/tests/brain_search_process.rs index 20b853d..09d3f0c 100644 --- a/tests/brain_search_process.rs +++ b/tests/brain_search_process.rs @@ -12,9 +12,14 @@ fn cora_cmd() -> Command { } /// Isolated CODECORA_HOME + tiny project, so tests never touch real data. -fn sandbox(name: &str) -> (PathBuf, PathBuf) { - let root = std::env::temp_dir().join(format!("cora-545-{name}")); - let _ = std::fs::remove_dir_all(&root); +/// Each call gets a unique temp dir (dropped with the returned guard), so two +/// concurrent `cargo test` runs cannot delete each other's sandbox. +fn sandbox(name: &str) -> (tempfile::TempDir, PathBuf, PathBuf) { + let guard = tempfile::Builder::new() + .prefix(&format!("cora-545-{name}-")) + .tempdir() + .unwrap(); + let root = guard.path().to_path_buf(); let proj = root.join("proj"); std::fs::create_dir_all(&proj).unwrap(); std::fs::write( @@ -29,7 +34,7 @@ fn sandbox(name: &str) -> (PathBuf, PathBuf) { .unwrap(); let home = root.join("home"); std::fs::create_dir_all(&home).unwrap(); - (proj, home) + (guard, proj, home) } fn run(cora: &mut Command) -> String { @@ -39,7 +44,7 @@ fn run(cora: &mut Command) -> String { #[test] fn brain_fresh_process_emits_vector_signal() { - let (proj, home) = sandbox("usearch"); + let (_guard, proj, home) = sandbox("usearch"); // 1. Index in one process (default usearch backend). run(cora_cmd() @@ -67,7 +72,7 @@ fn brain_fresh_process_emits_vector_signal() { #[test] fn brain_vecq_backend_uses_own_extension() { - let (proj, home) = sandbox("vecq"); + let (_guard, proj, home) = sandbox("vecq"); std::fs::write(proj.join(".cora.yaml"), "brain:\n vector_store: vecq\n").unwrap();