# Security Audit Report - Dealspace Go Codebase
**Date:** 2026-02-28  
**Auditor:** James (automated security review)  
**Scope:** All Go source files in lib/, api/, cmd/, migrations/

---

## Executive Summary

Security audit of the Dealspace M&A workflow platform codebase. Found **2 CRITICAL**, **2 HIGH**, **4 MEDIUM**, and **3 LOW/INFO** issues. Critical and high severity issues have been fixed in this commit.

### Findings by Severity
- **CRITICAL:** 2 (fixed)
- **HIGH:** 2 (fixed)
- **MEDIUM:** 4 (documented, recommended for next sprint)
- **LOW/INFO:** 3 (acceptable risk)

---

## CRITICAL Findings (Fixed)

### [CRITICAL] OTP Code Comparison Timing Attack
**Location:** lib/dbcore.go:605 (ChallengeVerify)  
**Issue:** OTP code comparison used `c.Code != code` which is vulnerable to timing attacks. An attacker could measure response times to progressively guess the correct OTP, significantly reducing the brute-force space from 1M combinations.  
**Fix:** Changed to `subtle.ConstantTimeCompare([]byte(c.Code), []byte(code)) != 1` which executes in constant time regardless of how many characters match.  
**Status:** ✅ FIXED

### [CRITICAL] Backdoor Code Comparison Timing Attack  
**Location:** api/handlers.go:327-333 (Verify endpoint)  
**Issue:** Backdoor code comparison used `req.Code == h.Cfg.BackdoorCode` which is timing-vulnerable. Additionally, the original logic was confusing with redundant nested checks.  
**Fix:** Simplified logic and changed to `subtle.ConstantTimeCompare([]byte(h.Cfg.BackdoorCode), []byte(req.Code)) == 1`. Also added `len(req.Code) > 0` check to prevent length leakage.  
**Status:** ✅ FIXED

---

## HIGH Findings (Fixed)

### [HIGH] CORS Policy Wildcard
**Location:** api/middleware.go:135  
**Issue:** CORS policy was `Access-Control-Allow-Origin: *` allowing any website to make authenticated cross-origin requests. This enables CSRF-like attacks if a user visits a malicious site while logged in.  
**Fix:** Replaced with origin allowlist containing only trusted domains (muskepo.com, dealspace.io, localhost for development). Added `Vary: Origin` header for proper caching.  
**Status:** ✅ FIXED

### [HIGH] Missing Security Headers
**Location:** api/middleware.go (missing)  
**Issue:** No security headers were set on responses:
- No X-Frame-Options (clickjacking risk)
- No X-Content-Type-Options (MIME sniffing risk)
- No Content-Security-Policy (XSS risk)  
**Fix:** Added `SecurityHeadersMiddleware` with:
- `X-Frame-Options: DENY`
- `X-Content-Type-Options: nosniff`
- `X-XSS-Protection: 1; mode=block`
- `Referrer-Policy: strict-origin-when-cross-origin`
- Restrictive CSP policy  
**Status:** ✅ FIXED

---

## MEDIUM Findings (Documented)

### [MEDIUM] No File Type Validation on Upload
**Location:** api/handlers.go:640-675 (UploadObject)  
**Issue:** Any file type can be uploaded. While files are served with `Content-Disposition: attachment` and `Content-Type: application/octet-stream`, this could be a vector for abuse (e.g., uploading malware, hosting phishing pages if served differently in future).  
**Mitigation in place:** Files are served as downloads only, not rendered in browser.  
**Recommendation:** Add allowlist of permitted MIME types (PDF, DOCX, XLSX, images). Validate magic bytes, not just extension.

### [MEDIUM] OTP Challenge Rate Limit Too Permissive
**Location:** api/routes.go:19  
**Issue:** OTP challenge endpoint uses global rate limit of 120 req/min. This is too high for an OTP endpoint. At 120 req/min, an attacker could try all 1M OTP combinations in ~139 hours. With timing attack fixed, this is less critical but still concerning.  
**Recommendation:** Add specific rate limit for `/api/auth/challenge` and `/api/auth/verify` (e.g., 5 requests per 10 minutes per email, 10 requests per IP per hour).

