diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index e4e67b71..df9d63cf 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -125,3 +125,11 @@ consequence — what they see, and which exit code they get — not just the cod This repo is **public**: never put a customer name, internal hostname, or internal-only ticket detail in a finding. A bare `tracebloc/backend#NNNN` reference is fine. + +## Working with Bugbot findings (team norm) + +Every Bugbot review thread gets a reply, then gets resolved: +- **Fixed**: say what changed and in which commit. +- **False positive**: say why, with evidence (file/line, measured behavior). +Unresolved cursor threads HOLD release-train promotions (soft gate) — an +unaddressed finding blocks the fleet, not just this PR. diff --git a/.github/workflows/code-quality-caller.yml b/.github/workflows/code-quality-caller.yml new file mode 100644 index 00000000..584c5578 --- /dev/null +++ b/.github/workflows/code-quality-caller.yml @@ -0,0 +1,22 @@ +name: Code quality + +on: + pull_request: + types: [opened, reopened, synchronize, ready_for_review] + +# Supersede the previous run when a branch is pushed again. Measured: +# workflows missing this stack ~10-minute duplicate runs per push. +concurrency: + group: code-quality-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + quality: + uses: tracebloc/.github/.github/workflows/code-quality.yml@main + with: + python: true # repos with Python + shell: true # repos with shell scripts + # soft-fail: false # flip once the backlog is clear diff --git a/.github/workflows/golangci.yml b/.github/workflows/golangci.yml new file mode 100644 index 00000000..257420c0 --- /dev/null +++ b/.github/workflows/golangci.yml @@ -0,0 +1,66 @@ +name: golangci-lint + +# Runs golangci-lint (with the gosec security linter) against the +# repo's .golangci.yml on every PR + push to develop/main. This is the +# "one tool, one config" successor being sized up for the standalone +# lint steps in build.yml, and the first thing in this repo that scans +# our own code for insecure patterns — govulncheck (build.yml + +# vulncheck.yml) only covers known CVEs in dependencies. +# +# Why the action is safe now: the golangci-lint-action timeout story +# (#6) was the SSA linters (staticcheck, unused) choking on the +# k8s.io/* dep tree. The current .golangci.yml enables no SSA linters; +# a full run measures ~11s wall locally / ~62s on the runner (#423). +# +# Part of backend#1305 (epic #930, Layer 1). + +on: + push: + branches: [develop, main] + pull_request: + branches: [develop, main] + +permissions: + contents: read + +concurrency: + group: golangci-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + golangci: + timeout-minutes: 10 + name: golangci-lint + runs-on: ubuntu-latest + # ============================= GATE ============================== + # Blocking by exit code since the backlog hit zero: the gosec + # findings sized on #423 were all resolved with per-site reviewed + # #nosec waivers (#427), so any finding this job reports from now + # on is NEW and fails the job — including typecheck errors, which + # ride the same exit path. History of the advisory era (the + # --issues-exit-code=0 flag, why job-level continue-on-error was + # not the tool) is in the #423/#426 discussions if you need it. + # Final step of the flip = marking this check required in branch + # protection (backend#1305 / epic #930). + # ================================================================== + steps: + - uses: actions/checkout@v7 + + - name: Set up Go + uses: actions/setup-go@v7 + with: + go-version-file: go.mod + cache: true + + # Both versions pinned for reproducibility, same policy as the + # standalone tools in build.yml (#127). golangci-lint v2.12.2 is + # built with Go 1.26 (required: go.mod says `go 1.26.0`; v1-era + # binaries can't typecheck this module). Bump deliberately, and + # keep the version in step with the format expectations noted in + # .golangci.yml AND with GOLANGCI_LINT_VERSION in the Makefile + # (make lint-full runs the same pinned version -- the local/CI + # mirror depends on the two never drifting). + - name: golangci-lint run (.golangci.yml) + uses: golangci/golangci-lint-action@v9.3.0 + with: + version: v2.12.2 diff --git a/.golangci.yml b/.golangci.yml index a6d52e76..4b12669d 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -4,9 +4,24 @@ # bugs without flooding PRs with style noise. Tune up over time # rather than turning everything on day one and quarantining half of # them. +# +# Format: golangci-lint v2 (`version: "2"`). The v1-format file died +# with golangci-lint v1: v1 binaries are EOL and predate Go 1.26, so +# they can't typecheck this module at all, and v2 binaries (what +# `brew install golangci-lint` ships) refuse v1 configs. Migrated via +# `golangci-lint migrate` (backend#1305, epic #930 Layer 1); needs +# golangci-lint >= v2. +# +# CI: run by the advisory `golangci-lint` job in +# .github/workflows/golangci.yml (pinned version there; keep it in +# lockstep when the format needs a newer binary). The old +# action-times-out story (#6) was about the SSA linters (staticcheck, +# unused) on the k8s.io dep tree — this set has none, so the action +# is safe with it. + +version: "2" run: - timeout: 5m # Track go.mod's `go` directive. Go's release cadence + `go mod # tidy`'s aggressive bumping (especially when k8s.io/* deps want # newer Go) keep dragging go.mod's minimum up; pinning a stale @@ -17,42 +32,73 @@ run: go: "1.26" linters: - disable-all: true + default: none enable: # The set is trimmed to the linters that DON'T do full whole-program # SSA analysis. `staticcheck` and `unused` were in the original set # and reproducibly caused the GitHub-hosted runner to time-budget # the job (~2 min then shutdown signal) on this module — k8s.io/* # transitive deps inflate the analysis graph enough to OOM-or-stall - # the standard 4-CPU/16GB runner. Re-enabling them is a v0.2 - # follow-up that needs either a larger runner, a much narrower - # scope (e.g. only `./internal/...`), or a faster successor like - # govulncheck for the security-only subset. + # the standard 4-CPU/16GB runner (#6). staticcheck now runs + # standalone in build.yml's Lint job instead. # Cheap, per-file checks — catch real bugs without SSA. - errcheck # unchecked error returns - govet # `go vet` - ineffassign # assignments that go nowhere + # Security: insecure code patterns (G1xx-G6xx) — command injection, + # path traversal, weak crypto, world-writable files. Complements + # govulncheck (known CVEs in deps) with our-own-code checks; this is + # a customer-installed binary that shells out and writes to disk, so + # both halves matter (backend#1305, epic #930 Layer 1). + - gosec + # Style / hygiene that pays for itself in code review time. - - gofmt - - goimports - misspell - unconvert # unnecessary type conversions -linters-settings: - goimports: - # Group imports: stdlib, third-party, our own. Keeps diffs - # readable when adding new imports. - local-prefixes: github.com/tracebloc/cli + exclusions: + # v1 had `exclude-use-default: false` — the default exclusions hide + # a lot of real findings, so keep opting back in (no presets). + generated: lax + rules: + # Test files often deliberately ignore err returns from + # bytes.Buffer / strings.Builder / fmt.Fprintf, which never fail. + # gosec is likewise test-exempt per convention: tests use fixed + # temp paths, os.Setenv, and relaxed perms that G-rules flag but + # that never ship in the binary. + - path: _test\.go + linters: + - errcheck + - gosec + paths: + - third_party$ + - builtin$ + - examples$ + +# v2 moved the formatters out of `linters`. Same tools as before — +# build.yml's Lint job runs the standalone equivalents (`gofmt -s`, +# `goimports -local`). +formatters: + enable: + - gofmt + - goimports + settings: + goimports: + # Group imports: stdlib, third-party, our own. Keeps diffs + # readable when adding new imports. + local-prefixes: + - github.com/tracebloc/cli + exclusions: + generated: lax + paths: + - third_party$ + - builtin$ + - examples$ issues: - # Default exclusions hide a lot of real findings — opt back in. - exclude-use-default: false - - exclude-rules: - # Test files often deliberately ignore err returns from - # bytes.Buffer / strings.Builder / fmt.Fprintf, which never fail. - - path: _test\.go - linters: - - errcheck + # Never cap repeated findings: the default (3) hid 10 of the 13 G304s + # behind a cache-flappy sample — "8 findings" were really 18 (#427). + # A gate must see the whole backlog, every run. + max-same-issues: 0 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 15123e47..2fe3a99a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -105,3 +105,7 @@ Coverage says a line *ran*; mutation testing says a test would *fail* if the lin 4. File one issue per real gap, titled `test(): pin (mutation survivor)`, quoting the gremlins line (mutant type + file:line) and what behavior the missing test must pin. That issue then flows through the kanban like any other test ticket — #262, #263, #264 are the pattern. Survivors are *findings to triage*, not build failures — the workflow stays green even when mutants live, on purpose. + +## Bugbot findings + +See `.cursor/BUGBOT.md` — every thread gets a reply (fixed / false-positive-with-evidence), then resolved. diff --git a/Makefile b/Makefile index 54e93dc6..d4151850 100644 --- a/Makefile +++ b/Makefile @@ -9,11 +9,12 @@ # ---- toggles ----------------------------------------------------- GO ?= go -GOLANGCI_LINT ?= golangci-lint PKGS := ./... # Pinned lint/analysis tool versions (reproducibility — no more @latest drift). -# Keep these in lockstep with .github/workflows/build.yml. Bump deliberately. +# Keep these in lockstep with .github/workflows/build.yml — and +# GOLANGCI_LINT_VERSION with .github/workflows/golangci.yml. Bump deliberately. +GOLANGCI_LINT_VERSION ?= v2.12.2 ERRCHECK_VERSION ?= v1.20.0 INEFFASSIGN_VERSION ?= v0.2.0 MISSPELL_VERSION ?= v0.3.4 @@ -24,8 +25,11 @@ GOIMPORTS_VERSION ?= v0.48.0 # ---- top-level targets ------------------------------------------- +# ci mirrors the PR gates exactly — including golangci-lint (lint-full), +# which fails on findings since #430. A green `make ci` must imply a green +# PR; lint-full's own guard tells you how to install the tool if missing. .PHONY: ci -ci: vet test lint fmt-check schema-check vulncheck file-budget deadcode check-style +ci: vet test lint lint-full fmt-check schema-check vulncheck file-budget deadcode check-style @echo "==> ci: all green" .PHONY: build @@ -126,15 +130,14 @@ deadcode: vulncheck: $(GO) run golang.org/x/vuln/cmd/govulncheck@$(GOVULNCHECK_VERSION) ./... +# Pinned to the exact version the golangci CI job runs (see +# .github/workflows/golangci.yml), via the same `go run tool@version` +# pattern as the tools above — no PATH dependency, so a green +# `make ci` and the PR gate can never disagree on golangci version. +# First run builds from source (~1-2 min); cached afterwards. .PHONY: lint-full lint-full: - @command -v $(GOLANGCI_LINT) >/dev/null 2>&1 || { \ - echo "==> $(GOLANGCI_LINT) not on PATH"; \ - echo " install via: brew install golangci-lint"; \ - echo " or see: https://golangci-lint.run/usage/install/"; \ - exit 1; \ - } - $(GOLANGCI_LINT) run + $(GO) run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) run .PHONY: fmt fmt: diff --git a/internal/cli/data.go b/internal/cli/data.go index 98421b86..cc7f8b86 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "strings" "github.com/spf13/cobra" @@ -165,7 +166,7 @@ func runDataIngest(ctx context.Context, out, errOut io.Writer, a runDataIngestAr // DROP/rm and then "succeed". plan := push.PlanTeardown(existingTable) rmSpin := a.Printer.Spinner(fmt.Sprintf("Removing the existing %q first", existingTable), "") - _, terr := push.Teardown(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, plan, push.PodSpecOptions{ + tres, terr := push.Teardown(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, plan, push.PodSpecOptions{ Namespace: resolved.Namespace, PVCClaimName: pvc.ClaimName, PVCMountPath: pvc.MountPath, @@ -185,6 +186,13 @@ func runDataIngest(ctx context.Context, out, errOut io.Writer, a runDataIngestAr "first, then re-run this ingest. Nothing new was staged. (%w)", existingTable, existingTable, terr)} } + if !tres.BookkeepingCleaned { + // Same surfacing `data delete` does (Bugbot on the PR): the + // overwrite pre-clean runs the identical teardown, and a silent + // bookkeeping failure here would hide the same schema-drift + // regression on this path. + a.Printer.Warnf("Bookkeeping cleanup incomplete — the old table is gone, but its run-journal/salt rows may remain: %s", strings.Join(tres.BookkeepingErrs, "; ")) + } a.Printer.Successf("Removed the old %q — ingesting the new data.", existingTable) } diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index 7b1e6d38..99614916 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -212,7 +212,7 @@ undone — re-ingesting the data is the only way back.`) p.Newline() p.Successf("Dry-run — nothing was deleted.") if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "dry-run", resolved.Namespace, release.ReleaseName, plan, nil) + writeDataDeleteJSON(a.JSONOut, "dry-run", resolved.Namespace, release.ReleaseName, plan, nil, false) jsonEmitted = true } return nil @@ -234,7 +234,7 @@ undone — re-ingesting the data is the only way back.`) // exit 0. One closure so the pair can't drift apart. declined := func() error { if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil) + writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil, false) jsonEmitted = true } return cleanCancel(p, "nothing was deleted.") @@ -283,9 +283,15 @@ undone — re-ingesting the data is the only way back.`) p.Newline() p.Successf("Deleted %s.%s and %d PVC path(s).", plan.Database, plan.Table, len(res.RemovedPaths)) + if !res.BookkeepingCleaned { + // Best-effort cleanup failed — say so, or a schema-drift regression + // (a renamed keying column) is indistinguishable from a legacy + // cluster without the bookkeeping tables (review, Saqlain). + p.Warnf("Bookkeeping cleanup incomplete — the table is gone, but its run-journal/salt rows may remain: %s", strings.Join(res.BookkeepingErrs, "; ")) + } p.Infof("The dataset's catalog metadata is kept as a record on tracebloc, marked unavailable — never removed.") if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "deleted", resolved.Namespace, release.ReleaseName, plan, res.RemovedPaths) + writeDataDeleteJSON(a.JSONOut, "deleted", resolved.Namespace, release.ReleaseName, plan, res.RemovedPaths, res.BookkeepingCleaned) jsonEmitted = true } return nil @@ -302,12 +308,17 @@ type dataDeleteJSON struct { Table string `json:"table"` // the REAL (case-resolved) spelling, not the raw argument PVCPaths []string `json:"pvc_paths"` RemovedPaths []string `json:"removed_paths"` + // BookkeepingCleaned mirrors push.TeardownResult: whether the + // run-journal/salt rows were removed with the table. Always false for + // dry-run/declined — nothing was attempted, and a strict consumer must + // never read "cleanup happened" out of a run that deleted nothing. + BookkeepingCleaned bool `json:"bookkeeping_cleaned"` } // writeDataDeleteJSON serializes the delete result to w (stdout in // --output-json mode). Marshal errors are dropped: marshaling our own // struct can't fail in practice, and the exit code remains the contract. -func writeDataDeleteJSON(w io.Writer, status, namespace, release string, plan push.TeardownPlan, removed []string) { +func writeDataDeleteJSON(w io.Writer, status, namespace, release string, plan push.TeardownPlan, removed []string, bookkeepingCleaned bool) { pvcPaths := plan.PVCPaths if pvcPaths == nil { pvcPaths = []string{} // emit [] not null @@ -316,13 +327,14 @@ func writeDataDeleteJSON(w io.Writer, status, namespace, release string, plan pu removed = []string{} // emit [] not null } res := dataDeleteJSON{ - Status: status, - Namespace: namespace, - Release: release, - Database: plan.Database, - Table: plan.Table, - PVCPaths: pvcPaths, - RemovedPaths: removed, + Status: status, + Namespace: namespace, + Release: release, + Database: plan.Database, + Table: plan.Table, + PVCPaths: pvcPaths, + RemovedPaths: removed, + BookkeepingCleaned: bookkeepingCleaned, } b, err := json.MarshalIndent(res, "", " ") if err != nil { diff --git a/internal/cli/home_local_fallback.go b/internal/cli/home_local_fallback.go index 405660fc..0afa1f6b 100644 --- a/internal/cli/home_local_fallback.go +++ b/internal/cli/home_local_fallback.go @@ -110,7 +110,7 @@ func tbAliasAvailable() bool { // different tracebloc at another path (Bugbot). Case-insensitive: .cmd is a // Windows artifact and NTFS paths are case-insensitive. func tbCmdAliasOurs(dir, exe string) bool { - b, err := os.ReadFile(filepath.Join(dir, binTB+".cmd")) + b, err := os.ReadFile(filepath.Join(dir, binTB+".cmd")) // #nosec G304 -- fixed name next to os.Executable(): inspects the install dir's own tb.cmd shim; whoever controls that dir already controls the binary. if err != nil { return false } diff --git a/internal/cli/ingest.go b/internal/cli/ingest.go index 748b827f..59354072 100644 --- a/internal/cli/ingest.go +++ b/internal/cli/ingest.go @@ -75,7 +75,7 @@ Exit codes: func runIngestValidate(cmd *cobra.Command, args []string) error { path := args[0] - body, err := os.ReadFile(path) + body, err := os.ReadFile(path) // #nosec G304 -- reading the ingest.yaml the operator named as the positional arg is this command's documented job; local CLI, invoking user's privileges. if err != nil { // fileError is exit-code 3 territory. We use a sentinel // exit-coded error so cobra propagates the right code via diff --git a/internal/cli/installlog.go b/internal/cli/installlog.go index 8c45a232..c85ab75c 100644 --- a/internal/cli/installlog.go +++ b/internal/cli/installlog.go @@ -34,7 +34,7 @@ func newInstallLog() (*installLog, string) { return nil, "" } path := filepath.Join(dir, "install-"+time.Now().UTC().Format("20060102-150405")+".log") - f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) // #nosec G304 -- creates (never reads) the install log at a timestamp-generated name under the CLI's own 0700 config dir, mode 0600. if err != nil { // No file was created — return an empty path so the caller never // advertises a "Full log:" location that doesn't exist (Bugbot). diff --git a/internal/cli/prepare_host.go b/internal/cli/prepare_host.go index d946aeba..cdfd8052 100644 --- a/internal/cli/prepare_host.go +++ b/internal/cli/prepare_host.go @@ -110,7 +110,7 @@ func prepareHostEnv(user string) []string { // that traps signals. We rely on the default SIGKILL rather than a custom // SIGINT-only Cancel (which a privileged child could ignore, hanging Wait). func prepareHostCmd(ctx context.Context) *exec.Cmd { - c := exec.CommandContext(ctx, "bash", "-c", prepareHostInstallerCmd) + c := exec.CommandContext(ctx, "bash", "-c", prepareHostInstallerCmd) // #nosec G204 -- argv is compile-time constant: literal "bash" -c installerRunScript("prepare-host"), built only from the installerURL const; no runtime input. c.WaitDelay = 5 * time.Second return c } diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index f2341e53..46dd8d40 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -60,6 +60,7 @@ screen. %s/%d are runtime placeholders. "%s — %s" "%s, … and %d more" "%s/%s" +"%s: %v" "%s: %w" "%s=%s,%s=%s" "%v (policy: %v)" @@ -96,6 +97,8 @@ screen. %s/%d are runtime placeholders. "Already signed out." "Applies to your next training run; a run already going keeps its size." "Ask one of these admins (or ask them to grant you access)" +"Bookkeeping cleanup incomplete — the old table is gone, but its run-journal/salt rows may remain: %s" +"Bookkeeping cleanup incomplete — the table is gone, but its run-journal/salt rows may remain: %s" "CPU cores for one run (1–%d)" "CSV %s has no columns" "Can't reach tracebloc from here." @@ -140,6 +143,7 @@ screen. %s/%d are runtime placeholders. "Ctrl-C to cancel" "Ctrl-C to stop watching — the run keeps going on the cluster" "DB failures" +"DELETE FROM `%s`.`%s` WHERE table_name='%s'" "DROP TABLE IF EXISTS `%s`.`%s`" "Datasets in %s (0)" "Datasets in %s — %d" @@ -553,6 +557,7 @@ screen. %s/%d are runtime placeholders. "resource env" "restarted ≥%d times — check logs: %v" "root" +"running mysql query: %w%s" "scanning the cluster for tracebloc clients: %w" "schema" "schema entry %q must be col:TYPE (e.g. age:INT,price:FLOAT)" diff --git a/internal/cli/update_check.go b/internal/cli/update_check.go index 8f317c48..f8214bd4 100644 --- a/internal/cli/update_check.go +++ b/internal/cli/update_check.go @@ -164,7 +164,7 @@ func readUpdateCache(path string) (updateCache, bool) { if path == "" { return updateCache{}, false } - raw, err := os.ReadFile(path) + raw, err := os.ReadFile(path) // #nosec G304 -- the CLI's own throttle cache: config.Dir() + the constant updateCacheFile name; contents JSON-validated before use. if err != nil { return updateCache{}, false } diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index 9c0d8b6f..686c1057 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -120,7 +120,7 @@ Safe to run anytime; safe to re-run.`, // Stream the installer straight to the user's terminal, and keep // stdin wired so its interactive prompts (sign-in, etc.) still work. ctx := cmd.Context() - c := exec.CommandContext(ctx, plan.name, plan.args...) + c := exec.CommandContext(ctx, plan.name, plan.args...) // #nosec G204 -- upgradePlanFor(runtime.GOOS) yields compile-time constants: "bash" -c installerRunScript(""); only the GOOS branch varies, no user input. c.Stdin, c.Stdout, c.Stderr = os.Stdin, os.Stdout, os.Stderr if err := c.Run(); err != nil { // User aborted (Ctrl-C) or the parent context was cancelled: exit diff --git a/internal/config/config.go b/internal/config/config.go index 5490ff3d..1e020c7d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -122,7 +122,7 @@ func Load() (*Config, error) { if err != nil { return nil, err } - data, err := os.ReadFile(path) + data, err := os.ReadFile(path) // #nosec G304 -- the CLI's own config: Dir()/config.json under ~/.tracebloc or the operator's explicit $TRACEBLOC_CONFIG_DIR override, read as the invoking user. if errors.Is(err, fs.ErrNotExist) { return &Config{Version: schemaVersion, Profiles: map[string]*Profile{}}, nil } diff --git a/internal/helm/upgrade.go b/internal/helm/upgrade.go index 67e713b7..aa0f8429 100644 --- a/internal/helm/upgrade.go +++ b/internal/helm/upgrade.go @@ -53,7 +53,7 @@ const ( // var so tests substitute a fake without spawning real helm. Mirrors // nodeboot.Runner exactly. var Runner = func(ctx context.Context, name string, args ...string) (string, error) { - out, err := exec.CommandContext(ctx, name, args...).CombinedOutput() + out, err := exec.CommandContext(ctx, name, args...).CombinedOutput() // #nosec G204 -- test seam: every caller passes the literal "helm"; args are the operator's own release/kubeconfig flags, exec'd as an argv array, no shell. return string(out), err } diff --git a/internal/nodeboot/nodeboot.go b/internal/nodeboot/nodeboot.go index 82599d84..db29f968 100644 --- a/internal/nodeboot/nodeboot.go +++ b/internal/nodeboot/nodeboot.go @@ -32,7 +32,7 @@ const imageReference = "ghcr.io/tracebloc/*" // Runner executes an external command and returns its combined output. A package // var so tests can substitute a fake without spawning real k3d/helm/docker. var Runner = func(ctx context.Context, name string, args ...string) (string, error) { - out, err := exec.CommandContext(ctx, name, args...).CombinedOutput() + out, err := exec.CommandContext(ctx, name, args...).CombinedOutput() // #nosec G204 -- test seam: callers pass literal tool names (k3d/helm/docker) with argv from package consts and the operator's own cluster/release names; no shell. return string(out), err } diff --git a/internal/push/detect.go b/internal/push/detect.go index cdcf5444..b82031c0 100644 --- a/internal/push/detect.go +++ b/internal/push/detect.go @@ -27,7 +27,7 @@ import ( // default, advising --target-size. (Since Discover only yields the // ingestor's accept-set — .jpg/.jpeg/.png — that path is defensive.) func DetectImageSize(path string) (width, height int, err error) { - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 -- decodes the header of an image the symlink-rejecting dataset walk found under the operator-chosen root; operator's own file. if err != nil { return 0, 0, err } diff --git a/internal/push/image_resolution.go b/internal/push/image_resolution.go index e33d8af8..64dcdf59 100644 --- a/internal/push/image_resolution.go +++ b/internal/push/image_resolution.go @@ -27,7 +27,7 @@ import ( func scanImageResolutions(paths []string, expectedW, expectedH, minW, minH int) (broken, tooSmall, mismatched []string) { for _, path := range paths { name := filepath.Base(path) - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 -- paths come from the symlink-rejecting dataset walk of the operator-chosen root; mirrors the in-cluster validator on the operator's own files. if err != nil { broken = append(broken, fmt.Sprintf("%s (unreadable: %v)", name, err)) continue diff --git a/internal/push/list_detailed.go b/internal/push/list_detailed.go index 2e0cd4f6..1ce5811e 100644 --- a/internal/push/list_detailed.go +++ b/internal/push/list_detailed.go @@ -288,7 +288,7 @@ func runMySQLQuery(ctx context.Context, exec Executor, namespace, pod, container script := `mysql -uroot -p"$MYSQL_ROOT_PASSWORD" -N` if err := exec.Exec(ctx, namespace, pod, container, []string{"sh", "-c", script}, strings.NewReader(query), &stdout, &stderr); err != nil { - return "", fmt.Errorf("querying datasets: %w%s", err, stderrSuffix(&stderr)) + return "", fmt.Errorf("running mysql query: %w%s", err, stderrSuffix(&stderr)) } return stdout.String(), nil } diff --git a/internal/push/preflight.go b/internal/push/preflight.go index 3315b275..8dc11305 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -42,7 +42,7 @@ var utf8BOM = []byte{0xEF, 0xBB, 0xBF} // non-EOF Read error is only ever a genuine I/O failure, so callers can treat // it as fail-closed. The caller closes the returned Closer. func openCSVReader(path string) (*csv.Reader, io.Closer, error) { - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 -- opens the operator's own dataset CSV to mirror the ingestor's checks locally before upload; no privilege boundary crossed. if err != nil { return nil, nil, err } @@ -78,7 +78,7 @@ func matchColumnIndex(header []string, want string) int { // HasBOM reports whether the file starts with a UTF-8 BOM. func HasBOM(path string) (bool, error) { - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 -- 3-byte BOM sniff of the same operator-supplied dataset CSV openCSVReader reads; local preflight as the invoking user. if err != nil { return false, err } @@ -802,7 +802,7 @@ func hasKnownExtension(name string) bool { // the FIRST gate validate_data runs in-cluster: the CSV must be valid UTF-8 // and free of NUL bytes, or the whole run aborts (after the upload). func CheckCSVEncoding(path string) error { - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 -- encoding-check of the operator's own dataset CSV, size-capped by LimitReader; same local-preflight threat model as openCSVReader. if err != nil { return fmt.Errorf("reading %s: %w", filepath.Base(path), err) } diff --git a/internal/push/stream.go b/internal/push/stream.go index f26233cd..74330224 100644 --- a/internal/push/stream.go +++ b/internal/push/stream.go @@ -469,7 +469,7 @@ func writeTarFile(tw *tar.Writer, src, dst string) (int64, error) { if err := tw.WriteHeader(hdr); err != nil { return 0, err } - f, err := os.Open(src) + f, err := os.Open(src) // #nosec G304 -- src came from the symlink-rejecting walk and the Lstat guard above re-rejects symlinks at stream time; reads the operator's own dataset file. if err != nil { return 0, err } diff --git a/internal/push/tabular.go b/internal/push/tabular.go index 05d52e92..319465f1 100644 --- a/internal/push/tabular.go +++ b/internal/push/tabular.go @@ -388,7 +388,7 @@ type SchemaInference struct { // redeclares them. The risky cases (empty-in-sample, id-like) are returned // alongside the schema so the caller can surface them as warnings. func InferSchema(csvPath string) (*SchemaInference, error) { - f, err := os.Open(csvPath) + f, err := os.Open(csvPath) // #nosec G304 -- csvPath is the dataset CSV DiscoverTabular's symlink-rejecting walk found under the operator-chosen root; local read as the invoking user. if err != nil { return nil, err } diff --git a/internal/push/teardown.go b/internal/push/teardown.go index b917d6f6..e81487f9 100644 --- a/internal/push/teardown.go +++ b/internal/push/teardown.go @@ -66,6 +66,18 @@ func PlanTeardown(table string) TeardownPlan { type TeardownResult struct { DroppedTable bool RemovedPaths []string + // BookkeepingCleaned reports whether the ingestor's bookkeeping rows + // for the table (run-journal + pseudonymization salt) were deleted + // alongside it. Best-effort: false on clusters whose ingestor never + // created those tables — the teardown itself still succeeds. + BookkeepingCleaned bool + // BookkeepingErrs carries the per-table failure detail (which + // bookkeeping table, mysql's stderr folded into the error) so callers + // can SURFACE it: a silent false is indistinguishable from the + // schema-drift regression this cleanup exists to prevent — e.g. a + // renamed keying column would otherwise no-op invisibly, reopening + // the husk-row leak (review, Saqlain). + BookkeepingErrs []string } // Teardown performs the in-cluster teardown described by plan: @@ -107,6 +119,39 @@ func Teardown(ctx context.Context, cs kubernetes.Interface, exec Executor, names } res.DroppedTable = true + // 1b. Best-effort bookkeeping cleanup (RFC-0003 I6 — tracebloc/backend#1209): + // the ingestor keeps one run-journal row per ingest and one + // pseudonymization-salt row per table; dropping the table alone + // strands them. Under per-ingestion tables (data-ingestors#408) + // every dataset is its own table, so every delete would leave one + // husk row of each kind — an unbounded slow leak. plan.Table passed + // ValidateTableName ([A-Za-z_][A-Za-z0-9_]*), so it cannot escape + // the single-quoted literal. Each DELETE runs separately and + // best-effort: either bookkeeping table may be absent on clusters + // that never ran a journal-aware ingestor, and these are metadata + // rows, not data — never fail a teardown whose DROP succeeded. + // The SQL rides runMySQLQuery — stdin, never a shell -e argument: the + // string literal's single quotes would terminate a single-quoted shell + // string and mysql would see an unquoted identifier, silently no-oping + // the DELETEs forever (Bugbot on the PR). Column contract, pinned + // against data-ingestors tracebloc_ingestor/database.py: BOTH + // bookkeeping tables key these rows by `table_name` — + // RUNS_TABLE tracebloc_ingest_runs (ingestor_id PK, table_name + // indexed via ix_tracebloc_ingest_runs_table) + // SALT_TABLE tracebloc_ingest_meta (table_name PK, salt) + // Each DELETE stays a separate best-effort call: batched on one stdin, + // a missing first table would abort the second (mysql stops on error). + res.BookkeepingCleaned = true + for _, bookkeeping := range []string{ingestRunsTable, ingestMetaTable} { + cleanupSQL := fmt.Sprintf("DELETE FROM `%s`.`%s` WHERE table_name='%s'", + plan.Database, bookkeeping, plan.Table) + if _, err := runMySQLQuery(ctx, exec, namespace, mysqlPod, mysqlContainer, cleanupSQL); err != nil { + res.BookkeepingCleaned = false + res.BookkeepingErrs = append(res.BookkeepingErrs, + fmt.Sprintf("%s: %v", bookkeeping, err)) + } + } + // 2. rm the PVC dirs from an ephemeral stage-identity pod (see the // doc note above + #259). The pod owns the staging files it // deletes, so this works on hostPath and CSI. diff --git a/internal/push/teardown_test.go b/internal/push/teardown_test.go index df514ed8..1b2bc506 100644 --- a/internal/push/teardown_test.go +++ b/internal/push/teardown_test.go @@ -3,6 +3,8 @@ package push import ( "context" "errors" + "fmt" + "io" "strings" "testing" @@ -183,3 +185,103 @@ func TestCleanStaging_PodCreateFailureReturnsError(t *testing.T) { t.Errorf("rm ran (%v) despite the pod never being created", fe.gotCmd) } } + +// TestTeardown_CleansBookkeepingRows pins the RFC-0003 I6 half of teardown +// (tracebloc/backend#1209): after the DROP, the ingestor's run-journal and +// salt rows for the table are deleted best-effort — and a failure there +// never fails a teardown whose DROP already succeeded. +func TestTeardown_CleansBookkeepingRows(t *testing.T) { + newCS := func() *fake.Clientset { + cs := fake.NewClientset(&corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "mysql-0", Namespace: "tracebloc"}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "mysql"}}}, + Status: corev1.PodStatus{Phase: corev1.PodRunning}, + }) + readyOnNextGet(cs) + return cs + } + opts := PodSpecOptions{ + Namespace: "tracebloc", + PVCClaimName: "client-pvc", + PVCMountPath: "/data/shared", + Table: "ds_0f2ab1de3c444e558f66778899aabbcc", + } + plan := PlanTeardown("ds_0f2ab1de3c444e558f66778899aabbcc") + + t.Run("journal and salt rows are deleted for the dropped table", func(t *testing.T) { + rec := &recordingExecutor{} + res, err := Teardown(context.Background(), newCS(), rec, "tracebloc", plan, opts) + if err != nil { + t.Fatalf("Teardown: %v", err) + } + if !res.BookkeepingCleaned { + t.Error("BookkeepingCleaned = false, want true") + } + // The SQL must arrive on STDIN with its quoted literal intact — + // never through a shell -e argument, where the literal's single + // quotes would be eaten by the shell (Bugbot: the DELETEs would + // silently no-op forever). + var journal, salt bool + for _, call := range rec.calls { + if strings.Contains(strings.Join(call.cmd, " "), "DELETE FROM") { + t.Errorf("DELETE passed as a shell argument (%q) — must be fed on stdin", call.cmd) + } + stdin := string(call.stdin) + want := "WHERE table_name='" + plan.Table + "'" + if strings.Contains(stdin, "DELETE FROM") && strings.Contains(stdin, ingestRunsTable) && strings.Contains(stdin, want) { + journal = true + } + if strings.Contains(stdin, "DELETE FROM") && strings.Contains(stdin, ingestMetaTable) && strings.Contains(stdin, want) { + salt = true + } + } + if !journal { + t.Errorf("no stdin DELETE against %s with a quoted literal for %s observed", ingestRunsTable, plan.Table) + } + if !salt { + t.Errorf("no stdin DELETE against %s with a quoted literal for %s observed", ingestMetaTable, plan.Table) + } + }) + + t.Run("bookkeeping failure never fails the teardown", func(t *testing.T) { + rec := &recordingExecutor{failWhenStdinContains: "DELETE FROM"} + res, err := Teardown(context.Background(), newCS(), rec, "tracebloc", plan, opts) + if err != nil { + t.Fatalf("Teardown should tolerate bookkeeping failures, got: %v", err) + } + if !res.DroppedTable { + t.Error("DroppedTable = false, want true") + } + if res.BookkeepingCleaned { + t.Error("BookkeepingCleaned = true, want false when the DELETEs fail") + } + if len(res.RemovedPaths) == 0 { + t.Error("PVC rm did not run — bookkeeping failure must not short-circuit step 2") + } + }) +} + +// recordingExecutor records every Exec call (command AND stdin) and can +// fail calls whose stdin matches a marker. +type recordingExecutor struct { + calls []execCall + failWhenStdinContains string +} + +type execCall struct { + pod, container string + cmd []string + stdin []byte +} + +func (r *recordingExecutor) Exec(ctx context.Context, namespace, pod, container string, cmd []string, stdin io.Reader, stdout, stderr io.Writer) error { + var in []byte + if stdin != nil { + in, _ = io.ReadAll(stdin) + } + r.calls = append(r.calls, execCall{pod: pod, container: container, cmd: cmd, stdin: in}) + if r.failWhenStdinContains != "" && strings.Contains(string(in), r.failWhenStdinContains) { + return fmt.Errorf("simulated bookkeeping failure") + } + return nil +} diff --git a/internal/push/text.go b/internal/push/text.go index 6cb73e13..6cfb089d 100644 --- a/internal/push/text.go +++ b/internal/push/text.go @@ -162,7 +162,7 @@ func validateTextRecords(csvPath, dirName string, files []string, rf RecordForma if path == "" { continue // manifest names a file not on disk — a missing-file check's job, not ours } - content, err := os.ReadFile(path) + content, err := os.ReadFile(path) // #nosec G304 -- path comes from byBase/byStem, keyed only by files the symlink-vetted walk found in the dataset dir; a manifest entry selects among them, it cannot point elsewhere. if err != nil { return fmt.Errorf("reading %s: %w", filepath.Join(dirName, filepath.Base(path)), err) } diff --git a/internal/slug/slug.go b/internal/slug/slug.go index fb3f8a08..17091a84 100644 --- a/internal/slug/slug.go +++ b/internal/slug/slug.go @@ -87,7 +87,7 @@ func toASCII(s string) string { var b strings.Builder for _, r := range norm.NFKD.String(s) { if r < 128 { - b.WriteByte(byte(r)) + b.WriteByte(byte(r)) // #nosec G115 -- false positive: range-over-string runes are non-negative and the r < 128 guard bounds them, so byte(r) is a lossless ASCII conversion. } } return b.String() diff --git a/scripts/install.sh b/scripts/install.sh index 312ee668..5886c611 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -173,11 +173,13 @@ ensure_cosign() { csums="$TMP/cosign_checksums.txt" echo " cosign not found — bootstrapping pinned ${COSIGN_VERSION} to verify the signature..." - # --tlsv1.2 floor for the cosign bootstrap fetch, matching the client - # installer's curls — never negotiate below TLS 1.2 to pull the verifier we - # then trust to authenticate the release. - if ! curl -fsSL --tlsv1.2 "$cbase/$casset" -o "$cbin" 2>/dev/null; then return 1; fi - if ! curl -fsSL --tlsv1.2 "$cbase/cosign_checksums.txt" -o "$csums" 2>/dev/null; then return 1; fi + # dl() carries the TLS 1.2 floor + stall-based bounding (see the helper) — + # never negotiate below TLS 1.2 to pull the verifier we then trust to + # authenticate the release, and never let a dead endpoint wedge the + # install. No wall-clock cap: the ~90MB cosign binary must be allowed to + # finish on slow links (review #426). + if ! dl "$cbase/$casset" "$cbin" 2>/dev/null; then return 1; fi + if ! dl "$cbase/cosign_checksums.txt" "$csums" 2>/dev/null; then return 1; fi cwant="$(grep " ${casset}\$" "$csums" | awk '{print $1}' | head -1)" [ -n "$cwant" ] || return 1 @@ -202,7 +204,7 @@ resolve_tag() { # Use the redirect-trail of /releases/latest to learn the tag — # avoids hitting the rate-limited /api/repos endpoint for the # zero-auth one-liner case. - redirect_url="$(curl -fsSI --tlsv1.2 \ + redirect_url="$(curl -fsSI --tlsv1.2 --connect-timeout 30 --max-time 30 \ "https://github.com/${GITHUB_REPO}/releases/latest" \ | awk '/^[Ll]ocation:/ { print $2 }' \ | tr -d '\r')" @@ -261,17 +263,28 @@ echo "Installing tracebloc CLI $TAG ($OS/$ARCH)..." BINARY_FILE="${BINARY_NAME}-${TAG}-${OS}-${ARCH}" BASE_URL="https://github.com/${GITHUB_REPO}/releases/download/${TAG}" +# Shared download profile for every body fetch in this script (review: #426). +# Stall-based bounding instead of a wall-clock cap: --max-time 300 made the +# ~50MB binary fail under ~1.4 Mbps and the ~90MB cosign bootstrap under +# ~2.6 Mbps — links that are slow but alive must be allowed to finish, while +# a dead connection (under 1 KiB/s for 60s straight) still aborts instead of +# wedging the install. TLS floor stays 1.2. Retune here, once. +# usage: dl +dl() { + curl -fsSL --tlsv1.2 --connect-timeout 30 --speed-limit 1024 --speed-time 60 "$1" -o "$2" +} + TMP="$(mktemp -d)" trap 'rm -rf "$TMP"' EXIT INT TERM echo "Downloading binary..." -if ! curl -fsSL --tlsv1.2 "$BASE_URL/$BINARY_FILE" -o "$TMP/$BINARY_FILE"; then +if ! dl "$BASE_URL/$BINARY_FILE" "$TMP/$BINARY_FILE"; then echo "Error: failed to download $BASE_URL/$BINARY_FILE" >&2 exit 1 fi echo "Downloading SHA256SUMS..." -if ! curl -fsSL --tlsv1.2 "$BASE_URL/SHA256SUMS" -o "$TMP/SHA256SUMS"; then +if ! dl "$BASE_URL/SHA256SUMS" "$TMP/SHA256SUMS"; then echo "Error: failed to download SHA256SUMS — release may be malformed" >&2 exit 1 fi @@ -345,8 +358,8 @@ verify_cosign_signature() { fi echo "Verifying cosign signature..." - if ! curl -fsSL --tlsv1.2 "$BASE_URL/$BINARY_FILE.sig" -o "$TMP/$BINARY_FILE.sig" 2>/dev/null \ - || ! curl -fsSL --tlsv1.2 "$BASE_URL/$BINARY_FILE.cert" -o "$TMP/$BINARY_FILE.cert" 2>/dev/null; then + if ! dl "$BASE_URL/$BINARY_FILE.sig" "$TMP/$BINARY_FILE.sig" 2>/dev/null \ + || ! dl "$BASE_URL/$BINARY_FILE.cert" "$TMP/$BINARY_FILE.cert" 2>/dev/null; then if [ "$ALLOW_UNVERIFIED" = "1" ]; then echo " WARNING: .sig/.cert not published for $TAG — signature NOT verified" >&2 echo " (TRACEBLOC_ALLOW_UNVERIFIED=1)." >&2 diff --git a/scripts/sync-backend-fixtures.sh b/scripts/sync-backend-fixtures.sh index a58b4761..4bf4f800 100755 --- a/scripts/sync-backend-fixtures.sh +++ b/scripts/sync-backend-fixtures.sh @@ -133,8 +133,10 @@ sync_one() { # sync_one is called as `if ! sync_one ...`, which suspends `set -e` for the # whole body — so check curl's exit explicitly and report the real fetch # failure instead of misdiagnosing an empty temp file as "not valid JSON". - # --tlsv1.2 matches every other curl in the repo (scripts/install.sh). - curl -fsSL --tlsv1.2 \ + # --tlsv1.2 matches every other curl in the repo (scripts/install.sh); the + # time bounds keep a hung GitHub API from wedging CI (18 fixtures per run, + # so a generous per-file ceiling would compound). + curl -fsSL --tlsv1.2 --connect-timeout 10 --max-time 60 \ -H "Authorization: Bearer ${TOKEN}" \ -H "Accept: application/vnd.github.raw+json" \ -H "X-GitHub-Api-Version: 2022-11-28" \ diff --git a/scripts/sync-schema.sh b/scripts/sync-schema.sh index 1fa12e5c..e6ee10ad 100755 --- a/scripts/sync-schema.sh +++ b/scripts/sync-schema.sh @@ -112,8 +112,9 @@ sync_one() { # whole body — so a failed curl (e.g. a 404) would otherwise fall through and # be misdiagnosed as "not valid JSON" on the empty temp file. Check curl's # exit explicitly and report the real fetch failure. --tlsv1.2 matches every - # other curl in the repo (scripts/install.sh). - curl -fsSL --tlsv1.2 "$url" -o "$tmp" + # other curl in the repo (scripts/install.sh); the time bounds keep a hung + # endpoint from wedging CI. + curl -fsSL --tlsv1.2 --connect-timeout 10 --max-time 60 "$url" -o "$tmp" local curl_rc=$? if [[ $curl_rc -ne 0 ]]; then echo "error: failed to fetch $url (curl exited $curl_rc)" >&2 diff --git a/scripts/tests/install-verify.sh b/scripts/tests/install-verify.sh index ed388ad2..f4993949 100755 --- a/scripts/tests/install-verify.sh +++ b/scripts/tests/install-verify.sh @@ -13,7 +13,10 @@ # served from a temp dir. No network, no real download. This harness is bash # (for arrays/locals); the script under test stays POSIX sh. # ============================================================================= -set -u +# pipefail so a failing pipeline producer (sha256sum | awk in _sha) can't be +# masked by its last stage exiting 0. Deliberately NO -e: this harness counts +# pass/fail itself and must keep running after a failed assertion. +set -uo pipefail SELF_DIR="$(cd "$(dirname "$0")" && pwd)" INSTALLER="$SELF_DIR/../install.sh"