Security dogfood hardening + vxd watch matcher fix + preflight scanner check#111
Merged
Conversation
…notate accepted findings Ran vxd security scan on vxd itself (346 findings) and closed the high-severity set: - Pin all 14 GitHub Actions references in ci.yml to full commit SHAs (mutable-tag supply-chain class, CWE-1357) with version comments. - Annotate the 29 accepted-by-design findings with #nosec / nosemgrep and a one-line rationale each: sampler seed conversion + math/rand (statistical sampling, crypto seed), server shutdown contexts (fresh ctx after parent cancel is the graceful-shutdown idiom), G703 path-taint sites (paths derive from $HOME/worktrees inside the operator trust boundary), and the 15 dangerous-exec-command sites that ARE the orchestrator's core function (each with its upstream validation named). vxd security scan . now reports 0 high+ findings on its own tree, so the scan is usable as a self-gate (--min high) once CI billing is restored.
…t tools The per-story security gate degrades gracefully when a scanner binary is absent (skipped, never fatal), which left operators with no signal that scan coverage was reduced. New CheckSecurityScanners (WARNING tier) lists missing binaries from the security.KnownScanners registry with install hints (security.InstallHint) and joins AllChecks — vxd preflight now runs 16 checks. lookPath is injected for testability, matching CheckBinaryPath's pattern. 4 new tests including a dangling-wire guard (AllChecks must include the check). Docs updated (CLAUDE.md + README check counts, security-agent section).
…nts + test coverage restore Two matcher bugs made `vxd watch` a silent no-op tail in production: 1. Story events: eventMatchesReq compared evt.StoryID[:8] against reqID[:8], but story IDs are namespaced with storyIDPrefix(reqID) — sha256(reqID)[:8] for any reqID longer than 8 chars, which every real ULID reqID is. The prefixes never matched, so no story event ever printed. The matcher now uses the exported engine.StoryIDPrefix (single source of truth). 2. Requirement events: the code commented 'REQ_* events get routed via payload below' but no payload routing existed — REQ_SUBMITTED/PLANNED/COMPLETED/ BLOCKED never printed. Now matched via the payload req_id/id keys (the two keys real emitters use: planner uses 'id', the planning heartbeat 'req_id'). The old TestEventMatchesReq_PrefixMatch pinned the broken raw-prefix behavior and was replaced (test was wrong, not the spec). New tests: hashed-prefix match, short-reqID verbatim match, payload routing for both key spellings, cross-requirement rejection, and an end-to-end tailRequirementEvents run against real file+sqlite stores that pins print-and-exit-on-terminal. Also restores the internal/cli coverage regression from PR #109 (68.0% → 72.9%): the security scan/kb commands, dashboard status/stop daemon commands, and watch were all untested. New: security_test.go (9 tests — pure helpers, kb text/json, scan with empty PATH pinning graceful degradation + skipped- scanner reporting), dashboard_daemon_test.go (7 tests — not-running status, idempotent stop, malformed/stale pidfiles, watch unknown-req error).
…kill dead branch + inert assertions
- auth.go: the cookie site had the nosemgrep half of the annotation but gosec
G124 still fired; add the #nosec with the same rationale.
- watch.go: drop the unreachable evt.StoryID == prefix branch — the planner
always emits <prefix>-<suffix> IDs, so HasPrefix covers every real case.
- checks_security_test.go: the installed-scanner negative assertions matched
a substring ('missing: gitleaks') that could never occur in the real message
format; assert on the bare scanner names so a regression can actually fire.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improvement pass driven by running
vxd security scanon vxd itself (dogfooding the new security agent from #109) plus a coverage/bug sweep of what that PR left untested.1. Dogfood scan hardening (chore)
ci.ymlto full commit SHAs (mutable-tag supply-chain class, CWE-1357), with version comments. All six pins verified against the GitHub API, including the annotated-tag dereference for golangci-lint-action.#nosec/nosemgrepand a one-line rationale each (sampler seed, graceful-shutdown contexts, path-taint sites inside the operator trust boundary, and the 15 exec sites that are the orchestrator's core function — each naming its upstream validation).vxd security scan .reports 0 high+ findings on its own tree, so--min highis usable as a self-gate once CI billing is restored.2. Bug fix:
vxd watchsilently dropped every event (fix)eventMatchesReqcomparedevt.StoryID[:8]toreqID[:8], but story IDs are namespaced withstoryIDPrefix(reqID)— sha256-hashed for every real ULID reqID — so no story event ever matched in production. REQ_* payload routing was promised in a comment but never implemented. Now matched via the newly exportedengine.StoryIDPrefixplus payloadreq_id/idkeys (both spellings real emitters use). The old test pinned the broken behavior and was replaced; 8 new tests including an end-to-end tail against real file+sqlite stores.3. New preflight check:
security_scanners(feat)The security gate degrades gracefully when a scanner binary is missing (skipped, never fatal) — which meant zero operator signal that coverage shrank.
CheckSecurityScanners(WARNING tier, check 16) lists missing binaries from thesecurity.KnownScannersregistry with install hints. InjectablelookPathmatching the package's testability pattern; includes a dangling-wire guard test.4. Coverage regression restore (test)
internal/clihad regressed 72.9% → 68.0% (PR #109's security commands, dashboard daemon commands, and watch landed untested). Restored to 72.9% with 16 new tests: security scan/kb (including the empty-PATH graceful-degradation path pinning skipped-scanner reporting), dashboard status/stop idempotency + stale/malformed pidfiles, watch matcher + tail.Review
everything-claude-code:go-reviewerpass: verdict WARN — 1 MEDIUM + 2 LOW, all applied in the final commit (completed the G124 half of the cookie annotation, removed a dead branch, fixed inert test assertions). All SHA pins and suppressions independently verified by the reviewer.Test plan
go build ./...clean, binary at~/.local/bin/vxdgo test ./... -count=1— all 30 packages passgo vet ./...clean;golangci-lint run— 0 issuesvxd security scan . --json— 0 high+ findingsvxd preflightlive —✓ Security scanners installed: gitleaks, semgrep, gosec, govulncheck, npm