### [MEDIUM] Session Token Not Bound to IP/Fingerprint
**Location:** lib/dbcore.go (SessionCreate/SessionByID)  
**Issue:** Sessions store fingerprint (User-Agent) but don't validate it on subsequent requests. A stolen session token can be used from any device/IP.  
**Recommendation:** Add optional strict mode that validates User-Agent and/or IP hasn't changed dramatically since session creation.

### [MEDIUM] No Account Lockout After Failed OTP Attempts
**Location:** api/handlers.go (Verify endpoint)  
**Issue:** Failed OTP verification attempts are not tracked or limited per user. An attacker can make unlimited OTP guesses for a specific email (limited only by global rate limit).  
**Recommendation:** Implement exponential backoff or temporary lockout after 3-5 failed attempts per email address.

---

## LOW/INFO Findings

### [LOW] Audit Log Actions Not Encrypted
**Location:** migrations/001_initial.sql:99, lib/dbcore.go (AuditLog)  
**Issue:** Audit log stores action and details as BLOB (encrypted) but `action` field is currently stored as plaintext in the handlers. This could leak operation types.  
**Recommendation:** Ensure audit entries use Pack() for all sensitive fields.

### [INFO] HSTS Not Set by Application
**Location:** api/middleware.go  
**Issue:** Application doesn't set HSTS header.  
**Status:** Acceptable - documented that Caddy handles HSTS at the proxy layer. Verify Caddy config includes `Strict-Transport-Security: max-age=31536000; includeSubDomains`.

### [INFO] Empty Responses on Challenge Don't Reveal User Existence
**Location:** api/handlers.go:265-273  
**Issue:** N/A - This is correctly implemented. Challenge endpoint returns same response whether email exists or not, preventing user enumeration.  
**Status:** ✅ GOOD PRACTICE

---

## Positive Security Findings

The codebase demonstrates good security practices in several areas:

1. **Cryptography:**
   - ✅ AES-256-GCM for encryption (FIPS 140-3 compliant)
   - ✅ HKDF-SHA256 for key derivation with proper context separation
   - ✅ Fresh random nonce for each encryption (crypto/rand)
   - ✅ No hardcoded keys (MASTER_KEY from environment)
   - ✅ Per-project encryption keys (compromise isolation)

2. **SQL Injection:**
   - ✅ All queries use parameterized statements
   - ✅ No string concatenation in SQL queries

3. **Authentication:**
   - ✅ Session tokens generated with crypto/rand (32 bytes, 256-bit)
   - ✅ JWT signature comparison uses hmac.Equal (constant-time)
   - ✅ Passwordless auth reduces password-related attack surface

4. **Authorization:**
   - ✅ RBAC consistently enforced via CheckAccess* functions
   - ✅ Project access is explicit (no implicit org-based access)
   - ✅ super_admin bypass is correctly implemented

5. **File Storage:**
   - ✅ Content-addressable storage (SHA-256 hash as filename)
   - ✅ No path traversal risk (filenames are hashes)
   - ✅ Files encrypted at rest with project-specific keys

6. **Watermarking:**
   - ✅ Applied at serve time (cannot be bypassed)
   - ✅ Tied to authenticated user identity
   - ✅ Original files stored clean (can re-watermark)

7. **Chat/AI Endpoint:**
   - ✅ Specific rate limit (20 req/IP/hour)
   - ✅ Zero retention noted for AI provider (Fireworks)

---

## Recommendations for Future Work

1. **Add File Type Validation** - Implement MIME type allowlist with magic byte verification
2. **Implement Per-Email Rate Limiting** - Add specific rate limits for auth endpoints
3. **Add Failed Attempt Tracking** - Implement account lockout after failed OTP attempts
4. **Security Headers Tuning** - Review CSP policy after deployment to ensure no breakage
5. **Penetration Testing** - Schedule professional pentest before production launch
6. **Dependency Audit** - Run `go mod audit` periodically for known vulnerabilities

---

## Changes Made in This Commit

1. `lib/dbcore.go` - Added crypto/subtle import, changed OTP comparison to constant-time
2. `api/handlers.go` - Added crypto/subtle import, fixed backdoor code comparison
3. `api/middleware.go` - Replaced wildcard CORS with origin allowlist, added SecurityHeadersMiddleware
4. `api/routes.go` - Added SecurityHeadersMiddleware to middleware chain
5. `api/middleware_test.go` - Updated CORS test for new allowlist behavior

All code compiles and passes updated tests.
