Skip to content

Audit codebase for security vulnerabilities#2

Merged
vedssharma merged 4 commits intomainfrom
claude/security-audit-faJm9
Jan 21, 2026
Merged

Audit codebase for security vulnerabilities#2
vedssharma merged 4 commits intomainfrom
claude/security-audit-faJm9

Conversation

@vedssharma
Copy link
Copy Markdown
Owner

No description provided.

- Add response body size limit (50MB) to prevent memory exhaustion DoS attacks
- Add SSRF protection with URL validation:
  - Block requests to cloud metadata endpoints (AWS/GCP/Azure)
  - Warn on requests to localhost/loopback addresses
  - Warn on requests to private IP ranges (10.x, 192.168.x, 172.16-31.x)
  - Validate URL scheme (only http/https allowed)
- Expand sensitive headers list to include AWS, GCP, and Azure credential headers
- Add warning when request body contains potentially sensitive data (passwords, tokens, etc.)
- Sanitize terminal output to prevent ANSI escape sequence injection attacks
- Fix ignored JSON unmarshal errors in SQLite storage with proper error handling
- Run Docker container as non-root user (appuser) for defense in depth
- Fix TOCTOU race condition in database file permissions by pre-creating
  the file with secure permissions before SQLite opens it
- Filter sensitive headers when adding requests to collections
@vedssharma vedssharma merged commit f5fcef7 into main Jan 21, 2026
1 check passed
@claude
Copy link
Copy Markdown

claude Bot commented Jan 21, 2026

Security Audit PR Review

This PR implements several important security improvements to the codebase. Here is my detailed review:

✅ Strengths

1. Docker Security Improvements (Dockerfile)

  • Good: Running as non-root user (appuser)
  • Good: Proper ownership and permissions for application directory
  • This prevents privilege escalation attacks in containerized environments

2. Output Sanitization (internal/format/output.go:17-40)

  • Excellent: sanitizeOutput() function prevents terminal injection attacks
  • Good: Escapes ANSI escape sequences and control characters
  • Good: Comprehensive application across all output functions

3. Sensitive Data Filtering (cmd/request.go:19-53, cmd/collection.go:153-154)

  • Excellent: Expanded list of sensitive headers (AWS, GCP, Azure credentials)
  • Good: Warning users about sensitive data in request bodies
  • Good: Filtering headers before storing in collections

4. SSRF Protection (internal/http/client.go:134-206)

  • Good: URL validation and scheme restriction
  • Excellent: Blocking cloud metadata endpoints (prevents SSRF attacks)
  • Good: Warnings for localhost and private IP ranges

5. File Security (internal/storage/sqlite.go:40-64)

  • Excellent: ensureSecureFile() prevents TOCTOU race conditions
  • Good: Secure file permissions (0600) for database

6. Resource Limits (internal/http/client.go:14-19, 78-88)

  • Good: 50MB response size limit prevents memory exhaustion
  • Good: 30-second timeout prevents hanging requests

@claude
Copy link
Copy Markdown

claude Bot commented Jan 21, 2026

⚠️ Issues & Suggestions

1. CRITICAL: SSRF Protection Too Weak

Location: internal/http/client.go:134-206

Issues:

  • The isPrivateOrReservedHost() function only checks string prefixes, which is insufficient and can be bypassed
  • Missing validation for: IPv6 private ranges, octal/hex IP encodings, DNS rebinding attacks
  • Missing check for CIDR ranges like 172.16.0.0/12

Example bypasses:

Recommended fix: Use net.ParseIP() and net.ParseCIDR() to properly validate against comprehensive private/reserved IP ranges including IPv6.

2. Missing: SQL Injection Prevention Documentation

Location: internal/storage/sqlite.go

Issue: While the current code uses prepared statements (good!), there is no explicit documentation confirming this protection.

Suggestion: Add comments confirming SQL injection protection via parameterized statements.

3. Error Handling: Silent JSON Parse Failures

Location: internal/storage/sqlite.go:15-18, 211, 322, 421

Issue: parseJSONHeaders() returns errors but they are ignored with underscore

  • Corrupted data in database will be silently lost
  • Could mask data integrity issues

Suggestion: Log errors to stderr for debugging while continuing with empty map.

@claude
Copy link
Copy Markdown

claude Bot commented Jan 21, 2026

4. Incomplete: Body Sensitivity Detection

Location: cmd/request.go:361-376

Issues:

  • Pattern matching on lowercase field names can miss camelCase/PascalCase (e.g., accessToken, ApiKey)
  • No detection of sensitive data in JSON values, only keys
  • Base64-encoded credentials will not be detected

Suggestions:

  • Add case-insensitive JSON key checking
  • Consider checking for common patterns like JWT tokens (eyJ...), Base64 strings in sensitive fields

5. Minor: Response Truncation UX

Location: internal/http/client.go:85-88

Issue: Warning goes to stderr but user does not know how much was truncated or how to increase limit

Suggestion: Improve warning message clarity about truncation size and limits.

6. go.sum Added

Location: Root directory

Note: This is good - the go.sum file should be committed. However, verify that:

  • All checksums are correct (run go mod verify)
  • No unexpected dependencies were added

@claude
Copy link
Copy Markdown

claude Bot commented Jan 21, 2026

🧪 Test Coverage

CRITICAL MISSING: No test files found in the repository!

Required tests for security features:

  1. internal/http/client_test.go:

    • Test SSRF protection with various bypass attempts
    • Test response size limits
    • Test URL validation edge cases
  2. internal/format/output_test.go:

    • Test sanitization with malicious control characters
    • Test ANSI escape sequence handling
  3. internal/storage/sqlite_test.go:

    • Test file permission enforcement
    • Test JSON parsing error handling
  4. cmd/request_test.go:

    • Test sensitive header filtering
    • Test body sensitivity detection

🔒 Security Checklist

  • ✅ Terminal injection prevention
  • ✅ Non-root Docker user
  • ✅ Sensitive data filtering
  • ⚠️ SSRF protection (needs strengthening)
  • ✅ File permission security
  • ✅ Resource limits (memory, timeout)
  • ❌ No security tests
  • ⚠️ Error handling could be improved

@claude
Copy link
Copy Markdown

claude Bot commented Jan 21, 2026

📋 Recommendations

High Priority:

  1. Fix SSRF validation using proper IP parsing and CIDR ranges
  2. Add comprehensive security tests for all new security features
  3. Improve error handling for JSON parsing failures

Medium Priority:

  1. Enhance body sensitivity detection for JSON values and encodings
  2. Add logging/monitoring for security events (blocked SSRF attempts, etc.)
  3. Consider adding rate limiting for requests

Low Priority:

  1. Document security features in README
  2. Add security policy (SECURITY.md) for vulnerability reporting

🎯 Overall Assessment

Verdict: Good foundation, but needs critical fixes before merge.

This PR demonstrates strong security awareness and addresses many important concerns. The sanitization, Docker hardening, and credential filtering are excellent additions. However, the SSRF protection has implementation gaps that could be exploited, and the complete lack of tests is concerning for security-critical code.

Recommendation: Request changes - specifically:

  1. Fix SSRF validation using proper IP parsing
  2. Add minimum test coverage for security features
  3. Improve error handling for data parsing

The effort here is commendable and this moves the project in the right direction. With these fixes, it will be ready to merge.


Review generated using Claude Code

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.

2 participants