diff --git a/.sdlc/features/user-preferences/audit.md b/.sdlc/features/user-preferences/audit.md new file mode 100644 index 0000000..9ecc795 --- /dev/null +++ b/.sdlc/features/user-preferences/audit.md @@ -0,0 +1,111 @@ +# Security Audit: User Preferences API + +## Summary + +**Overall Assessment: PASS** + +The User Preferences API feature demonstrates solid security practices. No critical or high severity findings were identified. The implementation correctly enforces authentication via JWT middleware, performs authorization checks (own-user or admin), validates all inputs at appropriate layers, and avoids common vulnerability patterns. A few medium/low observations are noted for future hardening. + +## Static Analysis Results + +- **`go vet ./...`**: Clean — no warnings or errors +- **All tests passing**: 30/30 tests pass across domain, adapter, service, and handler layers +- **No `golangci-lint` available** in the environment; `go vet` was the sole static analyzer run + +## OWASP Assessment + +| Category | Status | Notes | +|----------|--------|-------| +| **A01: Broken Access Control** | PASS | Authorization enforced in handler layer via `authorizeAccess()`. Users can only access own preferences unless they have `admin` role. Tested with forbidden and admin-access test cases. | +| **A02: Cryptographic Failures** | PASS | No sensitive data stored. Preferences contain no PII beyond user_id. JWT secret sourced from environment variable, not hardcoded. | +| **A03: Injection** | PASS | No SQL queries, command execution, or template rendering. All data is in-memory Go maps. JSON input is parsed via standard `encoding/json` with manual key allowlisting. | +| **A04: Insecure Design** | PASS | Hexagonal architecture cleanly separates concerns. Domain validation prevents invalid state. Merge semantics use pointer fields to distinguish present vs absent values. | +| **A05: Security Misconfiguration** | PASS (with note) | Auth is configurable via `AUTH_ENABLED` env var. When disabled, the `authorizeAccess()` function allows all access (returns nil when no auth user in context). This is the intended local-dev behavior but warrants documentation. No debug modes exposed. | +| **A06: Vulnerable Components** | PASS | Uses standard Go stdlib `encoding/json`, `regexp`, `sync`. External deps: `chi/v5` (router), `google/uuid` (UUID parsing) — both well-maintained. | +| **A07: Auth Failures** | PASS | JWT middleware from `pkg/auth` handles token extraction and validation. Unauthenticated requests are blocked before reaching handler code (when auth is enabled). | +| **A08: Software/Data Integrity** | PASS | No deserialization of untrusted types. JSON decoding targets known structs with explicit field tags. Unknown fields in `preferences` are rejected via manual key allowlisting. | +| **A09: Logging & Monitoring Gaps** | PASS (with note) | Service layer logs successful upserts with user_id. However, failed authorization attempts are not explicitly logged (they return httperror.Forbidden which is handled by the framework). Failed validation is also not logged at the service level. | +| **A10: SSRF** | PASS | No outbound HTTP calls, no user-controlled URLs, no network access from this service. | + +## Critical Findings + +None. + +## High Findings + +None. + +## Medium Findings + +### M1: No Request Body Size Limit + +**Severity:** Medium +**Location:** `internal/api/handlers/preferences.go:117-123` (Update handler) + +The `httpresponse.DecodeJSON()` call does not enforce a maximum request body size. Neither the handler nor the shared `pkg/httpresponse` package uses `http.MaxBytesReader`. An attacker could send an arbitrarily large JSON payload to exhaust server memory. + +**Risk:** Denial-of-service via oversized request bodies. + +**Remediation:** Apply `http.MaxBytesReader(w, r.Body, maxBytes)` before decoding, either in the handler or as framework middleware. A reasonable limit for preferences would be 64KB. + +**Note:** This is a framework-level concern shared across all services, not specific to this feature. + +### M2: Auth Bypass When AUTH_ENABLED=false + +**Severity:** Medium +**Location:** `internal/api/routes.go:29`, `internal/api/handlers/preferences.go:183-192` + +When `AUTH_ENABLED` is `false` (default), the auth middleware is not applied. The `authorizeAccess()` function handles this by checking `if user == nil { return nil }` — allowing all requests through without any authorization check. This means any caller can read/write any user's preferences. + +**Risk:** In a deployment where auth is accidentally left disabled, all preferences become world-readable/writable. + +**Remediation:** This is the documented design for local development, but production deployments should enforce `AUTH_ENABLED=true` via deployment configuration or health-check validation. Consider logging a warning at startup when auth is disabled. + +## Low Findings + +### L1: Limited Audit Logging + +**Severity:** Low +**Location:** `internal/service/preferences.go:57` + +Only successful upserts are logged. Failed authorization, validation failures, and read operations are not logged at the application level (though framework middleware may capture HTTP-level access logs). + +**Remediation:** Add structured logging for authorization denials and validation failures at the handler layer for operational visibility. + +### L2: Time Format Precision + +**Severity:** Low +**Location:** `internal/api/handlers/preferences.go:82` + +The `UpdatedAt` field is formatted with `"2006-01-02T15:04:05Z"` which drops sub-second precision. This is cosmetic but means two rapid updates in the same second would appear to have the same timestamp. + +**Remediation:** Consider using `time.RFC3339Nano` or at least millisecond precision if needed for conflict detection in future. + +## Recommendations + +1. **Add request body size limiting** — Apply `http.MaxBytesReader` at the framework level or per-handler (Medium priority) +2. **Log a warning when auth is disabled** — Make it obvious in startup logs that the service is running without authentication (Medium priority) +3. **Add audit logging for authz failures** — Log when authorization checks reject a request, including the requesting user ID and target user ID (Low priority) +4. **Document AUTH_ENABLED behavior** — Ensure deployment runbooks require `AUTH_ENABLED=true` in production (Low priority) + +## Files Reviewed + +| File | Lines | Reviewed | +|------|-------|----------| +| `internal/domain/preferences.go` | 113 | Yes | +| `internal/domain/errors.go` | 11 | Yes | +| `internal/domain/preferences_test.go` | 211 | Yes | +| `internal/port/preferences.go` | 17 | Yes | +| `internal/adapter/memory/preferences.go` | 50 | Yes | +| `internal/adapter/memory/preferences_test.go` | 73 | Yes | +| `internal/service/preferences.go` | 59 | Yes | +| `internal/service/preferences_test.go` | 153 | Yes | +| `internal/api/handlers/preferences.go` | 210 | Yes | +| `internal/api/handlers/preferences_test.go` | 303 | Yes | +| `internal/api/routes.go` | 42 | Yes | +| `internal/api/spec.go` | 89 | Yes | +| `internal/config/config.go` | 34 | Yes | +| `cmd/server/main.go` | 39 | Yes | +| `pkg/auth/middleware.go` | 234 | Yes (shared) | +| `pkg/auth/auth.go` | 92 | Yes (shared) | +| `pkg/httpresponse/response.go` | 193 | Yes (shared) | diff --git a/.sdlc/features/user-preferences/manifest.yaml b/.sdlc/features/user-preferences/manifest.yaml index 513fe28..8b1f659 100644 --- a/.sdlc/features/user-preferences/manifest.yaml +++ b/.sdlc/features/user-preferences/manifest.yaml @@ -19,7 +19,7 @@ phase_history: entered: 2026-02-08T01:55:43.600701486Z artifacts: audit: - status: pending + status: passed path: audit.md design: status: approved diff --git a/.sdlc/state.yaml b/.sdlc/state.yaml index f74be19..608b68d 100644 --- a/.sdlc/state.yaml +++ b/.sdlc/state.yaml @@ -6,8 +6,8 @@ active_work: - slug: user-preferences phase: implementation blocked: [] -last_updated: 2026-02-08T02:08:48.467658449Z -last_action: NEEDS_FIX_ARTIFACT +last_updated: 2026-02-08T02:11:06.156803496Z +last_action: PASS_ARTIFACT last_actor: user history: - timestamp: 2026-02-08T01:41:06.381900491Z @@ -100,3 +100,8 @@ history: feature: user-preferences actor: user result: success + - timestamp: 2026-02-08T02:11:06.156801893Z + action: PASS_ARTIFACT + feature: user-preferences + actor: user + result: success