fix: address security review findings (1 CRITICAL, 5 HIGH, 3 MEDIUM) - #1
Merged
Conversation
- secrets: rewrite broken relativeToRoot using filepath.Rel (err was a bool, os.Getwd result discarded) - history: add safePath() guard against path traversal in LoadScan/LoadScanResult - history: tighten scan dir/file perms (0755->0700, 0644->0600) - engine: drop dead Config fields (MinScore, OutputJSON, Verbose) and unused DefaultConfig - sarif: derive stable ruleID from module+CWE instead of loop index - html: remove dead Summary field with mismatched type - audit: replace hand-rolled resolveAbsPath with filepath.Abs - audit/scan: defer Close() on GITHUB_OUTPUT fd - markdown: differentiate GradeB emoji from GradeA Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Engine: - Bound Threads to >= 1 to prevent deadlock/panic on invalid config - Add context.Context to Module.Run interface for per-module timeout - Aggregate per-module errors via ScoreResult.Errors and (ScoreResult, error) - Guard against nil Config / nil Engine - Protect modules map with sync.RWMutex OSV scanner (pkg/local/deps.go): - Use /v1/querybatch endpoint (1 request per ecosystem instead of N) - Propagate context.Context through HTTP calls (NewRequestWithContext) - Retry with exponential backoff on 429/5xx (3 attempts max) - Surface scan errors instead of silently swallowing them - Cap response body via io.LimitReader (1 MiB) - Stream lockfiles with bufio.Scanner instead of strings.Split Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Multi-agent security review surfaced one CRITICAL, five HIGH and three MEDIUM issues across
cmd/,pkg/history,pkg/engine,pkg/local, andpkg/report. This PR fixes all of them.Fixes
🔴 CRITICAL
pkg/local/secrets.go—relativeToRootwas broken:strings.CutPrefixreturns(string, bool)but the code treated theboolas anerror, and theos.Getwd()result above was discarded. Rewritten withfilepath.Rel. All local scanner findings (secrets, authconfig, codevulns) used absolute paths wheneverrootdidn't prefix-match.🟠 HIGH
pkg/history/history.go— Path traversal inLoadScan/LoadScanResult:filepath.Join(dir, args[0])letvx history show ../../etc/passwdescape~/.vx/scans/. AddedsafePath()guard.pkg/engine/config.go— Removed dead fieldsMinScore,OutputJSON,Verboseand unusedDefaultConfig(written fromcmd/but never read anywhere in the engine).pkg/report/sarif.go— SARIFruleIDused the loop index (VX-%s-%03d), so the same finding got a different ID across scans when ordering changed (goroutines). Switched to a stable ID derived from module + CWE (or hash of module+title as fallback).pkg/report/html.go— RemovedSummary map[string]intfield: type incompatible withengine.ScoreResult.Summary(map[engine.Severity]int) and never assigned.🟡 MEDIUM
pkg/history/history.go— Scan dir / file permissions tightened:0755 → 0700,0644 → 0600. Reports may contain sensitive URLs and detected credentials.cmd/audit.go— Replaced hand-rolledresolveAbsPath(hard-coded/separator, noClean) withfilepath.Abs.cmd/audit.go,cmd/scan.go—os.OpenFile(GITHUB_OUTPUT)now usesdefer Close()instead of a manual close afterfmt.Fprintfcalls.pkg/report/markdown.go—GradeBnow returns 🟩 instead of the same 🟢 asGradeA.Out of scope (LOW, non-blocking)
Typo
ComparScans → CompareScans(exported, breaking), regex recompiled in hot paths, unusedcolorWhiteconstant, swallowed errors in non-security paths,deps.goglobalhttpClient. Can be handled in a follow-up PR.Test plan
go build ./...go vet ./...go test ./...(no test files yet)vx history show ../../etc/passwdmust now reject~/.vx/scans/perms after a scan (stat -c %a)🤖 Generated with Claude Code