Skip to content

fix: address security review findings (1 CRITICAL, 5 HIGH, 3 MEDIUM)#1

Open
MakFly wants to merge 1 commit intomainfrom
fix/security-review
Open

fix: address security review findings (1 CRITICAL, 5 HIGH, 3 MEDIUM)#1
MakFly wants to merge 1 commit intomainfrom
fix/security-review

Conversation

@MakFly
Copy link
Copy Markdown
Owner

@MakFly MakFly commented Apr 17, 2026

Summary

Multi-agent security review surfaced one CRITICAL, five HIGH and three MEDIUM issues across cmd/, pkg/history, pkg/engine, pkg/local, and pkg/report. This PR fixes all of them.

Fixes

🔴 CRITICAL

  • pkg/local/secrets.gorelativeToRoot was broken: strings.CutPrefix returns (string, bool) but the code treated the bool as an error, and the os.Getwd() result above was discarded. Rewritten with filepath.Rel. All local scanner findings (secrets, authconfig, codevulns) used absolute paths whenever root didn't prefix-match.

🟠 HIGH

  • pkg/history/history.go — Path traversal in LoadScan / LoadScanResult: filepath.Join(dir, args[0]) let vx history show ../../etc/passwd escape ~/.vx/scans/. Added safePath() guard.
  • pkg/engine/config.go — Removed dead fields MinScore, OutputJSON, Verbose and unused DefaultConfig (written from cmd/ but never read anywhere in the engine).
  • pkg/report/sarif.go — SARIF ruleID used 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 — Removed Summary map[string]int field: type incompatible with engine.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-rolled resolveAbsPath (hard-coded / separator, no Clean) with filepath.Abs.
  • cmd/audit.go, cmd/scan.goos.OpenFile(GITHUB_OUTPUT) now uses defer Close() instead of a manual close after fmt.Fprintf calls.
  • pkg/report/markdown.goGradeB now returns 🟩 instead of the same 🟢 as GradeA.

Out of scope (LOW, non-blocking)

Typo ComparScans → CompareScans (exported, breaking), regex recompiled in hot paths, unused colorWhite constant, swallowed errors in non-security paths, deps.go global httpClient. Can be handled in a follow-up PR.

Test plan

  • go build ./...
  • go vet ./...
  • go test ./... (no test files yet)
  • Manual: vx history show ../../etc/passwd must now reject
  • Manual: SARIF output — same finding produces same ruleID across two scans
  • Manual: check ~/.vx/scans/ perms after a scan (stat -c %a)

🤖 Generated with Claude Code

- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant