diff --git a/.github/actions/bootintel-scan/action.yml b/.github/actions/bootintel-scan/action.yml index 1a7ab1a..915a3eb 100644 --- a/.github/actions/bootintel-scan/action.yml +++ b/.github/actions/bootintel-scan/action.yml @@ -27,6 +27,18 @@ # # For full server-side analysis (CVE matching, exploit paths, PDF), # add `--api` to your own workflow step with a $BOOTINTEL_API_KEY secret. +# +# Exit codes this step can fail with: +# +# 1 a critical-exposure detector fired (with gate-critical: true) +# 2 the log was empty — nothing was inspected +# 3 the log had content but matched no detector +# +# 2 and 3 are failures on purpose. A capture that never happened (the +# UART did not come up, the adapter fell out, the artifact path was +# wrong) used to report green, which is the worst possible answer for a +# gate. If your logs legitimately contain nothing bootintel recognizes, +# set gate-critical: false and inspect `analysis-status` yourself. name: bootintel-scan description: Run BootIntel client-side detectors against a boot log, optionally gating on critical exposure findings. @@ -61,6 +73,9 @@ outputs: findings-count: description: Number of findings in the report (only populated when format=json). value: ${{ steps.run.outputs.findings-count }} + analysis-status: + description: "'matched', 'unrecognized', or 'empty' — whether the capture was actually inspected and whether anything was recognized." + value: ${{ steps.run.outputs.analysis-status }} runs: using: composite @@ -107,6 +122,13 @@ runs: echo "findings-json=$out" >> "$GITHUB_OUTPUT" + case "$rc" in + 2) status=empty ;; + 3) status=unrecognized ;; + *) status=matched ;; + esac + echo "analysis-status=$status" >> "$GITHUB_OUTPUT" + if [ "${{ inputs.format }}" = "json" ]; then if command -v jq >/dev/null; then count=$(jq '.findings | length' "$out" 2>/dev/null || echo 0) @@ -118,4 +140,14 @@ runs: if [ "${{ inputs.format }}" != "junit" ]; then cat "$out" || true fi + + # Say plainly why the step failed. Exit 2 and 3 mean the scan + # did not actually inspect anything; without this the run log + # shows a bare non-zero exit and a reader assumes a detector + # fired. + case "$rc" in + 2) echo "::error title=Empty capture::The boot log at '${{ inputs.log-file }}' was empty, so nothing was inspected. This is not a passing scan — check that the capture step ran and that the path is correct." ;; + 3) echo "::error title=Nothing recognized::The boot log at '${{ inputs.log-file }}' had content but matched none of bootintel's detectors. Common causes: the capture started after the boot banner, or the baud rate was wrong." ;; + esac + exit $rc diff --git a/.github/workflows/cli-release.yml b/.github/workflows/cli-release.yml index 40ff628..d18da79 100644 --- a/.github/workflows/cli-release.yml +++ b/.github/workflows/cli-release.yml @@ -14,6 +14,8 @@ # the "Publish release" button. # 4. Computes a combined SHA256SUMS file across all binaries so # install.sh can verify. +# 5. Generates a signed build-provenance attestation for every +# released artifact (see "Provenance" below). # # What it does NOT do: # - No auto-publish to Homebrew tap / winget / apt / Nix registry. @@ -45,8 +47,32 @@ on: type: boolean default: false +# Provenance +# ---------- +# Every released artifact gets a signed build-provenance attestation +# via actions/attest-build-provenance. That is a Sigstore signature +# over the artifact digest plus a SLSA statement recording which +# workflow, at which commit, on which runner produced it — so a user +# can verify an artifact really came from this repository's release +# workflow and not from someone's laptop: +# +# gh attestation verify bootintel-v0.3.2-x86_64-linux.tar.gz \ +# --repo BootIntel/cli +# +# It is free for public repositories, needs no key material of our own +# (keyless: the runner's OIDC identity is what gets signed, and the +# certificate lives in the public Rekor transparency log), and gives a +# stronger guarantee than a paid code-signing certificate would for +# this audience — it binds the artifact to a *build*, not merely to an +# organisation that paid a CA. +# +# SHA256SUMS stays exactly as it was: it answers "did this download +# arrive intact", which is a different question and still the one +# install.sh asks. permissions: contents: write # needed to create the release draft + id-token: write # OIDC identity for keyless attestation signing + attestations: write # write the attestation to the repo's store jobs: build: @@ -152,6 +178,19 @@ jobs: fi cat ${{ matrix.asset_name }}.sha256 + # Attest the archive we are about to ship. Runs per-matrix-leg so + # each platform's artifact is attested on the runner that built + # it, which is what makes the provenance meaningful. + # + # Skipped on dry runs: a dry run produces nothing anyone will + # download, and attesting it would put a misleading entry in the + # public transparency log. + - name: Attest build provenance + if: '!inputs.dry_run' + uses: actions/attest-build-provenance@v2 + with: + subject-path: ${{ matrix.asset_name }} + - name: Upload artifact uses: actions/upload-artifact@v4 with: @@ -194,6 +233,29 @@ jobs: echo "─────────────────────────────────────" cat release-assets/SHA256SUMS + - name: Write release notes + run: | + cat > RELEASE_NOTES.md <<'NOTES' + Release notes: see CHANGELOG.md. + + This is a DRAFT — publish manually after verifying artifacts. + + ## Verify what you downloaded + + Checksums are in `SHA256SUMS`: + + sha256sum -c SHA256SUMS --ignore-missing + + Every archive also carries a signed build-provenance attestation, + so you can confirm it was produced by this repository's release + workflow rather than by someone's laptop: + + gh attestation verify --repo BootIntel/cli + NOTES + # Strip the leading indentation the YAML block required. + sed -i 's/^ //' RELEASE_NOTES.md + cat RELEASE_NOTES.md + - name: Create GH Release DRAFT env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -201,7 +263,7 @@ jobs: gh release create "cli-v${{ inputs.version }}" \ --draft \ --title "bootintel-cli v${{ inputs.version }}" \ - --notes "Release notes: see CHANGELOG.md. This is a DRAFT — publish manually after verifying artifacts." \ + --notes-file RELEASE_NOTES.md \ release-assets/* docker: @@ -212,6 +274,8 @@ jobs: permissions: contents: read packages: write + id-token: write + attestations: write steps: - uses: actions/checkout@v4 @@ -226,6 +290,7 @@ jobs: uses: docker/setup-buildx-action@v3 - name: Build + push (linux/amd64, linux/arm64) + id: docker_build uses: docker/build-push-action@v6 with: context: . @@ -237,3 +302,9 @@ jobs: ghcr.io/bootintel/cli:latest cache-from: type=gha cache-to: type=gha,mode=max + - name: Attest image provenance + uses: actions/attest-build-provenance@v2 + with: + subject-name: ghcr.io/bootintel/cli + subject-digest: ${{ steps.docker_build.outputs.digest }} + push-to-registry: true diff --git a/CHANGELOG.md b/CHANGELOG.md index f812d63..2c00cc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,107 @@ All notable changes to bootintel-cli are documented here. Format follows [Keep a ## [Unreleased] -_No unreleased changes since 0.3.1._ +Correctness batch from an SME review that installed the v0.3.1 release +binary and exercised it against socat PTY pairs. Every item below was +reproduced by running, not by reading. + +**This batch changes exit-code policy** (see MAJOR in the versioning +policy above): `scan` now exits 2 for an empty/unusable capture and 3 +when nothing was recognized, where it previously exited 0 for both. + +### Fixed +- **`--log-file` no longer loses the capture.** Bytes were buffered and + flushed only on `Drop`, so a capture smaller than the 8 KiB buffer — + which is most of them — reached the filesystem only if the session + quit through the clean path. Measured on the v0.3.1 binary: the log + file was 0 bytes at 2, 4, 6, 8 and 10 seconds into a live session and + 0 bytes after both SIGINT and SIGTERM, while the session correctly + analyzed those same bytes on screen. Writes are now flushed as they + arrive, which also makes the file `tail -f`-able from another + terminal. SIGINT/SIGTERM/SIGHUP handlers were added so the terminal + exits through its normal path — raw mode restored, capture flushed — + instead of the process dying where it stands. +- **An empty capture no longer passes `--gate-critical` with exit 0.** A + CI job whose UART never came up, whose adapter fell out, or whose + artifact path was wrong reported green. `scan` now follows the legacy + Node analyzer's ladder: 2 for empty or unusable input, 3 for + non-empty input that matched no detector. +- **Line-anchored detectors no longer die on a line prefix.** A plain + `U-Boot 2020.10` line was detected, but the same line behind a + `[12:34:56.789]` prefix, an ISO-8601 timestamp, or an ANSI colour + escape produced no findings and exit 0. This was self-inflicted: + `bootintel analyze --log-timestamps` prefixes every line with an + ISO-8601 timestamp, so the tool's own capture mode broke its own + `scan`. Lines are now normalized (ANSI CSI stripper + bracketed + timestamp stripper, ported from the Node analyzer) before matching, + while evidence still reports the original line verbatim. +- **A non-UTF-8 byte is no longer a hard error.** A capture containing + `\xff\xfe\x80\x81\xc0\xc1` failed with "stream did not contain valid + UTF-8" and exit 1, while the Node analyzer read the same file and + returned findings. That is the normal shape of a real UART capture + (pre-baud-lock noise, framing errors, a binary splash, a reset + mid-line) — and since `--log-file` writes raw bytes, `analyze + --log-file` followed by `scan` could fail on the tool's own output. + Logs are now read as bytes and decoded lossily; `-v` reports how many + bytes were replaced. Line numbers are unaffected. +- **A closed pipe is one silent outcome instead of three.** `bootintel + batch … --format json | head -2` gave exit 1 plus `Error: Broken pipe + (os error 32)` on 6 of 6 runs when output exceeded the 64 KiB pipe + buffer, while smaller outputs raced between 141 and 0. `bootintel + manpage | head` — a packager's first command — hit the same thing. + The broken-pipe check now walks the whole `anyhow` cause chain and + understands `serde_json::Error`, which is how the error actually + arrived, and the result is a silent exit 0 every time. +- **SARIF names the real input file.** Every result carried + `artifactLocation.uri = "boot.log"` regardless of the input, so the + SARIF upload action attached findings to a file not in the repository + and the annotations landed nowhere. The real path is now emitted, + relative to `$GITHUB_WORKSPACE` (or the working directory) where + possible, and `stdin` for piped input. `startLine` now comes from the + detector library rather than a substring search. +- **`-q` quiets, and the upsell is off stdout.** `scan --format text -q` + still printed a three-line block advertising `--api`, on stdout — so + `scan --format text > report.txt` shipped marketing inside a + customer's report, and `-q` did nothing despite its own help text + promising it suppresses banners and status hints. The summary block + now goes to stderr and honours `-q`; stdout carries findings only. +- **`bootintel cve` works when installed.** It resolved + `./data/embedded-cves-feed.json` relative to the working directory, so + it only ever worked from inside a source checkout. The feed is now + read from the platform state dir, populated by a new `--refresh` that + fetches the public feed. Repo-relative paths are still tried last. +- **`bootintel analyze /etc/hostname` is diagnosed correctly.** A + readable regular file reported "permission denied" and advised adding + the user to the `dialout` group. It now says the path is not a serial + device and points at `scan` / `watch`. +- **The non-TTY error has a recovery hint.** `entering terminal raw + mode: No such device or address` was the only error in the CLI that + arrived with no suggested next step. + +### Changed +- `bootintel ports` sorts USB adapters first and annotates them with the + manufacturer/product string. Previously 32 bare `/dev/ttyS*` paths + came back in enumeration order with nothing to distinguish the one + adapter the user was looking for. +- `bootintel scan`'s local history write is disclosed on first use — one + stderr notice naming the file, what it records, and how to turn it + off. It remains on by default and entirely local; it was simply never + announced, which sits badly with a tool whose pitch is that it uploads + nothing. + +### Added +- `analysis_status` (`matched` / `unrecognized`) on the `scan --format + json` envelope, and `line_number` on each finding. Both are additive; + no existing key changed name or meaning. `bootintel schema` and the + bundled GitHub Action are updated to match. +- Signed build-provenance attestations + (`actions/attest-build-provenance`) for every release artifact and for + the container image. Free for public repositories, keyless, and a + stronger claim than a paid signing certificate — it binds an artifact + to the workflow and commit that built it. `SHA256SUMS` is unchanged. +- Regression tests for each of the above, including four that assert the + log file is non-zero **mid-session** (the pre-existing + flush-on-drop test passed against the broken code). ## [0.3.1] — 2026-08-30 — security-hygiene + refactor + dep bumps diff --git a/README.md b/README.md index e55e2db..f1721fe 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,10 @@ Use `bootintel term` when you want a clean terminal and `bootintel analyze` when - Local detection runs on your machine and never requires an account. - BootIntel does not send serial input automatically. Any write, break, modem-line action, or full analysis is initiated by you. -- `--log-file` writes raw capture bytes only to the path you choose. +- `--log-file` writes raw capture bytes only to the path you choose. Bytes + are flushed as they arrive, so the capture survives Ctrl-C, a closed + terminal window, an unplugged adapter or a suspended laptop — and you can + `tail -f` it from another terminal while the session runs. - Server analysis is opt-in: use `--api` or `--api --preview`; the CLI states when it is submitting a log. - Release artifacts include SHA256 checksums. See [SECURITY.md](SECURITY.md) for reporting guidance. @@ -111,6 +114,26 @@ Pin to a release tag (`@cli-v0.3.1`) or a commit SHA — **never `@main`** (a co **Windows:** the one-liner above downloads + SHA256-verifies the latest release, extracts `bootintel.exe` into `$env:USERPROFILE\.local\bin`, and prints a `setx PATH` line if that dir isn't already on your PATH. Override with `$env:BOOTINTEL_VERSION` / `$env:BOOTINTEL_INSTALL_DIR`, or use `$env:BOOTINTEL_TARBALL` for offline installs. Currently x86_64 only — ARM64 users need `cargo install --path crates/cli --features tui`. +### Verify a download + +Checksums for every release are in `SHA256SUMS`: + +```sh +sha256sum -c SHA256SUMS --ignore-missing +``` + +Every archive also carries a signed [build-provenance attestation](https://docs.github.com/actions/security-for-github-actions/using-artifact-attestations). That binds the artifact to the workflow, repository and commit that built it — a stronger statement than a code-signing certificate, which only says an organisation paid a CA: + +```sh +gh attestation verify bootintel-v0.3.1-x86_64-linux.tar.gz --repo BootIntel/cli +``` + +Container images are attested the same way: + +```sh +gh attestation verify oci://ghcr.io/bootintel/cli:latest --repo BootIntel/cli +``` + ## Build from source ``` @@ -181,14 +204,43 @@ Rate-limit UX: `HTTP 429` is never a silent fallback to client-side. The CLI pri | Exit | Meaning | Trigger | | --- | --- | --- | -| 0 | OK | success | -| 1 | gate-critical failure | `--gate-critical` and a critical finding fired | +| 0 | OK | the scan ran and recognized at least one thing | +| 1 | gate failure | `--gate-critical` and a critical finding fired; a failed `--gate` assertion; `--baseline` drift | +| 2 | empty / unusable input | the log was empty or whitespace only — **nothing was inspected** | +| 3 | nothing recognized | the log had content but matched no detector | | 65 | EX_DATAERR | server rejected the request body (log too large) | | 69 | EX_UNAVAILABLE | network / DNS / TLS failure | | 75 | EX_TEMPFAIL | rate-limited (429) | | 76 | EX_PROTOCOL | server 5xx or malformed response body | | 77 | EX_NOPERM | 401/403 auth failure | +### 2 and 3 exist so an empty capture can't pass a gate + +`scan` used to exit 0 on a 0-byte file, so `--gate-critical` reported +green for a CI job whose UART never came up, whose adapter fell out, or +whose artifact path was wrong. An empty capture is not a clean bill of +health — it is the absence of evidence, and the two are not the same +result. + +Codes 2 and 3 match the legacy Node analyzer's ladder, so a pipeline can +be pointed at either implementation and dispatch the same way. Treat +**any** non-zero exit as "do not merge": + +```sh +bootintel scan boot.log --format sarif --gate-critical > results.sarif +case $? in + 0) echo "clean" ;; + 1) echo "critical exposure found" ; exit 1 ;; + 2) echo "capture was empty — did the UART come up?" ; exit 1 ;; + 3) echo "nothing recognized — wrong baud, or capture started too late" ; exit 1 ;; + *) echo "scan failed" ; exit 1 ;; +esac +``` + +The `json` envelope carries the same answer in the added +`analysis_status` field (`matched` / `unrecognized`), so a consumer +parsing stdout does not have to infer it from an empty `findings` array. + Response schema is auto-follow with graceful degrade (Stripe-style): unknown fields are preserved via a passthrough map so a server-side schema addition never crashes an older CLI. See `api/response.rs` for the parser. ## TUI dashboard @@ -273,7 +325,10 @@ crates/cli/src/term/ ├── mod.rs — module glue ├── hotkey.rs — Ctrl-A escape state machine (pure, unit-tested) ├── raw_mode.rs — RAII guard that restores terminal on drop even under panic -├── logfile.rs — BufWriter for --log-file, error-tolerant on disk-full +├── logfile.rs — BufWriter for --log-file, flushed per read so the +│ capture is durable + tailable; error-tolerant on disk-full +├── signals.rs — SIGINT/SIGTERM/SIGHUP → graceful exit (restores the +│ terminal, flushes the capture) └── run.rs — main terminal loop (2 background threads + mpsc), takes optional analyzer ``` @@ -325,9 +380,22 @@ The tui loop reuses `AnalyzeState` + `ScanClient` + `ApiConfig` unchanged. When Downstream consumers reading `.findings[]` can rely on the array shape across releases. The envelope grows additively (new top-level fields never break parsing). +Two fields were added and no existing key changed name or meaning: + +| Field | Where | Meaning | +| --- | --- | --- | +| `analysis_status` | envelope | `matched` or `unrecognized` — pairs with exit codes 0 / 3 | +| `line_number` | each finding | 1-based line of `source` in the analyzed log; omitted when unknown | + +`source` now carries the **original** log line, prefix and all, rather +than only the substring the detector regex matched. Prefixed captures +(`[12:34:56.789] `, ISO-8601 timestamps, ANSI colour) are normalized +before matching, so a line-anchored detector still fires — but the +evidence shown back to you is what your capture actually contained. + SARIF v2.1.0 and JUnit XML outputs conform to their respective specs and validate against GitHub Code Scanning and standard `junit-report` consumers. -`--gate-critical` returns exit 0 (no critical exposures) or 1 (at least one — currently `Autoboot interruptable` or `Telnet exposure`). +`--gate-critical` returns exit 0 (no critical exposures) or 1 (at least one — currently `Autoboot interruptable` or `Telnet exposure`). It returns 2 for an empty capture and 3 when nothing was recognized; see the exit-code table above — an empty capture must never read as a passing gate. ## Detector sync discipline diff --git a/crates/cli/src/analyze/dedupe.rs b/crates/cli/src/analyze/dedupe.rs index af9d219..e7d1ac9 100644 --- a/crates/cli/src/analyze/dedupe.rs +++ b/crates/cli/src/analyze/dedupe.rs @@ -72,6 +72,7 @@ mod tests { value: value.to_string(), detail: None, source: None, + line_number: None, } } @@ -81,6 +82,7 @@ mod tests { value: value.to_string(), detail: None, source: Some(source.to_string()), + line_number: None, } } @@ -146,12 +148,14 @@ mod tests { value: "Linux 5.15".to_string(), detail: Some("gcc-11.2.0".to_string()), source: Some("Linux version 5.15…".to_string()), + line_number: None, }]; let b = vec![Finding { label: "Kernel".to_string(), value: "Linux 5.15".to_string(), detail: Some("gcc-11.2.0 (OpenWrt GCC 11.2.0)".to_string()), source: Some("Linux version 5.15…".to_string()), + line_number: None, }]; assert_eq!(d.new_findings(&a).len(), 1); assert_eq!(d.new_findings(&b).len(), 0); diff --git a/crates/cli/src/analyze/render.rs b/crates/cli/src/analyze/render.rs index 9484f50..918b616 100644 --- a/crates/cli/src/analyze/render.rs +++ b/crates/cli/src/analyze/render.rs @@ -160,6 +160,7 @@ mod tests { value: value.to_string(), detail: None, source: None, + line_number: None, } } diff --git a/crates/cli/src/cmd/cve.rs b/crates/cli/src/cmd/cve.rs index 62ca667..5cca235 100644 --- a/crates/cli/src/cmd/cve.rs +++ b/crates/cli/src/cmd/cve.rs @@ -1,16 +1,28 @@ //! `bootintel cve ` — look up a CVE in the local feed data. //! -//! Reads `data/embedded-cves-feed.json` (the same file the cron -//! rewrites every 4h and that the /feed/embedded-cves.* routes -//! serve) and prints the entry for the requested CVE, or lists all -//! entries if no ID is given. +//! Prints the entry for the requested CVE from a local copy of the +//! embedded-CVE feed (the same data the cron rewrites every 4h and +//! that the `/feed/embedded-cves.*` routes serve), or lists every +//! entry if no ID is given. //! //! The point isn't to replace `nvd` — it's to make the "which of my //! covered targets got a fresh CVE this week" data usable without a -//! browser, and to give the CLI a natural extension of the feed -//! ecosystem. Deliberately reads the LOCAL file (not the HTTPS -//! endpoint) so the command works offline / air-gapped once you've -//! pulled the data down. +//! browser. Lookups read the LOCAL file, never the network, so the +//! command works offline / air-gapped once the data is on disk. +//! +//! # Where the feed lives +//! +//! In the platform state dir — `~/.local/state/bootintel/` on Linux, +//! `~/Library/Application Support/bootintel/` on macOS, +//! `%LOCALAPPDATA%\bootintel\` on Windows — populated by +//! `bootintel cve --refresh`. +//! +//! This used to resolve `./data/embedded-cves-feed.json` relative to +//! the working directory, which meant the command worked only when run +//! from inside a checkout of the source repo and failed everywhere an +//! installed binary actually runs — while its own help implied the +//! feed shipped with the binary. The repo-relative paths are still +//! tried, last, so working in a checkout keeps behaving as before. use anyhow::{bail, Context, Result}; use clap::Args as ClapArgs; @@ -27,13 +39,20 @@ pub struct Args { #[arg(value_name = "ID")] id: Option, - /// Feed file to read. Defaults to $BOOTINTEL_CVE_FEED, or - /// ./data/embedded-cves-feed.json, or /data/embedded-cves-feed.json - /// (whichever exists first). Handy for pointing at a copy pulled - /// from an air-gapped mirror. + /// Feed file to read. Defaults to $BOOTINTEL_CVE_FEED, then the + /// cached copy in the platform state dir (see `--refresh`), then + /// ./data/embedded-cves-feed.json for in-repo use. Handy for + /// pointing at a copy pulled from an air-gapped mirror. #[arg(long, value_name = "PATH")] feed: Option, + /// Download the current feed from bootintel.com into the platform + /// state dir, then continue with the lookup. The feed is a moving + /// 72h window, so refresh before trusting an absence. Needs + /// network; everything else in this subcommand is offline. + #[arg(long)] + refresh: bool, + /// Filter by severity floor (CRITICAL, HIGH, MEDIUM). Only /// meaningful in list mode (no ID given). #[arg(long, value_name = "LEVEL")] @@ -84,16 +103,62 @@ struct Entry { posted_to_bluesky: Option, } +/// Public URL the feed is published at. Returns the same JSON the +/// `--feed` file holds. +const FEED_URL: &str = "https://bootintel.com/feed/embedded-cves.json"; + +/// Where a refreshed feed is cached. Same state dir `history` uses. +pub fn cached_feed_path() -> Option { + let root = dirs::state_dir().or_else(dirs::data_local_dir)?; + Some(root.join("bootintel").join("embedded-cves-feed.json")) +} + +/// Fetch the feed and write it to the cache path. Returns the path. +fn refresh_feed() -> Result { + let path = cached_feed_path() + .context("no platform state dir resolvable, so there is nowhere to cache the feed; use --feed PATH instead")?; + crate::vinfo!("fetching {FEED_URL}"); + let body = ureq::get(FEED_URL) + .call() + .with_context(|| format!("fetching {FEED_URL}"))? + .into_string() + .context("reading feed response body")?; + + // Parse before writing so a captive-portal HTML page or a + // truncated transfer can't replace a good cached copy with junk. + let parsed: FeedFile = + serde_json::from_str(&body).context("the feed URL did not return a valid feed document")?; + + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("creating {}", parent.display()))?; + } + std::fs::write(&path, &body).with_context(|| format!("writing {}", path.display()))?; + if !crate::verbose::is_quiet() { + eprintln!( + "[bootintel] feed refreshed — {} entries, generated {}, cached at {}", + parsed.count, + parsed.generated_at, + path.display() + ); + } + Ok(path) +} + pub fn run(args: Args) -> Result<()> { - let path = resolve_feed_path(&args.feed)?; + let path = if args.refresh { + refresh_feed()? + } else { + resolve_feed_path(&args.feed)? + }; let text = std::fs::read_to_string(&path) .with_context(|| format!("reading feed {}", path.display()))?; let feed: FeedFile = serde_json::from_str(&text).with_context(|| format!("parsing feed {}", path.display()))?; let stdout = io::stdout(); - let color_on = crate::output::resolve_color_mode(args.no_color, &stdout) - == crate::output::ColorMode::On; + let color_on = + crate::output::resolve_color_mode(args.no_color, &stdout) == crate::output::ColorMode::On; let mut out = stdout.lock(); @@ -160,6 +225,16 @@ fn resolve_feed_path(explicit: &Option) -> Result { if let Ok(env) = std::env::var("BOOTINTEL_CVE_FEED") { return Ok(PathBuf::from(env)); } + // The cached copy written by `--refresh`. This is the path that + // makes the command work for an installed binary, from any cwd. + if let Some(p) = cached_feed_path() { + if p.is_file() { + return Ok(p); + } + } + // Repo-relative fallbacks, last: convenient when hacking on the + // source tree or running the container image, but never the thing + // an installed binary should depend on. for candidate in [ "data/embedded-cves-feed.json", "/data/embedded-cves-feed.json", @@ -169,8 +244,16 @@ fn resolve_feed_path(explicit: &Option) -> Result { return Ok(p); } } + let cached = cached_feed_path() + .map(|p| p.display().to_string()) + .unwrap_or_else(|| "".to_string()); bail!( - "no CVE feed file found. Tried $BOOTINTEL_CVE_FEED, ./data/embedded-cves-feed.json, /data/embedded-cves-feed.json.\n Fetch a fresh copy: curl -o /tmp/feed.json https://bootintel.com/feed/embedded-cves.json\n Then: bootintel cve --feed /tmp/feed.json" + "no CVE feed on disk yet.\n \ + Download it:\n bootintel cve --refresh\n \ + That caches {FEED_URL} at {cached}.\n \ + Already have a copy (air-gapped mirror, CI artifact)? Point at it:\n \ + bootintel cve --feed /path/to/embedded-cves-feed.json\n \ + Or set $BOOTINTEL_CVE_FEED to the same path." ); } diff --git a/crates/cli/src/cmd/demo.rs b/crates/cli/src/cmd/demo.rs index 9e44e0a..b0114e7 100644 --- a/crates/cli/src/cmd/demo.rs +++ b/crates/cli/src/cmd/demo.rs @@ -42,7 +42,16 @@ pub fn run(args: Args) -> Result<()> { writeln!(out, "{SAMPLE}")?; writeln!(out, "── findings ──")?; } - output::write(&mut out, &findings, args.format, SAMPLE, color)?; + // The built-in sample has no on-disk path; name it as such + // so SARIF from `demo` never claims a real repository file. + output::write( + &mut out, + &findings, + args.format, + SAMPLE, + color, + "bootintel-sample-log", + )?; if !args.show_log && matches!(args.format, Format::Text) && !crate::verbose::is_quiet() { writeln!(out)?; writeln!( diff --git a/crates/cli/src/cmd/diff.rs b/crates/cli/src/cmd/diff.rs index ae98857..22c46c2 100644 --- a/crates/cli/src/cmd/diff.rs +++ b/crates/cli/src/cmd/diff.rs @@ -343,4 +343,3 @@ footer {{ margin-top: 2rem; padding-top: 1rem; border-top: 1px solid #eee; color )?; Ok(()) } - diff --git a/crates/cli/src/cmd/init.rs b/crates/cli/src/cmd/init.rs index 6665469..a4aa523 100644 --- a/crates/cli/src/cmd/init.rs +++ b/crates/cli/src/cmd/init.rs @@ -48,7 +48,8 @@ const MACROS_STUB: &str = "\ pub fn run(args: Args) -> Result<()> { let stdout = io::stdout(); - let color_on = crate::output::resolve_color_mode(false, &stdout) == crate::output::ColorMode::On; + let color_on = + crate::output::resolve_color_mode(false, &stdout) == crate::output::ColorMode::On; let (bold_open, bold_close) = if color_on { ("\x1b[1m", "\x1b[0m") } else { diff --git a/crates/cli/src/cmd/ports.rs b/crates/cli/src/cmd/ports.rs index 1bbf5c1..8270453 100644 --- a/crates/cli/src/cmd/ports.rs +++ b/crates/cli/src/cmd/ports.rs @@ -3,6 +3,19 @@ //! Enumerates via `serialport::available_ports()`. Text format shows //! path + USB VID:PID + product string; --json emits a structured //! array so scripts can filter on VID/PID or bus type without regex. +//! +//! # Ordering +//! +//! USB ports come first, then everything else, each group sorted +//! naturally by port number. +//! +//! This matters because `available_ports()` returns whatever order the +//! platform enumerated in, and on a typical Linux box that is 32 +//! motherboard `/dev/ttyS*` stubs in essentially random order. The one +//! port the user cares about — the USB adapter they just plugged in — +//! was somewhere in the middle of that wall of text with nothing to +//! distinguish it. The adapter is the answer to the question being +//! asked, so it goes at the top, labelled. use anyhow::{Context, Result}; use clap::Args as ClapArgs; @@ -21,8 +34,33 @@ pub struct Args { json: bool, } +/// Sort key: USB first, then by a natural ordering of the port name so +/// `/dev/ttyS9` sorts before `/dev/ttyS10` instead of after it. +fn sort_key(p: &serialport::SerialPortInfo) -> (u8, String, u32, String) { + let bus_rank = match &p.port_type { + SerialPortType::UsbPort(_) => 0, + SerialPortType::BluetoothPort => 1, + SerialPortType::PciPort => 2, + SerialPortType::Unknown => 3, + }; + // Split the trailing digit run off so numbers compare as numbers. + let name = &p.port_name; + let digits_start = name + .rfind(|c: char| !c.is_ascii_digit()) + .map(|i| i + 1) + .unwrap_or(0); + let (stem, num) = name.split_at(digits_start); + ( + bus_rank, + stem.to_string(), + num.parse::().unwrap_or(0), + name.clone(), + ) +} + pub fn run(args: Args) -> Result<()> { - let ports = serialport::available_ports().context("enumerating serial ports")?; + let mut ports = serialport::available_ports().context("enumerating serial ports")?; + ports.sort_by_key(sort_key); if args.json { let items: Vec = ports.iter().map(port_json).collect(); @@ -45,16 +83,42 @@ pub fn run(args: Args) -> Result<()> { } return Ok(()); } + let usb_count = ports + .iter() + .filter(|p| matches!(p.port_type, SerialPortType::UsbPort(_))) + .count(); + + // Pad the name column so the annotations line up into a readable + // second column rather than ragging off each path. + let width = ports + .iter() + .map(|p| p.port_name.chars().count()) + .max() + .unwrap_or(0) + .min(32); + + let mut printed_divider = false; for p in &ports { - print!("{}", p.port_name); + let is_usb = matches!(p.port_type, SerialPortType::UsbPort(_)); + // One blank line between the USB adapters and the built-in + // ports, so the interesting group reads as a group. + if !is_usb && usb_count > 0 && !printed_divider && !args.quiet { + println!(); + printed_divider = true; + } + print!("{:width$}", p.port_name); match &p.port_type { SerialPortType::UsbPort(info) => { print!(" USB {:04x}:{:04x}", info.vid, info.pid); - if let Some(m) = &info.manufacturer { - print!(" {m}"); - } - if let Some(prod) = &info.product { - print!(" / {prod}"); + // The product string is how a human recognises their + // adapter ("FT232R USB UART", "CP2102 USB to UART + // Bridge Controller"), so it is the part that must + // always show. + match (&info.manufacturer, &info.product) { + (Some(m), Some(prod)) => print!(" {m} / {prod}"), + (None, Some(prod)) => print!(" {prod}"), + (Some(m), None) => print!(" {m}"), + (None, None) => print!(" (USB serial adapter)"), } if let Some(sn) = &info.serial_number { print!(" SN={sn}"); @@ -62,10 +126,21 @@ pub fn run(args: Args) -> Result<()> { } SerialPortType::PciPort => print!(" (PCI serial)"), SerialPortType::BluetoothPort => print!(" (Bluetooth)"), - SerialPortType::Unknown => {} + // Not "unknown" to a human: on Linux these are the + // motherboard's 8250 stubs, which almost never have + // anything attached. + SerialPortType::Unknown => print!(" (built-in / no USB descriptor)"), } println!(); } + + if !args.quiet && usb_count > 0 && ports.len() > usb_count { + eprintln!(); + eprintln!( + " {usb_count} USB adapter(s) listed first; the remaining {} are built-in ports \n with nothing attached in the usual case.", + ports.len() - usb_count + ); + } Ok(()) } diff --git a/crates/cli/src/cmd/scan/api.rs b/crates/cli/src/cmd/scan/api.rs index aaea9e3..8705c22 100644 --- a/crates/cli/src/cmd/scan/api.rs +++ b/crates/cli/src/cmd/scan/api.rs @@ -1,7 +1,7 @@ //! `scan --api` — server-side path. POSTs the log to bootintel.com //! (or the configured `--api-base`) for CVE matching + exploit paths -//! + AI summary. Falls back to `--api --preview` for anonymous / -//! rate-limited free use. +//! + AI summary. Falls back to `--api --preview` for anonymous, +//! rate-limited free use. //! //! Split from mod.rs so the auth flow, error taxonomy, and //! sysexits-mapping are testable in isolation from the offline path. @@ -94,6 +94,7 @@ pub(super) fn run_api(args: &Args, raw: &str) -> Result<()> { args.format, raw, output::ColorMode::Off, + &super::source_label(args), )?; } } diff --git a/crates/cli/src/cmd/scan/baseline.rs b/crates/cli/src/cmd/scan/baseline.rs index 7827668..23012c0 100644 --- a/crates/cli/src/cmd/scan/baseline.rs +++ b/crates/cli/src/cmd/scan/baseline.rs @@ -85,11 +85,18 @@ pub(super) fn extract_findings_from_json(v: &serde_json::Value) -> Option Result<()> { - let raw = read_input(&args)?; + let log = read_input(&args)?; + log.report_replacements(); + let raw = log.text; + + // An empty capture is not a passing scan. + // + // Previously `scan --gate-critical` on a 0-byte file printed an + // empty finding list and exited 0, so every CI gate downstream went + // green on a capture that never happened. The legacy Node analyzer + // has always exited 2 here, and its README warns in as many words + // never to treat an empty capture as a successful gate. Checked + // before the --api branch as well: there is no point posting + // nothing to the server either. + if raw.trim().is_empty() { + eprintln!( + "bootintel: empty capture; no inspection performed ({})\n \ + Nothing was analyzed, so this is not a passing scan.\n \ + Check that the serial capture actually ran, that the adapter is still \ + attached, and that the path is the one your capture step wrote.", + log.source_label + ); + record_history(&args, 0, 0, EXIT_EMPTY_INPUT); + std::process::exit(EXIT_EMPTY_INPUT); + } if args.api { return api::run_api(&args, &raw); @@ -161,7 +196,14 @@ pub fn run(args: Args) -> Result<()> { let stdout = io::stdout(); let color = output::resolve_color_mode(args.no_color, &stdout); let mut out = stdout.lock(); - output::write(&mut out, &findings, args.format, &raw, color)?; + output::write( + &mut out, + &findings, + args.format, + &raw, + color, + &log.source_label, + )?; if args.context > 0 && matches!(args.format, Format::Text) { context::write_context_blocks(&mut out, &findings, &raw, args.context, color)?; } @@ -175,6 +217,26 @@ pub fn run(args: Args) -> Result<()> { .count() as u32; use std::io::Write as _; + + // Non-empty input, but no detector recognized anything. Same + // reasoning as the empty case: report it rather than calling it a + // clean bill of health. Ordered ahead of the gate checks to match + // the Node analyzer's ladder; with zero findings neither + // --gate-critical nor a positive --gate assertion can fire anyway. + if findings.is_empty() { + let _ = out.flush(); + eprintln!( + "bootintel: no recognized evidence in {}; this is not a successful inspection\n \ + The capture has content but matched none of the {} detectors. Common causes: \ + the capture started after the boot banner scrolled past, or the baud rate \ + was wrong and the bytes are noise.", + log.source_label, + bootintel_detectors::detector_labels().len() + ); + record_history(&args, 0, 0, EXIT_UNRECOGNIZED); + std::process::exit(EXIT_UNRECOGNIZED); + } + if args.gate_critical { let hit = findings .iter() @@ -205,6 +267,17 @@ pub fn run(args: Args) -> Result<()> { Ok(()) } +/// How to name this scan's input in output that has to identify it +/// (SARIF's `artifactLocation`, the empty/unrecognized messages). +/// `stdin` for a pipe, the path as the user typed it otherwise. +pub(super) fn source_label(args: &Args) -> String { + if args.stdin || args.file == "-" { + "stdin".to_string() + } else { + args.file.clone() + } +} + /// Append one history line for this scan. Best-effort — a locked file /// / read-only mount / disabled-via-env case never bubbles up to the /// caller (see `crate::history::append_entry`). @@ -238,13 +311,9 @@ pub(super) fn record_history(args: &Args, findings: u32, critical: u32, exit_cod }); } -fn read_input(args: &Args) -> Result { +fn read_input(args: &Args) -> Result { if args.stdin || args.file == "-" { - let mut buf = String::new(); - io::stdin() - .read_to_string(&mut buf) - .context("reading log from stdin")?; - return Ok(buf); + return crate::input::read_stdin(); } // Skip a pre-flight exists() check — that would misreport a // permission-denied file as "not found" (TOCTOU) and swallow @@ -252,7 +321,10 @@ fn read_input(args: &Args) -> Result { // io::Error, then translate the common kinds into an actionable // hint instead of the bare libc-style "No such file or directory". let path = PathBuf::from(&args.file); - std::fs::read_to_string(&path).map_err(|e| match e.kind() { + // Bytes, not `read_to_string`: a UART capture is frequently not + // valid UTF-8 and rejecting it outright loses a perfectly + // analyzable log. See `crate::input`. + crate::input::read_file(&path).map_err(|e| match e.kind() { std::io::ErrorKind::NotFound => anyhow::anyhow!( "no such file: {}\n Check the path, or pipe from stdin: bootintel scan - < path/to/log.txt", args.file diff --git a/crates/cli/src/cmd/schema.rs b/crates/cli/src/cmd/schema.rs index ddb6333..d5e02ff 100644 --- a/crates/cli/src/cmd/schema.rs +++ b/crates/cli/src/cmd/schema.rs @@ -66,6 +66,11 @@ fn scan_schema() -> serde_json::Value { "type": "array", "description": "Per-detector matches, in detector-registration order.", "items": { "$ref": "#/$defs/Finding" } + }, + "analysis_status": { + "type": "string", + "enum": ["matched", "unrecognized"], + "description": "Whether any detector recognized anything. Pairs with the process exit code: `matched` -> 0, `unrecognized` -> 3. An empty or unusable capture never reaches this document at all — it exits 2 before output. Lets a consumer distinguish 'clean' from 'we did not recognize this capture' without inferring it from an empty findings array." } }, "additionalProperties": false, @@ -88,7 +93,12 @@ fn scan_schema() -> serde_json::Value { }, "source": { "type": ["string", "null"], - "description": "Optional source line the detector matched against. Useful for finding provenance." + "description": "The original log line this finding came from, as it appeared in the capture — including any timestamp or ANSI prefix. Detector matching runs over a normalized copy, but evidence is reported verbatim so it can be found again in the log. May be absent." + }, + "line_number": { + "type": ["integer", "null"], + "minimum": 1, + "description": "1-based line number of `source` within the analyzed log. Absent when the evidence could not be located." } }, "additionalProperties": false @@ -118,10 +128,11 @@ mod tests { let props = &schema["$defs"]["Finding"]["properties"]; let keys: std::collections::HashSet<_> = props.as_object().unwrap().keys().cloned().collect(); - let expected: std::collections::HashSet = ["label", "value", "detail", "source"] - .iter() - .map(|s| s.to_string()) - .collect(); + let expected: std::collections::HashSet = + ["label", "value", "detail", "source", "line_number"] + .iter() + .map(|s| s.to_string()) + .collect(); assert_eq!( keys, expected, "Finding schema drifted from output::FindingOut — update BOTH" diff --git a/crates/cli/src/cmd/view.rs b/crates/cli/src/cmd/view.rs index bd7eb91..79553f0 100644 --- a/crates/cli/src/cmd/view.rs +++ b/crates/cli/src/cmd/view.rs @@ -63,7 +63,14 @@ pub fn run(args: Args) -> Result<()> { // heuristic uses it and gracefully degrades to line 1 when // it can't find the source string. let raw_hint = extract_raw_log(&parsed).unwrap_or_default(); - output::write(&mut out, &findings, args.format, &raw_hint, color)?; + output::write( + &mut out, + &findings, + args.format, + &raw_hint, + color, + &args.file.display().to_string(), + )?; } let _ = out.flush(); Ok(()) @@ -100,11 +107,18 @@ fn extract_findings(v: &serde_json::Value) -> Option> { .get("source") .and_then(|s| s.as_str()) .map(String::from); + // Additive field — archived envelopes written before + // line_number existed simply leave it None. + let line_number = item + .get("line_number") + .and_then(|n| n.as_u64()) + .map(|n| n as usize); out.push(Finding { label, value, detail, source, + line_number, }); } Some(out) diff --git a/crates/cli/src/cmd/watch.rs b/crates/cli/src/cmd/watch.rs index 5e040b5..3ec71eb 100644 --- a/crates/cli/src/cmd/watch.rs +++ b/crates/cli/src/cmd/watch.rs @@ -67,8 +67,8 @@ pub struct Args { pub fn run(args: Args) -> Result<()> { let stdout = io::stdout(); - let color_on = crate::output::resolve_color_mode(args.no_color, &stdout) - == crate::output::ColorMode::On; + let color_on = + crate::output::resolve_color_mode(args.no_color, &stdout) == crate::output::ColorMode::On; // Track inode so we can detect log rotation (a fresh file appears // at the same path with a different inode). On non-Unix, `dev` diff --git a/crates/cli/src/cmd/whoami.rs b/crates/cli/src/cmd/whoami.rs index 347e505..98afa2f 100644 --- a/crates/cli/src/cmd/whoami.rs +++ b/crates/cli/src/cmd/whoami.rs @@ -24,7 +24,6 @@ use serde::Deserialize; use std::io::{self, Write}; use std::time::Duration; - /// sysexits.h EX_NOPERM. const EX_NOPERM: i32 = 77; /// sysexits.h EX_UNAVAILABLE. @@ -540,11 +539,7 @@ mod tests { if path == "/api/whoami" { status(404, r#"{"detail":"not found"}"#) } else { - status_with_header( - 429, - &[("Retry-After", "42")], - r#"{"detail":"slow down"}"#, - ) + status_with_header(429, &[("Retry-After", "42")], r#"{"detail":"slow down"}"#) } }, 4, @@ -552,7 +547,10 @@ mod tests { let id = probe(&base, "bik_test_xyz9", Duration::from_millis(500)).unwrap(); assert_eq!(id.status, "authenticated (rate-limited)"); assert_eq!(id.rate_limited, Some(42)); - assert!(id.tier.is_none(), "tier must NOT be hijacked with retry-in-Ns"); + assert!( + id.tier.is_none(), + "tier must NOT be hijacked with retry-in-Ns" + ); } #[test] diff --git a/crates/cli/src/config.rs b/crates/cli/src/config.rs index 8aaca1a..76b4e2c 100644 --- a/crates/cli/src/config.rs +++ b/crates/cli/src/config.rs @@ -382,7 +382,6 @@ pub const ALL_KEYS: &[ConfigKey] = &[ ConfigKey::NoHistory, ]; - /// Resolve the effective `api_base` URL for a subcommand, layering: /// CLI flag > `$BOOTINTEL_API_BASE` env > config file > built-in default. /// @@ -636,8 +635,8 @@ mod tests { #[test] fn write_atomic_sets_0600_on_unix() { use std::os::unix::fs::PermissionsExt; - let tmp = std::env::temp_dir() - .join(format!("bootintel-perms-test-{}.toml", std::process::id())); + let tmp = + std::env::temp_dir().join(format!("bootintel-perms-test-{}.toml", std::process::id())); let _ = std::fs::remove_file(&tmp); write_atomic(&tmp, b"api_base = \"https://x\"\n").unwrap(); let mode = std::fs::metadata(&tmp).unwrap().permissions().mode() & 0o777; @@ -649,8 +648,7 @@ mod tests { #[test] fn restrict_dir_perms_sets_0700_on_unix() { use std::os::unix::fs::PermissionsExt; - let tmp = std::env::temp_dir() - .join(format!("bootintel-perms-dir-{}", std::process::id())); + let tmp = std::env::temp_dir().join(format!("bootintel-perms-dir-{}", std::process::id())); let _ = std::fs::remove_dir_all(&tmp); std::fs::create_dir_all(&tmp).unwrap(); restrict_dir_perms(&tmp).unwrap(); diff --git a/crates/cli/src/detector_filter.rs b/crates/cli/src/detector_filter.rs index 2c094d1..c34ed31 100644 --- a/crates/cli/src/detector_filter.rs +++ b/crates/cli/src/detector_filter.rs @@ -67,6 +67,7 @@ mod tests { value: "x".into(), detail: None, source: None, + line_number: None, } } diff --git a/crates/cli/src/gate.rs b/crates/cli/src/gate.rs index a480d23..8434f45 100644 --- a/crates/cli/src/gate.rs +++ b/crates/cli/src/gate.rs @@ -159,6 +159,7 @@ mod tests { value: value.into(), detail: None, source: None, + line_number: None, } } diff --git a/crates/cli/src/history.rs b/crates/cli/src/history.rs index 8093abf..dd461e6 100644 --- a/crates/cli/src/history.rs +++ b/crates/cli/src/history.rs @@ -22,6 +22,20 @@ //! (single generation) and truncate. A perfectly-behaved daily-use //! account would take years to hit this — the cap exists to keep a //! script-driven CI loop from silently filling the user's disk. +//! +//! # Disclosure +//! +//! bootintel's pitch is that it uploads nothing. That promise is kept +//! — nothing here leaves the machine — but "we write a record of every +//! scan you run to disk, including the path of every log you scanned" +//! is still a thing a privacy-conscious user is entitled to be told +//! rather than to discover. It was previously on by default and +//! announced nowhere. +//! +//! So the first time this file is created, we say so on stderr, once, +//! naming the path and how to turn it off. Subsequent runs are silent. +//! `-q` suppresses it like every other status hint, and the notice is +//! never printed when history is already disabled. use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; @@ -133,6 +147,9 @@ fn try_append(entry: &Entry) -> Result<()> { let mut line = serde_json::to_string(entry).context("serializing entry")?; line.push('\n'); let file_existed = path.exists(); + if !file_existed { + announce_first_write(&path); + } let mut file = open_history_for_append(&path)?; file.write_all(line.as_bytes()) .with_context(|| format!("writing to {}", path.display()))?; @@ -144,6 +161,27 @@ fn try_append(entry: &Entry) -> Result<()> { Ok(()) } +/// Tell the user, once, that we are keeping a local scan history. +/// +/// Called only when the history file is about to be created for the +/// first time. stderr, so it never lands in a redirected report. +fn announce_first_write(path: &std::path::Path) { + if crate::verbose::is_quiet() { + return; + } + eprintln!( + "[bootintel] note: recording a local scan history at {}", + path.display() + ); + eprintln!("[bootintel] One line per scan: timestamp, log path, finding counts, exit code."); + eprintln!("[bootintel] It stays on this machine — bootintel uploads nothing without --api."); + eprintln!("[bootintel] Turn it off with `bootintel config set no_history true`, or"); + eprintln!( + "[bootintel] per-run with BOOTINTEL_NO_HISTORY=1. Inspect it with `bootintel history`." + ); + eprintln!("[bootintel] This notice prints once."); +} + /// Open the history file for append. On Unix we use `.mode(0o600)` /// via OpenOptionsExt so the file is created owner-only from the /// start — closing the race between `open(create)` and a later chmod. diff --git a/crates/cli/src/input.rs b/crates/cli/src/input.rs new file mode 100644 index 0000000..99c9e87 --- /dev/null +++ b/crates/cli/src/input.rs @@ -0,0 +1,190 @@ +//! Reading boot logs off disk / stdin. +//! +//! # Why this is not `read_to_string` +//! +//! A UART capture is a byte stream, not a text file, and it is +//! routinely not valid UTF-8: +//! +//! * bytes received before the baud rate locks are framing garbage; +//! * a parity or framing error corrupts whatever byte it lands on; +//! * a vendor bootloader prints a binary splash or a raw memory dump; +//! * the board resets mid-line and the UART emits a break. +//! +//! `std::fs::read_to_string` rejects the whole file for any one of +//! those, so `bootintel scan` exited 1 with "stream did not contain +//! valid UTF-8" and analyzed nothing — on captures the legacy Node +//! analyzer reads without complaint. Worse, `--log-file` writes raw +//! bytes, so `bootintel analyze --log-file cap.log` followed by +//! `bootintel scan cap.log` could fail on the tool's own output. +//! +//! So: read bytes, convert lossily, and carry on. Every invalid +//! sequence becomes U+FFFD, exactly as `String::from_utf8_lossy` would, +//! but we also count how many bytes were replaced so `-v` can say so. +//! +//! # Line numbers +//! +//! Replacement never merges or splits lines: `\n` (0x0A) and `\r` +//! (0x0D) are single-byte ASCII and can never be part of an invalid +//! UTF-8 sequence, so they survive byte-for-byte. Line numbering over +//! the converted text therefore matches the original capture. + +use anyhow::{Context, Result}; +use std::io::Read; +use std::path::Path; + +/// A log that has been read and made safe to treat as text. +pub struct LoadedLog { + /// The log as UTF-8, with invalid sequences replaced. + pub text: String, + /// How many input bytes were not valid UTF-8 and got replaced. + pub replaced_bytes: usize, + /// What to show a user (and put in SARIF) as the input's identity. + /// The path as given for a file; `stdin` for a pipe. + pub source_label: String, +} + +impl LoadedLog { + /// Emit the `-v` note about replaced bytes, if there were any. + /// Goes to stderr like every other verbose line, so JSON/SARIF + /// consumers on stdout are unaffected. + pub fn report_replacements(&self) { + if self.replaced_bytes > 0 { + crate::vinfo!( + "{}: {} byte(s) were not valid UTF-8 and were replaced with U+FFFD \ + (normal for a UART capture: pre-baud-lock noise, framing errors, a binary splash)", + self.source_label, + self.replaced_bytes + ); + } + } +} + +/// Read every byte of stdin, lossily decoded. +pub fn read_stdin() -> Result { + let mut buf = Vec::new(); + std::io::stdin() + .read_to_end(&mut buf) + .context("reading log from stdin")?; + let (text, replaced_bytes) = from_utf8_lossy_counted(&buf); + Ok(LoadedLog { + text, + replaced_bytes, + source_label: "stdin".to_string(), + }) +} + +/// Read a file's bytes, lossily decoded. Error mapping is the caller's +/// job — `scan` and `batch` want different hints. +pub fn read_file(path: &Path) -> std::io::Result { + let bytes = std::fs::read(path)?; + let (text, replaced_bytes) = from_utf8_lossy_counted(&bytes); + Ok(LoadedLog { + text, + replaced_bytes, + source_label: path.display().to_string(), + }) +} + +/// `String::from_utf8_lossy`, but it also tells you how many bytes it +/// had to replace. +/// +/// Matches `from_utf8_lossy`'s replacement policy exactly: one U+FFFD +/// per maximal invalid subsequence, per the WHATWG Encoding Standard. +/// The count is of input *bytes* dropped, not of replacement chars — +/// "6 bytes were replaced" is what a user debugging a capture wants to +/// know, and it is what distinguishes one stray byte from a megabyte of +/// binary. +pub fn from_utf8_lossy_counted(bytes: &[u8]) -> (String, usize) { + // Fast path: the overwhelmingly common case allocates once and + // scans once. + if let Ok(s) = std::str::from_utf8(bytes) { + return (s.to_string(), 0); + } + + let mut out = String::with_capacity(bytes.len()); + let mut replaced = 0usize; + let mut rest = bytes; + loop { + match std::str::from_utf8(rest) { + Ok(valid) => { + out.push_str(valid); + break; + } + Err(e) => { + let valid_up_to = e.valid_up_to(); + // Safe by construction: from_utf8 just told us this + // prefix is valid. + out.push_str(std::str::from_utf8(&rest[..valid_up_to]).unwrap_or("")); + out.push('\u{FFFD}'); + match e.error_len() { + // A bad sequence of known length in the middle. + Some(len) => { + replaced += len; + rest = &rest[valid_up_to + len..]; + } + // Truncated sequence at end of input — nothing + // follows, so we are done. + None => { + replaced += rest.len() - valid_up_to; + break; + } + } + } + } + } + (out, replaced) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn valid_utf8_is_unchanged_and_counts_zero() { + let (s, n) = from_utf8_lossy_counted(b"U-Boot 2020.10\nhello\n"); + assert_eq!(s, "U-Boot 2020.10\nhello\n"); + assert_eq!(n, 0); + } + + #[test] + fn counts_every_invalid_byte() { + // The exact byte sequence from the SME's report. + let (s, n) = from_utf8_lossy_counted(b"a\xff\xfe\x80\x81\xc0\xc1b"); + assert_eq!( + n, 6, + "expected all 6 invalid bytes counted, got {n} ({s:?})" + ); + assert!(s.starts_with('a') && s.ends_with('b')); + } + + #[test] + fn matches_std_lossy_output_exactly() { + for case in [ + b"\xff\xfe\x80\x81\xc0\xc1".as_slice(), + b"ok\xffmid\xc3".as_slice(), + b"\xe2\x82".as_slice(), // truncated 3-byte sequence at EOF + b"plain".as_slice(), + b"\xf0\x9f\x92\xa9 emoji survives".as_slice(), + ] { + let (ours, _) = from_utf8_lossy_counted(case); + assert_eq!( + ours, + String::from_utf8_lossy(case), + "diverged from std on {case:?}" + ); + } + } + + #[test] + fn line_structure_survives_replacement() { + // Line numbers must stay correct: newlines can never be part + // of an invalid UTF-8 sequence. + let raw = b"line1\n\xff\xfe\nline3\n\x80line4\n"; + let (s, n) = from_utf8_lossy_counted(raw); + assert_eq!(n, 3); + assert_eq!(s.lines().count(), 4); + assert_eq!(s.lines().next().unwrap(), "line1"); + assert_eq!(s.lines().nth(2).unwrap(), "line3"); + assert!(s.lines().nth(3).unwrap().ends_with("line4")); + } +} diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index b9bce6c..59dc348 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -14,6 +14,7 @@ mod config; mod detector_filter; mod gate; mod history; +mod input; mod output; mod spinner; mod term; @@ -24,10 +25,35 @@ mod verbose; #[cfg(test)] mod test_util; -/// Exit code the CLI returns when its stdout is closed by a downstream -/// consumer (e.g. `bootintel scan foo.log | head -20`). Matches the -/// convention every real Unix filter uses — 141 = 128 + SIGPIPE(13). -const EXIT_SIGPIPE: i32 = 141; +/// Whether this error (or anything in its `anyhow` cause chain) is a +/// broken-pipe I/O error. +/// +/// Two shapes have to be recognised, and the old code caught neither +/// reliably: +/// +/// * `io::Error` behind any number of `.context(...)` wrappers — the +/// previous `err.downcast_ref::()` only ever inspected +/// the *outermost* error, so a single `.context("reading …")` hid +/// it; +/// * `serde_json::Error` wrapping an `io::Error` — every JSON/SARIF +/// path goes through `serde_json::to_writer_pretty`, so this is +/// the common case, and `serde_json::Error` does not downcast to +/// `io::Error` at all. Its `Display` is what produced the observed +/// `Error: Broken pipe (os error 32)`. +/// +/// `serde_json::Error::io_error_kind()` is the documented way to ask +/// the second question without string-matching the message. +pub fn is_broken_pipe(err: &anyhow::Error) -> bool { + err.chain().any(|cause| { + if let Some(io_err) = cause.downcast_ref::() { + return io_err.kind() == std::io::ErrorKind::BrokenPipe; + } + if let Some(json_err) = cause.downcast_ref::() { + return json_err.io_error_kind() == Some(std::io::ErrorKind::BrokenPipe); + } + false + }) +} #[derive(Parser)] #[command( @@ -183,15 +209,27 @@ fn main() -> Result<()> { Cmd::Version(args) => cmd::version::run(args), }; - // Graceful handling of a downstream consumer closing stdout mid- - // write (e.g. `bootintel scan foo.log | head -20`). Without this - // we'd panic on the underlying Broken-pipe io error, which is - // ugly + wrong for a Unix filter. Exit 141 per convention. + // A downstream consumer closing stdout mid-write (`bootintel + // manpage | head`, `bootintel batch … --format json | head -2`) is + // not an error — it is the reader saying "I have enough". + // + // The Rust runtime sets SIGPIPE to SIG_IGN before `main`, so the + // write returns EPIPE instead of killing us, and the old code then + // gave three different answers for one command: exit 1 with + // `Error: Broken pipe (os error 32)` when the error arrived wrapped + // (serde_json, or any `.context(…)`), and a race between 141 and 0 + // when it did not. + // + // Collapse all of it into one deterministic, silent success. This + // is checked here rather than by restoring SIG_DFL so the behaviour + // is identical on Windows, where there is no SIGPIPE to restore. if let Err(err) = &result { - if let Some(io_err) = err.downcast_ref::() { - if io_err.kind() == std::io::ErrorKind::BrokenPipe { - std::process::exit(EXIT_SIGPIPE); - } + if is_broken_pipe(err) { + // Best-effort: drop any buffered stdout on the floor rather + // than letting the runtime's own flush-at-exit print a + // second broken-pipe complaint to stderr. + let _ = std::io::Write::flush(&mut std::io::stdout()); + std::process::exit(0); } } result diff --git a/crates/cli/src/output.rs b/crates/cli/src/output.rs index 12dc871..a7e2b6e 100644 --- a/crates/cli/src/output.rs +++ b/crates/cli/src/output.rs @@ -44,6 +44,11 @@ struct FindingOut<'a> { detail: Option<&'a str>, #[serde(skip_serializing_if = "Option::is_none")] source: Option<&'a str>, + /// 1-based line of `source` in the analyzed log. Additive field — + /// omitted when unknown, so consumers written against the older + /// shape are unaffected. + #[serde(skip_serializing_if = "Option::is_none")] + line_number: Option, } impl<'a> From<&'a Finding> for FindingOut<'a> { @@ -53,16 +58,36 @@ impl<'a> From<&'a Finding> for FindingOut<'a> { value: &f.value, detail: f.detail.as_deref(), source: f.source.as_deref(), + line_number: f.line_number, } } } +/// Did this scan actually inspect something, and did it recognize +/// anything? Mirrors the legacy Node analyzer's field of the same name +/// and pairs with the exit ladder in `cmd::scan` (2 = empty input, +/// 3 = unrecognized). +/// +/// Consumers that only ever looked at `findings` keep working; this is +/// an added key, and it exists so that "zero findings" can be +/// distinguished from "clean" without inferring it from an array +/// length. +fn analysis_status(findings: &[Finding]) -> &'static str { + if findings.is_empty() { + "unrecognized" + } else { + "matched" + } +} + #[derive(Serialize)] struct Envelope<'a> { bootintel_version: &'a str, analysis_source: &'a str, detector_count: usize, findings: Vec>, + /// Added field — see `analysis_status`. + analysis_status: &'a str, } /// Whether the `text` format should emit ANSI colour escapes. @@ -148,17 +173,24 @@ pub fn resolve_color_mode_from_tty(no_color_flag: bool, is_tty: bool) -> ColorMo } } +/// Render `findings` in `format`. +/// +/// `source_uri` identifies the analyzed input — the path as the user +/// gave it, or `stdin` for a pipe. SARIF needs it to point its +/// `artifactLocation` at a file that actually exists in the caller's +/// workspace; the other formats ignore it. pub fn write( out: &mut W, findings: &[Finding], format: Format, raw_log: &str, color: ColorMode, + source_uri: &str, ) -> Result<()> { match format { Format::Json => write_json(out, findings), Format::Text => write_text(out, findings, color), - Format::Sarif => write_sarif(out, findings, raw_log), + Format::Sarif => write_sarif(out, findings, raw_log, source_uri), Format::Junit => write_junit(out, findings), Format::Csv => write_csv(out, findings, None), Format::Html => write_html(out, findings), @@ -390,6 +422,7 @@ fn write_json(out: &mut W, findings: &[Finding]) -> Result<()> { analysis_source: "client", detector_count: bootintel_detectors::detector_labels().len(), findings: findings.iter().map(FindingOut::from).collect(), + analysis_status: analysis_status(findings), }; serde_json::to_writer_pretty(&mut *out, &env)?; writeln!(out)?; @@ -441,35 +474,84 @@ fn write_text(out: &mut W, findings: &[Finding], color: ColorMode) -> writeln!(out, " {:<24} {styled_detail}", "")?; } } - writeln!(out)?; - writeln!( - out, - " {} findings identified locally (client-side detectors only).", - findings.len() - )?; - writeln!(out, " For CVE matches + exploit paths + AI report:")?; - writeln!( - out, - " bootintel scan --api (with BOOTINTEL_API_KEY set)" - )?; - writeln!( - out, - " bootintel scan --api --preview (anonymous free preview — 3/day per IP)" - )?; - writeln!( - out, - " bootintel share (share via URL, log embedded, no upload)" - )?; + write_text_summary(findings.len()); Ok(()) } +/// The trailing count + "here is what the paid tier adds" block. +/// +/// Goes to **stderr**, unconditionally, and not at all under `-q`. +/// +/// It used to go to stdout and ignore `-q` entirely, which meant +/// `bootintel scan --format text > report.txt` shipped a three-line +/// advertisement inside a customer's report, and `-q` — whose own help +/// text promises it "suppresses banners + status hints" — did nothing. +/// stdout carries findings; commentary about them belongs on stderr +/// with every other status hint in this CLI. +fn write_text_summary(count: usize) { + if crate::verbose::is_quiet() { + return; + } + eprintln!(); + eprintln!(" {count} findings identified locally (client-side detectors only)."); + eprintln!(" For CVE matches + exploit paths + AI report:"); + eprintln!(" bootintel scan --api (with BOOTINTEL_API_KEY set)"); + eprintln!(" bootintel scan --api --preview (anonymous free preview — 3/day per IP)"); + eprintln!(" bootintel share (share via URL, log embedded, no upload)"); +} + // ── SARIF v2.1.0 ───────────────────────────────────────────────────── // // Small hand-rolled builder — SARIF is verbose but our subset is // tractable. Enough to satisfy GitHub Code Scanning ingestion. -fn write_sarif(out: &mut W, findings: &[Finding], raw_log: &str) -> Result<()> { +/// Turn the input's identity into a SARIF `artifactLocation.uri`. +/// +/// This used to be the constant `"boot.log"`, which quietly broke the +/// flagship CI integration: GitHub's SARIF upload attaches each result +/// to the file named here, so every annotation pointed at a path that +/// is not in the repository and landed nowhere. +/// +/// SARIF wants a URI relative to the run's root when it can be one, so: +/// an absolute path under the workspace (`$GITHUB_WORKSPACE`, else the +/// current directory) is emitted relative to it; anything else is +/// emitted as given. `stdin` passes through unchanged — there is no +/// file to annotate, and a consumer can see that plainly. +fn sarif_artifact_uri(source_uri: &str) -> String { + if source_uri == "stdin" || source_uri == "-" { + return "stdin".to_string(); + } + let path = std::path::Path::new(source_uri); + let root = std::env::var_os("GITHUB_WORKSPACE") + .map(std::path::PathBuf::from) + .or_else(|| std::env::current_dir().ok()); + if let Some(root) = root { + // Compare canonicalized forms so `./log.txt`, `log.txt` and a + // symlinked workspace all resolve the same way, but emit the + // *uncanonicalized* relative path so it matches what is + // actually checked in. + if let (Ok(abs_path), Ok(abs_root)) = (path.canonicalize(), root.canonicalize()) { + if let Ok(rel) = abs_path.strip_prefix(&abs_root) { + // SARIF URIs use forward slashes on every platform. + return rel + .components() + .map(|c| c.as_os_str().to_string_lossy().into_owned()) + .collect::>() + .join("/"); + } + } + } + source_uri.to_string() +} + +fn write_sarif( + out: &mut W, + findings: &[Finding], + raw_log: &str, + source_uri: &str, +) -> Result<()> { let ver = env!("CARGO_PKG_VERSION"); + let artifact_uri = sarif_artifact_uri(source_uri); let results: Vec = findings .iter() .map(|f| { @@ -482,11 +564,18 @@ fn write_sarif(out: &mut W, findings: &[Finding], raw_log: &str) -> Re if let Some(d) = &f.detail { msg.push_str(&format!(" ({d})")); } + // Prefer the line number the detector library recorded; + // fall back to locating the evidence text for findings + // rehydrated from an older archived envelope. let line_index = f - .source - .as_ref() - .and_then(|s| raw_log.lines().position(|line| line.contains(s))) - .map(|i| i as i64 + 1) + .line_number + .map(|n| n as i64) + .or_else(|| { + f.source + .as_ref() + .and_then(|s| raw_log.lines().position(|line| line.contains(s))) + .map(|i| i as i64 + 1) + }) .unwrap_or(1); serde_json::json!({ "ruleId": f.label, @@ -494,7 +583,7 @@ fn write_sarif(out: &mut W, findings: &[Finding], raw_log: &str) -> Re "message": { "text": msg }, "locations": [{ "physicalLocation": { - "artifactLocation": { "uri": "boot.log" }, + "artifactLocation": { "uri": artifact_uri }, "region": { "startLine": line_index } } }] @@ -613,6 +702,7 @@ mod tests { value: value.into(), detail: None, source: None, + line_number: None, } } diff --git a/crates/cli/src/term/logfile.rs b/crates/cli/src/term/logfile.rs index fb76e6d..bc1be20 100644 --- a/crates/cli/src/term/logfile.rs +++ b/crates/cli/src/term/logfile.rs @@ -1,9 +1,36 @@ //! Thin BufWriter wrapper for `--log-file`. //! -//! Writes every raw byte the serial port emits to a file. Flushes -//! on drop. Ignores write errors on the second attempt so a full -//! disk doesn't crash the whole terminal — the terminal keeps -//! working; the log file just stops growing. +//! Writes every raw byte the serial port emits to a file. Ignores +//! write errors on the second attempt so a full disk doesn't crash +//! the whole terminal — the terminal keeps working; the log file just +//! stops growing. +//! +//! # Durability +//! +//! Every `write_bytes` call ends in a `flush`. This is deliberate and +//! it is the whole point of the type. +//! +//! The previous design flushed only on `Drop`, which meant the buffer +//! reached the filesystem only when the session ended through the +//! clean quit path. A capture smaller than the 8 KiB buffer — which is +//! most of them, since a boot log is a short burst followed by an idle +//! console — sat entirely in user-space memory. Anyone who exited with +//! Ctrl-C, closed the terminal window, unplugged the adapter, or let +//! the laptop sleep lost the entire capture while `ls` showed them a +//! file that existed and the on-screen analysis showed the bytes had +//! been received and understood. A 0-byte file is the worst possible +//! outcome for a tool whose job is to not lose your capture. +//! +//! Flushing per read also makes the file **tailable**: `tail -f +//! capture.log` in a second terminal now follows the session live, +//! which is how people actually use a capture tool. +//! +//! The cost is one `write(2)` per serial read rather than one per +//! 8 KiB. At 115200 baud with the terminal's 100 ms read cadence +//! that is ~10 syscalls a second. The buffer is retained so a single +//! read is still a single syscall. + +use std::sync::atomic::{AtomicU64, Ordering}; use anyhow::{Context, Result}; use std::fs::{File, OpenOptions}; @@ -100,7 +127,13 @@ impl LogFile { if let Err(e) = res { eprintln!("[bootintel] log-file write failed: {e}. Further writes suppressed."); self.broken = true; + return; } + // Push to the filesystem now — see the module docs. Without + // this the capture exists only in this process's memory until + // the buffer happens to fill or the session quits cleanly. + self.flush(); + BYTES_PERSISTED.fetch_add(bytes.len() as u64, Ordering::Relaxed); } /// Split `bytes` on '\n' boundaries and emit a timestamp before @@ -139,6 +172,17 @@ impl Drop for LogFile { } } +/// Total bytes this process has written **and flushed** to a +/// `--log-file`. Used by `bootintel term`'s shutdown path to report +/// what was actually persisted, and by the regression tests to assert +/// that bytes reach the filesystem mid-session rather than at exit. +static BYTES_PERSISTED: AtomicU64 = AtomicU64::new(0); + +/// Bytes flushed to the log file so far this process. +pub fn bytes_persisted() -> u64 { + BYTES_PERSISTED.load(Ordering::Relaxed) +} + /// ISO-8601 UTC with millisecond precision, e.g. `2026-08-24T09:12:03.487Z`. /// Deliberately zero-dep — computed from `SystemTime` and a fixed /// civil-time algorithm. Cheaper than pulling in `chrono` for one @@ -315,6 +359,146 @@ mod tests { false } + // ── Durability regressions ────────────────────────────────────── + // + // `--log-file` used to flush only on Drop, so the capture reached + // the filesystem only if the session exited through the clean quit + // path. Measured against a socat PTY pair on the v0.3.1 release + // binary, the log file was 0 bytes at 2, 4, 6, 8 and 10 seconds + // into a live session, and 0 bytes after both SIGINT and SIGTERM, + // while the session itself was correctly analyzing those same + // bytes on screen. + // + // These tests assert the property that was missing: bytes are on + // disk MID-session, not merely by the end of it. Asserting only + // the final contents (as `writes_bytes_and_flushes_on_drop` above + // does) passes happily against the broken version. + + #[test] + fn bytes_are_on_disk_mid_session_not_just_at_drop() { + let dir = tempdir_or_current(); + let path = dir.join("bootintel-logfile-midsession-test.txt"); + let _ = std::fs::remove_file(&path); + + let mut lf = LogFile::create(&path, LogFileMode::Overwrite).unwrap(); + + // Deliberately far less than the 8 KiB buffer — this is the + // shape of a real capture (a short boot burst, then an idle + // console) and it is exactly the case the old code lost. + lf.write_bytes(b"U-Boot 2020.10 (Sep 17 2023 - 11:38:21 +0000)\n"); + + // NOTE: `lf` is still alive. No Drop has run. + let size_after_first_write = std::fs::metadata(&path).unwrap().len(); + assert!( + size_after_first_write > 0, + "log file is still 0 bytes after a write while the session is live — \ + a Ctrl-C here would lose the whole capture" + ); + + lf.write_bytes(b"Hit any key to stop autoboot: 3\n"); + let size_after_second_write = std::fs::metadata(&path).unwrap().len(); + assert!( + size_after_second_write > size_after_first_write, + "log file did not grow on the second write ({size_after_first_write} -> \ + {size_after_second_write}) — it is still being buffered" + ); + + drop(lf); + let _ = std::fs::remove_file(&path); + } + + #[test] + fn a_second_reader_can_tail_the_file_while_the_session_runs() { + // `tail -f capture.log` from another terminal is how people + // actually use a capture tool. That only works if the bytes + // are in the file, so this reads through an independent handle + // while the LogFile is still open. + let dir = tempdir_or_current(); + let path = dir.join("bootintel-logfile-tailable-test.txt"); + let _ = std::fs::remove_file(&path); + + let mut lf = LogFile::create(&path, LogFileMode::Overwrite).unwrap(); + lf.write_bytes(b"first burst\n"); + + let seen_now = std::fs::read_to_string(&path).unwrap(); + assert_eq!( + seen_now, "first burst\n", + "an independent reader could not see the bytes mid-session" + ); + + lf.write_bytes(b"second burst\n"); + let seen_later = std::fs::read_to_string(&path).unwrap(); + assert_eq!(seen_later, "first burst\nsecond burst\n"); + + drop(lf); + let _ = std::fs::remove_file(&path); + } + + #[test] + fn nothing_is_lost_when_the_process_never_drops_the_logfile() { + // Simulates the signal path: the LogFile is leaked, so Drop + // never runs, exactly as when a default-disposition SIGTERM + // tears the process down. Everything written must already be + // on disk. + let dir = tempdir_or_current(); + let path = dir.join("bootintel-logfile-noflushdrop-test.txt"); + let _ = std::fs::remove_file(&path); + + { + let mut lf = LogFile::create(&path, LogFileMode::Overwrite).unwrap(); + lf.write_bytes(b"line one\n"); + lf.write_bytes(b"line two\n"); + // Never dropped — Drop's flush cannot be what saves us. + std::mem::forget(lf); + } + + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + "line one\nline two\n", + "bytes were lost when Drop did not run — a signal would lose them too" + ); + let _ = std::fs::remove_file(&path); + } + + #[test] + fn timestamped_writes_are_also_flushed_mid_session() { + // The --log-timestamps path goes through a different write + // function; it must be just as durable. + let dir = tempdir_or_current(); + let path = dir.join("bootintel-logfile-ts-midsession-test.txt"); + let _ = std::fs::remove_file(&path); + + let mut lf = LogFile::create(&path, LogFileMode::Overwrite) + .unwrap() + .with_timestamps(true); + lf.write_bytes(b"U-Boot 2020.10\n"); + + let contents = std::fs::read_to_string(&path).unwrap(); + assert!( + contents.contains("U-Boot 2020.10"), + "timestamped write not flushed mid-session, got {contents:?}" + ); + drop(lf); + let _ = std::fs::remove_file(&path); + } + + #[test] + fn bytes_persisted_counter_tracks_flushed_writes() { + let before = bytes_persisted(); + let dir = tempdir_or_current(); + let path = dir.join("bootintel-logfile-counter-test.txt"); + let _ = std::fs::remove_file(&path); + { + let mut lf = LogFile::create(&path, LogFileMode::Overwrite).unwrap(); + lf.write_bytes(b"12345"); + } + assert!( + bytes_persisted() >= before + 5, + "persisted-byte counter did not advance" + ); + let _ = std::fs::remove_file(&path); + } + fn tempdir_or_current() -> std::path::PathBuf { // Prefer $CARGO_TARGET_TMPDIR when set (cargo test provides // it); fall back to std::env::temp_dir(); fall back to CWD. diff --git a/crates/cli/src/term/mod.rs b/crates/cli/src/term/mod.rs index 0e22498..dc6f7dd 100644 --- a/crates/cli/src/term/mod.rs +++ b/crates/cli/src/term/mod.rs @@ -10,8 +10,13 @@ //! (via Drop) so a crash never leaves the user's shell wedged in //! raw mode. //! * `logfile` — thin BufWriter wrapper for `--log-file`. Flushes on -//! drop; ignores write errors on the second try so a full disk -//! doesn't crash the terminal. +//! every write (so the capture survives Ctrl-C / an unplugged +//! adapter and is tailable live); ignores write errors on the +//! second try so a full disk doesn't crash the terminal. +//! * `signals` — SIGINT/SIGTERM/SIGHUP handlers that ask the `run` +//! loop to exit through its normal path, so raw mode is restored +//! and the log file is flushed instead of the process dying where +//! it stands. //! * `run` — the main terminal loop: two background threads (serial //! reader, keyboard reader) both send into one std::sync::mpsc //! channel that the main thread drains. Fully synchronous — no @@ -23,3 +28,4 @@ pub mod macros; pub mod newline; pub mod raw_mode; pub mod run; +pub mod signals; diff --git a/crates/cli/src/term/run.rs b/crates/cli/src/term/run.rs index 87b7352..b3c28e6 100644 --- a/crates/cli/src/term/run.rs +++ b/crates/cli/src/term/run.rs @@ -117,6 +117,15 @@ pub fn run_session(mut opts: TermOptions) -> Result<()> { opts.flow_control, )?; + // Catch SIGINT / SIGTERM / SIGHUP so the loop below can exit + // through its normal path instead of the process being torn down + // where it stands. That matters for two things we are holding: the + // user's terminal (raw mode must be restored) and, with + // --log-file, the capture itself (must be flushed). Installed + // before the log file is opened so there is no window where a + // signal can kill us with bytes buffered. + super::signals::install(); + // Log-file writer (optional). let mut log = if let Some(p) = &opts.log_file { Some(LogFile::create(p, opts.log_mode)?.with_timestamps(opts.log_timestamps)) @@ -130,7 +139,29 @@ pub fn run_session(mut opts: TermOptions) -> Result<()> { // Enter raw mode. Held for the life of the loop; dropped on any // exit path (success, error, panic) via RAII. - let mut _raw = RawModeGuard::enter().context("entering terminal raw mode")?; + let mut _raw = RawModeGuard::enter().map_err(|e| { + // The only error in the CLI that used to arrive with no + // recovery hint at all: bare "entering terminal raw mode: + // No such device or address (os error 6)". It means stdin is + // not a terminal, which happens constantly — in CI, under + // nohup, in a cron job, inside a pipeline — and the fix + // depends on which of those you are doing. + anyhow::anyhow!( + "{e:#}\n\ + \n\ + \x20 This subcommand is interactive: it puts your terminal into raw mode so\n\ + \x20 keystrokes reach the device, which needs stdin to be a terminal.\n\ + \n\ + \x20 Running under CI / cron / nohup / a pipeline? Use a non-interactive\n\ + \x20 subcommand instead:\n\ + \x20 bootintel scan analyze a log you already captured\n\ + \x20 bootintel watch follow a growing log file\n\ + \n\ + \x20 Want the interactive terminal from a script? Allocate a TTY:\n\ + \x20 script -qec 'bootintel ...' /dev/null\n\ + \x20 ssh -tt host bootintel ..." + ) + })?; // Channel that both background threads push into. Main thread // reads. 64-slot bounded channel — a slow terminal getting behind @@ -232,6 +263,14 @@ pub fn run_session(mut opts: TermOptions) -> Result<()> { } let exit_reason = loop { + // A signal handler may have asked us to stop. Break out so the + // teardown below runs: threads joined, raw mode restored, log + // file flushed. The 100 ms recv timeout bounds how long this + // takes to notice. + if super::signals::shutdown_requested() { + break format!("received {}", super::signals::shutdown_reason()); + } + // Bounded recv — poll every 100ms so the analyze mode's pacer // can time-flush partial-line findings even on a quiet line. let ev_opt = match rx.recv_timeout(Duration::from_millis(100)) { @@ -876,6 +915,14 @@ pub fn run_session(mut opts: TermOptions) -> Result<()> { if let Some(mut l) = log { l.flush(); + drop(l); + if let Some(p) = &opts.log_file { + eprintln!( + "[bootintel] capture saved — {} bytes in {}", + super::logfile::bytes_persisted(), + p.display() + ); + } } Ok(()) } @@ -1129,6 +1176,57 @@ fn _serial_port_traits_are_sane(_p: P) {} /// (or bootintel + picocom, etc.) sessions from silently corrupting /// each other's stream. Windows: no equivalent; serial ports there are /// exclusive by default via the CreateFile semantics. +/// Reject a path that exists but is not a serial device, before we try +/// to open it. +/// +/// `bootintel analyze /etc/hostname` used to report "permission denied +/// opening /etc/hostname" and advise adding the user to the `dialout` +/// group — on a world-readable regular file. The diagnosis was simply +/// wrong (the open fails because a regular file has no termios state, +/// not because of permissions) and the advice sent people off to +/// change their group membership for no reason. +/// +/// A serial port is a character device. Anything else that exists — a +/// regular file, a directory, a socket — gets told what it actually is, +/// and pointed at the subcommand that does want a file. +#[cfg(unix)] +fn reject_non_serial_path(port_name: &str) -> Result<()> { + use std::os::unix::fs::FileTypeExt; + let Ok(meta) = std::fs::metadata(port_name) else { + // Doesn't exist, or we can't stat it — let open() produce the + // authoritative error. + return Ok(()); + }; + let ft = meta.file_type(); + if ft.is_char_device() { + return Ok(()); + } + let what = if ft.is_dir() { + "a directory" + } else if ft.is_file() { + "a regular file" + } else if ft.is_socket() { + "a socket" + } else if ft.is_fifo() { + "a FIFO" + } else if ft.is_block_device() { + "a block device" + } else { + "not a character device" + }; + bail!( + "{port_name} is {what}, not a serial device\n\ + \n\ + \x20 This subcommand opens a UART and reads it live; it needs a character\n\ + \x20 device such as /dev/ttyUSB0 or /dev/ttyACM0.\n\ + \x20 Run `bootintel ports` to list what's here.\n\ + \n\ + \x20 To analyze a log you already have on disk:\n\ + \x20 bootintel scan {port_name} one-shot analysis\n\ + \x20 bootintel watch {port_name} follow the file as it grows" + ); +} + pub fn open_serial_with_hints( port_name: &str, baud: u32, @@ -1137,6 +1235,9 @@ pub fn open_serial_with_hints( stop_bits: StopBits, flow_control: FlowControl, ) -> Result> { + #[cfg(unix)] + reject_non_serial_path(port_name)?; + let builder = serialport::new(port_name, baud) .data_bits(data_bits) .parity(parity) diff --git a/crates/cli/src/term/signals.rs b/crates/cli/src/term/signals.rs new file mode 100644 index 0000000..76fdc41 --- /dev/null +++ b/crates/cli/src/term/signals.rs @@ -0,0 +1,142 @@ +//! Shutdown-signal handling for the interactive terminal. +//! +//! `bootintel term` / `bootintel analyze` hold two things the process +//! must not die holding: the user's terminal in raw mode, and (when +//! `--log-file` is in use) a capture the user believes they have. +//! +//! Default dispositions get both wrong. `SIGTERM` (a `kill`, a service +//! manager stopping us, a laptop suspending) and `SIGHUP` (the +//! terminal window closing, the SSH session dropping, the USB adapter's +//! tty going away) terminate the process immediately: no unwinding, no +//! `Drop`, so the raw-mode guard never restores the terminal and the +//! log file is whatever the last flush left behind. +//! +//! So we install handlers that do the only thing a signal handler may +//! safely do — set a flag — and let the terminal's main loop notice it +//! and exit through the normal path. That path drops the `RawMode` +//! guard (terminal restored) and the `LogFile` (final flush), exactly +//! as `Ctrl-A q` does. +//! +//! The loop polls on a 100 ms timeout, so the worst-case latency +//! between the signal and a clean exit is 100 ms. +//! +//! Note that `SIGINT` normally does *not* arrive here: once the +//! terminal is in raw mode, `Ctrl-C` is delivered to us as the byte +//! `0x03`, not as a signal. The handler matters for an explicit +//! `kill -INT`, and for the window between process start and raw mode +//! being entered. + +use std::sync::atomic::{AtomicBool, Ordering}; + +/// Set by the signal handler; read by the terminal loop. +static SHUTDOWN: AtomicBool = AtomicBool::new(false); + +/// Number (as `c_int`) of the signal that asked us to stop, or 0. +/// Only used to name the signal in the goodbye line. +/// +/// Gated because the only reader is the `cfg(unix)` arm of +/// `shutdown_reason` and the only writers are the `cfg(unix)` handler and +/// `reset_for_test`. A Windows release build has neither, so an +/// ungated static is dead code, and this crate denies warnings: it broke +/// `cargo build --release` on windows-latest while every other target +/// stayed green. +#[cfg(any(unix, test))] +static SHUTDOWN_SIGNAL: std::sync::atomic::AtomicI32 = std::sync::atomic::AtomicI32::new(0); + +/// Whether a shutdown signal has been received. +pub fn shutdown_requested() -> bool { + SHUTDOWN.load(Ordering::Relaxed) +} + +/// Human-readable name of the signal that requested shutdown. +pub fn shutdown_reason() -> &'static str { + #[cfg(unix)] + match SHUTDOWN_SIGNAL.load(Ordering::Relaxed) { + x if x == libc::SIGINT => "SIGINT (Ctrl-C)", + x if x == libc::SIGTERM => "SIGTERM", + x if x == libc::SIGHUP => "SIGHUP (terminal closed)", + _ => "signal", + } + #[cfg(not(unix))] + "signal" +} + +/// Request shutdown from ordinary (non-signal) code. Exposed so tests +/// can drive the same exit path a signal would. +#[cfg(test)] +pub fn request_shutdown() { + SHUTDOWN.store(true, Ordering::Relaxed); +} + +/// Clear the flag. Tests only — a process only shuts down once. +#[cfg(test)] +pub fn reset_for_test() { + SHUTDOWN.store(false, Ordering::Relaxed); + SHUTDOWN_SIGNAL.store(0, Ordering::Relaxed); +} + +/// The handler itself. Async-signal-safe: two relaxed atomic stores +/// and nothing else. No allocation, no locking, no I/O — a `println!` +/// here could deadlock against a `print!` interrupted mid-call. +#[cfg(unix)] +extern "C" fn handle(sig: libc::c_int) { + SHUTDOWN_SIGNAL.store(sig, Ordering::Relaxed); + SHUTDOWN.store(true, Ordering::Relaxed); +} + +/// Install handlers for SIGINT / SIGTERM / SIGHUP. +/// +/// Idempotent, and safe to call when not attached to a terminal. +#[cfg(unix)] +pub fn install() { + // SAFETY: `signal(2)` with a plain extern "C" fn pointer. The + // handler touches only atomics, so it is async-signal-safe. + // Called once, from the terminal command's entry point. + unsafe { + let h = handle as *const () as libc::sighandler_t; + libc::signal(libc::SIGINT, h); + libc::signal(libc::SIGTERM, h); + libc::signal(libc::SIGHUP, h); + } +} + +#[cfg(not(unix))] +pub fn install() { + // Windows: Ctrl-C arrives through crossterm's event stream while + // the console is in raw mode, and the terminal loop already treats + // it as a quit request. Nothing to install. +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn flag_starts_clear_and_can_be_set() { + reset_for_test(); + assert!(!shutdown_requested()); + request_shutdown(); + assert!(shutdown_requested()); + reset_for_test(); + assert!(!shutdown_requested()); + } + + #[cfg(unix)] + #[test] + fn real_signal_sets_the_flag() { + reset_for_test(); + install(); + // Raise SIGTERM at ourselves. With the default disposition + // this would terminate the test binary; the handler makes it + // a flag flip. + unsafe { + libc::raise(libc::SIGTERM); + } + assert!( + shutdown_requested(), + "SIGTERM did not set the shutdown flag" + ); + assert_eq!(shutdown_reason(), "SIGTERM"); + reset_for_test(); + } +} diff --git a/crates/cli/tests/cli_regressions.rs b/crates/cli/tests/cli_regressions.rs new file mode 100644 index 0000000..8908e2a --- /dev/null +++ b/crates/cli/tests/cli_regressions.rs @@ -0,0 +1,564 @@ +//! Regression tests for correctness defects found by exercising the +//! real v0.3.1 release binary against socat PTY pairs. +//! +//! These drive the compiled binary rather than calling library +//! functions, because every one of these bugs was invisible from +//! inside the library: the exit code, the stdout/stderr split and the +//! process's UTF-8 handling are all properties of the binary. +//! +//! Item numbering matches the defect report. +//! +//! Item 1 (`--log-file` writing 0 bytes until the clean quit path) is +//! covered by unit tests in `src/term/logfile.rs`, which is where the +//! mid-session-flush property can be asserted without a serial device. +//! Item 3 (line-anchored detectors) is covered in +//! `crates/detectors/tests/detector_tests.rs`. + +use std::io::Write; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +const BIN: &str = env!("CARGO_BIN_EXE_bootintel"); + +/// A scratch dir unique to this test binary, cleaned between runs. +fn tmpdir() -> PathBuf { + let base = std::env::var("CARGO_TARGET_TMPDIR") + .map(PathBuf::from) + .unwrap_or_else(|_| std::env::temp_dir()); + let dir = base.join("bootintel-cli-regressions"); + std::fs::create_dir_all(&dir).expect("creating scratch dir"); + dir +} + +fn write_file(name: &str, bytes: &[u8]) -> PathBuf { + let path = tmpdir().join(name); + let mut f = std::fs::File::create(&path).expect("creating fixture"); + f.write_all(bytes).expect("writing fixture"); + f.sync_all().ok(); + path +} + +fn run(args: &[&str]) -> Output { + Command::new(BIN) + .args(args) + // History writes are a side effect we don't want in tests, and + // the first-write disclosure would add stderr noise. + .env("BOOTINTEL_NO_HISTORY", "1") + .output() + .expect("running bootintel") +} + +fn code(out: &Output) -> i32 { + out.status.code().unwrap_or(-1) +} + +fn stdout(out: &Output) -> String { + String::from_utf8_lossy(&out.stdout).into_owned() +} + +fn stderr(out: &Output) -> String { + String::from_utf8_lossy(&out.stderr).into_owned() +} + +/// A log fragment that trips two detectors, one of them critical. +const GOOD_LOG: &str = "U-Boot 2020.10 (Sep 17 2023 - 11:38:21 +0000)\n\ + Hit any key to stop autoboot: 3\n"; + +// ── Item 2: an empty capture must not pass a gate ──────────────────── +// +// `scan --gate-critical` on a 0-byte file printed an empty finding +// list and exited 0, so a CI job whose UART never came up, whose +// adapter fell out, or whose artifact path was wrong reported green. + +#[test] +fn empty_file_exits_2_not_0() { + let path = write_file("empty.log", b""); + let out = run(&["scan", path.to_str().unwrap(), "--format", "json"]); + assert_eq!( + code(&out), + 2, + "empty capture must exit 2, got {}\nstderr: {}", + code(&out), + stderr(&out) + ); +} + +#[test] +fn empty_file_does_not_pass_gate_critical() { + let path = write_file("empty-gate.log", b""); + let out = run(&[ + "scan", + path.to_str().unwrap(), + "--gate-critical", + "--format", + "json", + ]); + assert_ne!( + code(&out), + 0, + "an empty capture reported a PASSING critical gate — this is the \ + exact failure that reports a broken CI job as green" + ); + assert_eq!(code(&out), 2); + assert!( + stderr(&out).contains("empty capture"), + "expected an explanation on stderr, got: {}", + stderr(&out) + ); +} + +#[test] +fn whitespace_only_capture_is_also_empty() { + let path = write_file("blank.log", b"\n\n \t\r\n \n"); + let out = run(&["scan", path.to_str().unwrap(), "--format", "json"]); + assert_eq!(code(&out), 2, "stderr: {}", stderr(&out)); +} + +#[test] +fn empty_stdin_exits_2() { + let mut child = Command::new(BIN) + .args(["scan", "-", "--format", "json"]) + .env("BOOTINTEL_NO_HISTORY", "1") + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .expect("spawning"); + drop(child.stdin.take()); // EOF immediately + let out = child.wait_with_output().expect("waiting"); + assert_eq!(code(&out), 2, "stderr: {}", stderr(&out)); + assert!(stderr(&out).contains("stdin"), "{}", stderr(&out)); +} + +#[test] +fn unrecognized_but_non_empty_exits_3() { + let path = write_file("noise.log", b"this capture contains nothing we know\n"); + let out = run(&["scan", path.to_str().unwrap(), "--format", "json"]); + assert_eq!( + code(&out), + 3, + "non-empty-but-unrecognized must exit 3, got {}\nstderr: {}", + code(&out), + stderr(&out) + ); + assert!( + stdout(&out).contains("\"analysis_status\": \"unrecognized\""), + "envelope should carry analysis_status, got: {}", + stdout(&out) + ); +} + +#[test] +fn a_real_capture_still_exits_0_and_reports_matched() { + let path = write_file("good.log", GOOD_LOG.as_bytes()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "json"]); + assert_eq!(code(&out), 0, "stderr: {}", stderr(&out)); + assert!( + stdout(&out).contains("\"analysis_status\": \"matched\""), + "got: {}", + stdout(&out) + ); +} + +#[test] +fn gate_critical_still_fails_with_1_on_a_real_finding() { + // The new ladder must not swallow the code the gate already had. + let path = write_file("critical.log", GOOD_LOG.as_bytes()); + let out = run(&["scan", path.to_str().unwrap(), "--gate-critical"]); + assert_eq!(code(&out), 1, "stderr: {}", stderr(&out)); +} + +#[test] +fn existing_envelope_keys_are_unchanged() { + // The fix adds keys; it must not rename or drop any. + let path = write_file("envelope.log", GOOD_LOG.as_bytes()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "json"]); + let v: serde_json::Value = serde_json::from_str(&stdout(&out)).expect("valid JSON"); + for key in [ + "bootintel_version", + "analysis_source", + "detector_count", + "findings", + ] { + assert!(v.get(key).is_some(), "envelope lost the `{key}` key"); + } + let first = &v["findings"][0]; + for key in ["label", "value"] { + assert!(first.get(key).is_some(), "finding lost the `{key}` key"); + } +} + +// ── Item 4: a non-UTF-8 byte must not be a hard error ──────────────── +// +// A capture containing \xff\xfe\x80\x81\xc0\xc1 failed with "stream did +// not contain valid UTF-8" and exit 1, while the legacy Node analyzer +// read the same file and returned findings. This is the normal shape of +// a real UART capture — bytes before the baud locks, framing errors, a +// binary splash — and `--log-file` writes raw bytes, so `analyze +// --log-file` followed by `scan` could fail on the tool's own output. + +/// The SME's fixture: a valid log with a run of invalid bytes in it. +fn non_utf8_log() -> Vec { + let mut v = Vec::new(); + v.extend_from_slice(b"U-Boot 2020.10 (Sep 17 2023 - 11:38:21 +0000)\n"); + v.extend_from_slice(b"\xff\xfe\x80\x81\xc0\xc1\n"); + v.extend_from_slice(b"Hit any key to stop autoboot: 3\n"); + v +} + +#[test] +fn non_utf8_capture_is_analyzed_not_rejected() { + let path = write_file("binary.log", &non_utf8_log()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "json"]); + assert_eq!( + code(&out), + 0, + "a capture with invalid UTF-8 was rejected\nstderr: {}", + stderr(&out) + ); + let v: serde_json::Value = serde_json::from_str(&stdout(&out)).expect("valid JSON"); + let findings = v["findings"].as_array().expect("findings array"); + assert_eq!( + findings.len(), + 2, + "expected both detectors to fire, got {}", + stdout(&out) + ); +} + +#[test] +fn non_utf8_preserves_line_numbers() { + // The whole point of counting bytes rather than reflowing: the + // autoboot line is line 3 of the file and must be reported as line + // 3, with the replaced bytes on line 2 not merging the lines. + let path = write_file("binary-lines.log", &non_utf8_log()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "json"]); + let v: serde_json::Value = serde_json::from_str(&stdout(&out)).expect("valid JSON"); + let autoboot = v["findings"] + .as_array() + .unwrap() + .iter() + .find(|f| f["label"] == "Autoboot interruptable") + .expect("autoboot finding"); + assert_eq!( + autoboot["line_number"], + 3, + "line number shifted by the UTF-8 replacement: {}", + stdout(&out) + ); +} + +#[test] +fn non_utf8_replacement_count_is_reported_under_v() { + let path = write_file("binary-verbose.log", &non_utf8_log()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "json", "-v"]); + let err = stderr(&out); + assert!( + err.contains("6 byte(s)") && err.contains("U+FFFD"), + "expected -v to report how many bytes were replaced, got: {err}" + ); +} + +#[test] +fn non_utf8_note_is_silent_without_v() { + let path = write_file("binary-quiet.log", &non_utf8_log()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "json"]); + assert!( + !stderr(&out).contains("U+FFFD"), + "the replacement note should be -v only, got: {}", + stderr(&out) + ); +} + +#[test] +fn non_utf8_over_stdin_is_also_accepted() { + let mut child = Command::new(BIN) + .args(["scan", "-", "--format", "json"]) + .env("BOOTINTEL_NO_HISTORY", "1") + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .expect("spawning"); + child + .stdin + .as_mut() + .unwrap() + .write_all(&non_utf8_log()) + .expect("writing stdin"); + drop(child.stdin.take()); + let out = child.wait_with_output().expect("waiting"); + assert_eq!(code(&out), 0, "stderr: {}", stderr(&out)); +} + +#[test] +fn a_capture_that_is_entirely_binary_exits_3_not_1() { + // Garbage in, but it is not *empty* garbage — so it is 3 + // ("looked, recognized nothing"), never a hard read error. + let path = write_file("all-binary.log", &[0xff, 0xfe, 0x80, 0x81, 0xc0, 0xc1]); + let out = run(&["scan", path.to_str().unwrap(), "--format", "json"]); + assert_eq!(code(&out), 3, "stderr: {}", stderr(&out)); +} + +// ── Item 5: SIGPIPE must be one silent outcome, not three ──────────── +// +// `bootintel batch … --format json | head -2` gave exit 1 plus +// "Error: Broken pipe (os error 32)" when output exceeded the 64K pipe +// buffer, while smaller outputs raced between 141 and 0. +// `bootintel manpage | head` is a packager's first command. + +/// Run ` | head -` under bash and return bootintel's +/// OWN exit status plus whatever it wrote to stderr. +fn piped_through_head(args: &str, head_lines: u32) -> Option<(i32, String)> { + let script = format!( + "{BIN} {args} 2>/tmp/bootintel-pipe-stderr.$$ | head -{head_lines} >/dev/null; \ + rc=${{PIPESTATUS[0]}}; cat /tmp/bootintel-pipe-stderr.$$; \ + rm -f /tmp/bootintel-pipe-stderr.$$; exit $rc" + ); + let out = Command::new("bash") + .arg("-c") + .arg(&script) + .env("BOOTINTEL_NO_HISTORY", "1") + .output() + .ok()?; + Some((code(&out), stdout(&out))) +} + +#[cfg(unix)] +#[test] +fn manpage_piped_to_head_is_silently_successful_every_time() { + // Six runs: the old behaviour raced, so a single run could pass by + // luck. Every run must give the same answer. + for attempt in 1..=6 { + let Some((rc, err)) = piped_through_head("manpage", 1) else { + eprintln!("[skip] bash unavailable"); + return; + }; + assert_eq!( + rc, 0, + "attempt {attempt}: expected a silent success, got exit {rc}. stderr: {err}" + ); + assert!( + !err.contains("Broken pipe"), + "attempt {attempt}: leaked a broken-pipe error to stderr: {err}" + ); + } +} + +#[cfg(unix)] +#[test] +fn large_json_piped_to_head_is_silently_successful_every_time() { + // Exercises the serde_json write path, whose error does not + // downcast to io::Error and so escaped the previous handling. + let samples = Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .and_then(|p| p.parent()) + .map(|p| p.join("samples")); + let Some(samples) = samples.filter(|p| p.is_dir()) else { + eprintln!("[skip] samples/ not in this checkout"); + return; + }; + let args = format!("batch {} --format json", samples.display()); + for attempt in 1..=6 { + let Some((rc, err)) = piped_through_head(&args, 2) else { + eprintln!("[skip] bash unavailable"); + return; + }; + assert_eq!( + rc, 0, + "attempt {attempt}: expected a silent success, got exit {rc}. stderr: {err}" + ); + assert!( + !err.contains("Broken pipe"), + "attempt {attempt}: leaked a broken-pipe error: {err}" + ); + } +} + +#[cfg(unix)] +#[test] +fn sarif_piped_to_head_is_silently_successful() { + let path = write_file("pipe-sarif.log", GOOD_LOG.as_bytes()); + let args = format!("scan {} --format sarif", path.display()); + let Some((rc, err)) = piped_through_head(&args, 1) else { + eprintln!("[skip] bash unavailable"); + return; + }; + assert_eq!(rc, 0, "exit {rc}, stderr: {err}"); + assert!(!err.contains("Broken pipe"), "{err}"); +} + +// ── Item 6: SARIF must name the real input file ────────────────────── +// +// Every result carried artifactLocation.uri = "boot.log" regardless of +// the actual input, so GitHub's SARIF upload attached findings to a +// file that is not in the repository and the annotations landed +// nowhere. + +fn sarif_uris(out: &Output) -> Vec { + let v: serde_json::Value = serde_json::from_str(&stdout(out)).expect("valid SARIF JSON"); + v["runs"][0]["results"] + .as_array() + .expect("results array") + .iter() + .map(|r| r["locations"][0]["physicalLocation"]["artifactLocation"]["uri"].clone()) + .map(|u| u.as_str().unwrap_or_default().to_string()) + .collect() +} + +#[test] +fn sarif_uri_is_not_the_hardcoded_boot_log() { + let path = write_file("my-device-capture.log", GOOD_LOG.as_bytes()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "sarif"]); + let uris = sarif_uris(&out); + assert!(!uris.is_empty(), "expected results in the SARIF"); + for uri in &uris { + assert_ne!( + uri, "boot.log", + "SARIF still hardcodes boot.log; CI annotations will land nowhere" + ); + assert!( + uri.contains("my-device-capture.log"), + "SARIF uri should name the real input, got {uri}" + ); + } +} + +#[test] +fn sarif_uri_is_relative_to_the_working_directory() { + // GitHub matches the uri against paths in the repository, so an + // input inside the workspace must be emitted relative to it. + let dir = tmpdir(); + let name = "relative-capture.log"; + write_file(name, GOOD_LOG.as_bytes()); + let out = Command::new(BIN) + .args(["scan", name, "--format", "sarif"]) + .current_dir(&dir) + .env("BOOTINTEL_NO_HISTORY", "1") + // Make sure a real CI env var doesn't steer the test. + .env_remove("GITHUB_WORKSPACE") + .output() + .expect("running bootintel"); + for uri in sarif_uris(&out) { + assert_eq!( + uri, + name, + "expected a workspace-relative uri, got {uri} (stderr: {})", + stderr(&out) + ); + } +} + +#[test] +fn sarif_uri_honours_github_workspace() { + let dir = tmpdir(); + let name = "workspace-capture.log"; + let path = write_file(name, GOOD_LOG.as_bytes()); + let out = Command::new(BIN) + .args(["scan", path.to_str().unwrap(), "--format", "sarif"]) + .env("BOOTINTEL_NO_HISTORY", "1") + .env("GITHUB_WORKSPACE", &dir) + .output() + .expect("running bootintel"); + for uri in sarif_uris(&out) { + assert_eq!(uri, name, "stderr: {}", stderr(&out)); + } +} + +#[test] +fn sarif_uri_for_piped_input_is_stdin() { + let mut child = Command::new(BIN) + .args(["scan", "-", "--format", "sarif"]) + .env("BOOTINTEL_NO_HISTORY", "1") + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .expect("spawning"); + child + .stdin + .as_mut() + .unwrap() + .write_all(GOOD_LOG.as_bytes()) + .expect("writing stdin"); + drop(child.stdin.take()); + let out = child.wait_with_output().expect("waiting"); + for uri in sarif_uris(&out) { + assert_eq!(uri, "stdin", "stderr: {}", stderr(&out)); + } +} + +#[test] +fn sarif_start_line_points_at_the_evidence() { + let log = "boot noise\nmore noise\nU-Boot 2020.10 (Sep 17 2023 - 11:38:21 +0000)\n"; + let path = write_file("sarif-lines.log", log.as_bytes()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "sarif"]); + let v: serde_json::Value = serde_json::from_str(&stdout(&out)).expect("valid SARIF"); + let line = + &v["runs"][0]["results"][0]["locations"][0]["physicalLocation"]["region"]["startLine"]; + assert_eq!(line, 3, "SARIF startLine should be the evidence line"); +} + +// ── Item 7: -q must quiet, and the upsell belongs on stderr ────────── + +#[test] +fn text_output_on_stdout_carries_no_upsell() { + let path = write_file("upsell.log", GOOD_LOG.as_bytes()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "text"]); + assert!( + !stdout(&out).contains("--api"), + "the upsell is still on stdout; `scan --format text > report.txt` would \ + ship marketing inside a customer's report:\n{}", + stdout(&out) + ); + assert!( + stderr(&out).contains("--api"), + "the upsell should still be shown, on stderr: {}", + stderr(&out) + ); +} + +#[test] +fn quiet_suppresses_the_summary_block_entirely() { + let path = write_file("upsell-quiet.log", GOOD_LOG.as_bytes()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "text", "-q"]); + assert!(!stdout(&out).contains("--api"), "{}", stdout(&out)); + assert!( + !stderr(&out).contains("--api"), + "-q must suppress the summary block, got: {}", + stderr(&out) + ); +} + +#[test] +fn quiet_still_prints_the_findings_themselves() { + let path = write_file("quiet-findings.log", GOOD_LOG.as_bytes()); + let out = run(&["scan", path.to_str().unwrap(), "--format", "text", "-q"]); + assert!( + stdout(&out).contains("U-Boot 2020.10"), + "-q suppressed the actual output: {}", + stdout(&out) + ); +} + +// ── Item 9: diagnose a non-serial path correctly ───────────────────── + +#[cfg(unix)] +#[test] +fn analyze_on_a_regular_file_says_so_instead_of_blaming_permissions() { + let path = write_file("not-a-tty.log", GOOD_LOG.as_bytes()); + let out = run(&["analyze", path.to_str().unwrap()]); + let err = stderr(&out); + assert!( + err.contains("not a serial device"), + "expected a correct diagnosis, got: {err}" + ); + assert!( + !err.contains("dialout"), + "still advising a dialout-group change for a readable regular file: {err}" + ); + assert!( + err.contains("bootintel scan"), + "should point at the subcommand that does want a file: {err}" + ); +} diff --git a/crates/detectors/src/lib.rs b/crates/detectors/src/lib.rs index 9d05d70..d99f234 100644 --- a/crates/detectors/src/lib.rs +++ b/crates/detectors/src/lib.rs @@ -29,6 +29,12 @@ pub struct Finding { pub value: String, pub detail: Option, pub source: Option, + /// 1-based line number of `source` within the analyzed log. + /// Populated by `analyze()`; `None` when the evidence could not be + /// located (or when a `Finding` is constructed by hand, e.g. from + /// an archived JSON envelope). Additive — existing consumers that + /// don't know about it are unaffected. + pub line_number: Option, } impl Finding { @@ -38,6 +44,7 @@ impl Finding { value: value.into(), detail: None, source: None, + line_number: None, } } fn detail(mut self, d: impl Into) -> Self { @@ -53,8 +60,135 @@ impl Finding { /// Analyze a boot log against every detector; return findings in /// detector-registration order. Detectors that don't match are /// silently dropped. +/// +/// # Line normalization +/// +/// Several detectors are line-anchored (`(?m)^U-Boot`, `(?m)^GRUB`, +/// `(?m)^coreboot-`, `(?m)^procd:`). Real captures very often carry a +/// per-line prefix that defeats a bare `^`: +/// +/// * `[12:34:56.789] ` — minicom / picocom / tio timestamping +/// * `[2026-09-23T10:00:00.000Z] ` — **our own** `--log-timestamps` +/// * `[ 0.000000] ` — kernel printk timestamps +/// * `\x1b[32m` — ANSI colour from a colourising bootloader +/// +/// Before `--log-timestamps` existed this was merely common; now the +/// tool's own capture mode breaks its own `scan`, which is why the +/// normalization lives here rather than being pushed onto callers. +/// +/// So: split into lines, strip ANSI CSI sequences and any run of +/// leading bracketed timestamps, and run the detectors over the +/// normalized text. Evidence is then mapped back to the **original** +/// (unmodified) line, so `source` always shows the user what their +/// capture actually contained, prefix and all. +/// +/// Ported from the legacy Node analyzer's `analyze()` so both +/// implementations agree on what a "line" is and what gets stripped. pub fn analyze(log: &str) -> Vec { - ALL_DETECTORS.iter().filter_map(|d| (d.run)(log)).collect() + let original = split_lines(log); + let normalized: Vec = original.iter().map(|l| normalize_line(l)).collect(); + let norm_log = normalized.join("\n"); + ALL_DETECTORS + .iter() + .filter_map(|d| (d.run)(&norm_log)) + .map(|f| attach_evidence(f, &original, &normalized)) + .collect() +} + +/// Split on any of CRLF / LF / CR. `str::lines()` only handles LF and +/// CRLF; a bare-CR stream (some bootloaders emit CR-only line endings) +/// would otherwise arrive as one giant line and defeat every +/// line-anchored detector. +fn split_lines(log: &str) -> Vec<&str> { + let mut out = Vec::new(); + let bytes = log.as_bytes(); + let mut start = 0usize; + let mut i = 0usize; + while i < bytes.len() { + match bytes[i] { + b'\n' => { + out.push(&log[start..i]); + i += 1; + start = i; + } + b'\r' => { + out.push(&log[start..i]); + // CRLF counts as one terminator. + i += if i + 1 < bytes.len() && bytes[i + 1] == b'\n' { + 2 + } else { + 1 + }; + start = i; + } + _ => i += 1, + } + } + out.push(&log[start..]); + out +} + +/// ANSI CSI escape sequence: ESC `[` params intermediates final. +/// Same character classes as the Node analyzer's +/// `/\x1b\[[0-?]*[ -/]*[@-~]/g`. +static RE_ANSI_CSI: LazyLock = + LazyLock::new(|| Regex::new(r"\x1b\[[0-?]*[ -/]*[@-~]").unwrap()); + +/// One leading bracketed timestamp. Three accepted shapes, matching +/// the Node analyzer: +/// * `[12:34:56]` / `[12:34:56.789]` — wall-clock terminal logger +/// * `[2026-09-23T10:00:00.000Z]` — ISO-8601 (our --log-timestamps) +/// * `[ 0.000000]` — kernel printk seconds +static RE_LEADING_TS: LazyLock = LazyLock::new(|| { + Regex::new( + r"^\s*\[(?:\d{2}:\d{2}:\d{2}(?:\.\d+)?|\d{4}-\d{2}-\d{2}[ T]\d{2}:\d{2}:\d{2}(?:\.\d+)?(?:Z|[+-]\d{2}:?\d{2})?|\s*\d+\.\d+)\]\s*", + ) + .unwrap() +}); + +/// Strip ANSI CSI sequences and leading bracketed timestamps from one +/// line. The timestamp strip loops: `bootintel analyze --log-timestamps` +/// over a Linux console produces *two* stacked prefixes +/// (`[2026-…Z] [ 0.000000] Linux version …`), and the Node +/// analyzer's single-shot strip would leave the second one in place. +fn normalize_line(line: &str) -> String { + let mut s = RE_ANSI_CSI.replace_all(line, "").into_owned(); + // Bounded loop — a pathological line of nothing but bracketed + // timestamps must not spin forever. + for _ in 0..8 { + match RE_LEADING_TS.find(&s) { + Some(m) if m.end() > 0 => { + s = s[m.end()..].to_string(); + } + _ => break, + } + } + s +} + +/// Point a finding's `source` at the original, unmodified line that +/// produced it, and record that line's 1-based number. +/// +/// Detectors return the matched substring as `source`, taken from the +/// normalized text. Locating it is a plain substring search over the +/// normalized lines — far cheaper than the Node analyzer's re-run of +/// every detector against every line, and exact for the same reason +/// (the needle came out of that text verbatim). +fn attach_evidence(mut f: Finding, original: &[&str], normalized: &[String]) -> Finding { + let Some(src) = f.source.clone() else { + return f; + }; + // A few regexes (`[^)]+`) can span a newline; anchor on the first + // physical line of the match. + let needle = src.split('\n').next().unwrap_or(&src); + if needle.is_empty() { + return f; + } + if let Some(i) = normalized.iter().position(|l| l.contains(needle)) { + f.source = Some(original[i].to_string()); + f.line_number = Some(i + 1); + } + f } /// Return the labels of every registered detector, in order. Used by diff --git a/crates/detectors/tests/detector_tests.rs b/crates/detectors/tests/detector_tests.rs index 6494f62..4b6a261 100644 --- a/crates/detectors/tests/detector_tests.rs +++ b/crates/detectors/tests/detector_tests.rs @@ -267,3 +267,133 @@ fn detector_labels_stable() { ] ); } + +// ── Line-prefix normalization (regression) ─────────────────────────── +// +// The line-anchored detectors (`(?m)^U-Boot`, `(?m)^coreboot-`, +// `(?m)^GRUB`, `(?m)^procd:`) used to be defeated by anything at all in +// front of the anchor. A plain `U-Boot 2020.10` line was detected; the +// same line behind a terminal timestamp, an ISO-8601 timestamp, or an +// ANSI colour escape produced NO findings and exit 0, while the Kernel, +// CPU and Autoboot detectors — which are not anchored — handled all +// four. +// +// The ISO-8601 case is self-inflicted: `bootintel analyze +// --log-timestamps` prefixes every line with exactly that, so the +// tool's own capture mode broke its own `scan`. +// +// One fixture per prefix shape, all four asserted to produce the same +// finding as the bare line. + +/// The bare line every prefixed variant below must still match. +const UBOOT_LINE: &str = "U-Boot 2020.10 (Sep 17 2023 - 11:38:21 +0000)"; + +fn uboot_value(log: &str) -> Option { + find(log, "Bootloader").map(|f| f.value) +} + +#[test] +fn bootloader_matches_bare_line() { + assert_eq!(uboot_value(UBOOT_LINE).as_deref(), Some("U-Boot 2020.10")); +} + +#[test] +fn bootloader_survives_bracketed_clock_prefix() { + let log = format!("[12:34:56.789] {UBOOT_LINE}"); + assert_eq!(uboot_value(&log).as_deref(), Some("U-Boot 2020.10")); +} + +#[test] +fn bootloader_survives_iso8601_prefix() { + // Exactly what `bootintel analyze --log-timestamps` writes. + let log = format!("[2026-09-23T10:00:00.000Z] {UBOOT_LINE}"); + assert_eq!(uboot_value(&log).as_deref(), Some("U-Boot 2020.10")); +} + +#[test] +fn bootloader_survives_ansi_colour_prefix() { + let log = format!("\x1b[32m{UBOOT_LINE}\x1b[0m"); + assert_eq!(uboot_value(&log).as_deref(), Some("U-Boot 2020.10")); +} + +#[test] +fn bootloader_survives_kernel_printk_prefix() { + let log = format!("[ 0.000000] {UBOOT_LINE}"); + assert_eq!(uboot_value(&log).as_deref(), Some("U-Boot 2020.10")); +} + +#[test] +fn bootloader_survives_stacked_prefixes() { + // `--log-timestamps` over a Linux console stacks two prefixes. + let log = format!("[2026-09-23T10:00:00.000Z] [ 0.000000] {UBOOT_LINE}"); + assert_eq!(uboot_value(&log).as_deref(), Some("U-Boot 2020.10")); +} + +#[test] +fn bootloader_survives_ansi_and_timestamp_together() { + let log = format!("\x1b[1;33m[12:34:56] \x1b[0m{UBOOT_LINE}"); + assert_eq!(uboot_value(&log).as_deref(), Some("U-Boot 2020.10")); +} + +#[test] +fn evidence_keeps_the_original_prefixed_line() { + // Normalization is for matching only. What we show back to the + // user must be what their capture actually contained, otherwise + // they cannot find it again in the log. + let log = format!("noise\n[2026-09-23T10:00:00.000Z] {UBOOT_LINE}\nmore noise"); + let f = find(&log, "Bootloader").expect("Bootloader should fire"); + assert_eq!( + f.source.as_deref(), + Some(format!("[2026-09-23T10:00:00.000Z] {UBOOT_LINE}").as_str()) + ); + assert_eq!(f.line_number, Some(2), "line number must be 1-based"); +} + +#[test] +fn coreboot_and_grub_anchors_also_normalized() { + let coreboot = find("[12:34:56] coreboot-4.19 Tue Jan 1", "Bootloader"); + assert_eq!(coreboot.map(|f| f.value).as_deref(), Some("coreboot 4.19")); + + let grub = find("[2026-09-23T10:00:00.000Z] GRUB version 2.06", "Bootloader"); + assert_eq!(grub.map(|f| f.value).as_deref(), Some("GRUB 2.06")); +} + +#[test] +fn init_system_anchor_also_normalized() { + // `(?m)^procd:` is anchored too. + let f = find("[ 3.123456] procd: - early -", "Init system"); + assert!( + f.is_some(), + "procd should be detected behind a printk prefix" + ); +} + +#[test] +fn unanchored_detectors_are_unaffected_by_normalization() { + // Regression guard the other way: the detectors that already + // handled prefixes must keep working, and must not start matching + // things they shouldn't because of the stripping. + let log = "[ 0.000000] Linux version 5.15.137 (builder@buildhost) (gcc 11.2.0) #0 SMP"; + assert!(find(log, "Kernel").is_some()); +} + +#[test] +fn a_bracketed_non_timestamp_is_not_stripped() { + // Only timestamp-shaped brackets come off. A line that genuinely + // begins with some other bracketed tag keeps it, so evidence and + // matching stay honest. + let log = "[vendor-tag] U-Boot 2020.10 (Sep 17 2023 - 11:38:21 +0000)"; + assert!( + uboot_value(log).is_none(), + "a non-timestamp bracket must not be stripped by the timestamp rule" + ); +} + +#[test] +fn bare_cr_line_endings_are_split() { + // Some bootloaders emit CR-only line endings. `str::lines()` does + // not split on those, which would leave the whole capture as one + // line and defeat every anchored detector. + let log = format!("boot start\r{UBOOT_LINE}\rdone"); + assert_eq!(uboot_value(&log).as_deref(), Some("U-Boot 2020.10")); +}