Skip to content

fix(security): address critical vulnerabilities from code review#3

Merged
cschuman merged 1 commit into
mainfrom
feat/session-20260105-214342
Jan 6, 2026
Merged

fix(security): address critical vulnerabilities from code review#3
cschuman merged 1 commit into
mainfrom
feat/session-20260105-214342

Conversation

@cschuman

@cschuman cschuman commented Jan 6, 2026

Copy link
Copy Markdown
Owner

Summary

Addresses all critical security vulnerabilities and code quality issues identified in the comprehensive code review.

Security Fixes (Critical)

Path Traversal Protection (CWE-22)

  • Added validatePath() function in main.go
  • Verifies paths are within current working directory
  • Resolves symlinks to prevent TOCTOU attacks

Arbitrary File Write Protection (CWE-73)

  • Added validatePathForWrite() in fixer/fixer.go
  • Validates file is regular file (not device/socket)
  • Checks path and symlink target are within CWD

Resource Exhaustion Prevention (CWE-400)

const (
    MaxFilesPerScan   = 10000
    MaxFileSizeBytes  = 10 * 1024 * 1024 // 10MB
    MaxDirectoryDepth = 50
)

Code Quality Fixes

Context Leak False Positives

Fixed detection to only track calls to known cancel functions from the cancelFuncs map, preventing false positives when other functions with matching names exist.

Shared Helpers Extraction

Created rules/helpers.go with shared functions:

  • isHTTPHandler() - expanded to support Echo, Gin, Chi, Fiber frameworks
  • estimateStructSizes()
  • getTypeName()
  • itoa()
  • isInLoop()
  • findFunctionContaining()

Fixer Cleanup

  • Removed misleading AST modification code that set modified = true but never applied changes
  • Fixed build error from redundant newline
  • Added isValidIdentifier() for safe message extraction

Testing

Package Before After
fixer 0% 28.2%
rules 5.6% 14.8%

New Tests Added

  • Context background in handler detection
  • Context leak detection (including false positive prevention)
  • HTTP handler detection for multiple frameworks
  • Struct size estimation
  • pprof in hot path detection
  • Path validation security tests
  • Identifier validation tests

All 50 tests pass.

Test Plan

  • go build -o goperf . succeeds
  • go test ./... passes (50 tests)
  • Security validation rejects path traversal attempts
  • Context leak detection no longer has false positives

Security fixes:
- Add path traversal protection in collectGoFiles (CWE-22)
- Add arbitrary file write protection in fixer (CWE-73)
- Add symlink TOCTOU race condition prevention (CWE-61)
- Add resource limits to prevent DoS (MaxFiles, MaxSize, MaxDepth)

Code quality fixes:
- Fix context leak false positives (only track known cancel functions)
- Extract shared helpers to rules/helpers.go (isHTTPHandler, etc.)
- Remove misleading AST modification code in fixer
- Fix fixer build error (redundant newline in fmt.Println)

Testing improvements:
- Add 50 comprehensive tests for detection rules
- Add security validation tests for fixer
- Test coverage: fixer 28.2%, rules 14.8%

Key tests added:
- Context leak detection with false positive prevention
- HTTP handler detection for multiple frameworks
- Path traversal/symlink security validation
- Identifier validation for safe extraction
@cschuman cschuman merged commit 42b3b36 into main Jan 6, 2026
5 of 6 checks passed
@cschuman cschuman deleted the feat/session-20260105-214342 branch January 6, 2026 02:57
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