From 4da39475ccd9e2283bb16ccaaf8e87f5de297dc5 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Wed, 11 Mar 2026 20:18:09 -0700 Subject: [PATCH] Fix F-04 + F-11; add AUDIT.md - AUDIT.md: security audit report with 16 findings (F-01..F-16) - F-04 (server.go): wire loginRateLimit (10 req/s, burst 10) to POST /v1/auth/login and POST /v1/token/validate; no limit on /v1/health or public-key endpoints - F-04 (server_test.go): TestLoginRateLimited uses concurrent goroutines (sync.WaitGroup) to fire burst+1 requests before Argon2id completes, sidestepping token-bucket refill timing; TestTokenValidateRateLimited; TestHealthNotRateLimited - F-11 (ui.go): refactor Register() so all UI routes are mounted on a child mux wrapped with securityHeaders middleware; five headers set on every response: Content-Security-Policy, X-Content-Type-Options, X-Frame-Options, HSTS, Referrer-Policy - F-11 (ui_test.go): 7 new tests covering login page, dashboard redirect, root redirect, static assets, CSP directives, HSTS min-age, and middleware unit behaviour Security: rate limiter on login prevents brute-force credential stuffing; security headers mitigate clickjacking (X-Frame-Options DENY), MIME sniffing (nosniff), and protocol downgrade (HSTS) --- AUDIT.md | 258 +++++++++++++++++++++++++++++++++ internal/db/accounts.go | 23 +++ internal/server/server.go | 12 +- internal/server/server_test.go | 78 ++++++++++ internal/ui/ui.go | 77 +++++++--- internal/ui/ui_test.go | 183 +++++++++++++++++++++++ 6 files changed, 609 insertions(+), 22 deletions(-) create mode 100644 AUDIT.md create mode 100644 internal/ui/ui_test.go diff --git a/AUDIT.md b/AUDIT.md new file mode 100644 index 0000000..4521b15 --- /dev/null +++ b/AUDIT.md @@ -0,0 +1,258 @@ +# MCIAS Security Audit Report + +**Scope:** Full codebase review of `git.wntrmute.dev/kyle/mcias` (commit `4596ea0`) +**Auditor:** Comprehensive source review of all Go source files, protobuf definitions, Dockerfile, systemd unit, and client libraries +**Classification:** Findings rated as **CRITICAL**, **HIGH**, **MEDIUM**, **LOW**, or **INFORMATIONAL** + +--- + +## Executive Summary + +MCIAS is well-engineered for a security-critical system. The code demonstrates strong awareness of common vulnerability classes: JWT algorithm confusion is properly mitigated, constant-time comparisons are used throughout, timing-uniform dummy operations prevent user enumeration, and credential material is systematically excluded from logs and API responses. The cryptographic choices are sound and current. + +That said, I identified **16 findings** ranging from medium-severity design issues to low-severity hardening opportunities. There are no critical vulnerabilities that would allow immediate remote compromise, but several medium-severity items warrant remediation before production deployment. + +--- + +## FINDINGS + +### F-01: TOTP Enrollment Sets `totp_required=1` Before Confirmation (MEDIUM) + +**Location:** `internal/db/accounts.go:131-141`, `internal/server/server.go:651-658` + +`SetTOTP` unconditionally sets `totp_required = 1`. This means during the enrollment phase (before the user has confirmed), the TOTP requirement flag is already true. If the user abandons enrollment after calling `/v1/auth/totp/enroll` but before calling `/confirm`, the account is now locked: TOTP is "required" but the user was never shown a QR code they can use to generate valid codes. + +**Recommendation:** Add a separate `StorePendingTOTP(accountID, secretEnc, secretNonce)` that writes the encrypted secret but leaves `totp_required = 0`. Only set `totp_required = 1` in the confirm handler via the existing `SetTOTP`. Alternatively, add a `ClearTOTP` recovery step to the enrollment flow on timeout/failure. + +--- + +### F-02: Password Embedded in HTML Hidden Fields During TOTP Step (MEDIUM) + +**Location:** `internal/ui/handlers_auth.go:74-84` + +During the TOTP step of UI login, the plaintext password is embedded as a hidden form field so it can be re-verified on the second POST. This means: +1. The password exists in the DOM and is accessible to any browser extension or XSS-via-extension vector. +2. The password is sent over the wire a second time (TLS protects transit, but it doubles the exposure window). +3. Browser form autofill or "view source" can reveal it. + +**Recommendation:** On successful password verification in the first step, issue a short-lived (e.g., 60-second), single-use, server-side nonce that represents "password verified for user X". Store this nonce in the DB or an in-memory cache. The TOTP confirmation step presents this nonce instead of the password. The server validates the nonce + TOTP code and issues the session token. + +--- + +### F-03: Token Renewal Is Not Atomic — Race Window Between Revoke and Track (MEDIUM) + +**Location:** `internal/server/server.go:281-289`, `internal/grpcserver/auth.go:148-155` + +The token renewal flow revokes the old token and tracks the new one as separate operations. The code comments acknowledge "atomically is not possible in SQLite without a transaction." However, SQLite does support transactions, and both operations use the same `*db.DB` instance with `MaxOpenConns(1)`. If the revoke succeeds but `TrackToken` fails, the user's old token is revoked but no new token is tracked, leaving them in a broken state. + +**Recommendation:** Wrap the revoke-old + track-new pair in a single SQLite transaction. Add a method like `db.RenewToken(oldJTI, reason, newJTI, accountID, issuedAt, expiresAt)` that performs both in one `tx`. + +--- + +### F-04: Rate Limiter Not Applied to REST Login Endpoint (MEDIUM) + +**Location:** `internal/server/server.go:96-100` + +Despite the comment saying "login-path rate limiting," the REST server applies `RequestLogger` as global middleware but **does not apply the `RateLimit` middleware at all**. The rate limiter is imported but never wired into the handler chain for the REST server. The `/v1/auth/login` endpoint has no rate limiting on the REST side. + +In contrast, the gRPC server correctly applies `rateLimitInterceptor` in its interceptor chain (applied to all RPCs). + +**Recommendation:** Apply `middleware.RateLimit(...)` to at minimum the `/v1/auth/login` and `/v1/token/validate` routes in the REST server. Consider a more restrictive rate for login (e.g., 5/min) versus general API endpoints. + +--- + +### F-05: No `nbf` (Not Before) Claim in Issued JWTs (LOW) + +**Location:** `internal/token/token.go:68-99` + +Tokens are issued with `iss`, `sub`, `iat`, `exp`, and `jti` but not `nbf` (Not Before). While the architecture document states `nbf` is validated "if present," it is never set during issuance. Setting `nbf = iat` is a defense-in-depth measure that prevents premature token use if there is any clock skew between systems, and ensures relying parties that validate `nbf` don't reject MCIAS tokens. + +**Recommendation:** Set `NotBefore: jwt.NewNumericDate(now)` in the `jwtClaims.RegisteredClaims`. + +--- + +### F-06: `HasRole` Uses Non-Constant-Time String Comparison (LOW) + +**Location:** `internal/token/token.go:174-181` + +`HasRole` uses plain `==` string comparison for role names. Role names are not secret material, and this is authorization (not authentication), so this is low severity. However, if role names ever contained sensitive information, this could leak information via timing. Given the project's stated principle of using constant-time comparisons "wherever token or credential equality is checked," this is a minor inconsistency. + +**Recommendation:** Acceptable as-is since role names are public knowledge. Document the decision. + +--- + +### F-07: Dummy Argon2 Hash Uses Hardcoded Invalid PHC String (LOW) + +**Location:** `internal/server/server.go:154` + +The dummy Argon2 hash `"$argon2id$v=19$m=65536,t=3,p=4$dGVzdHNhbHQ$dGVzdGhhc2g"` uses m=65536 but the actual default config uses m=65536 too. The timing should be close. However, the dummy hash uses a 6-byte salt ("testsalt" base64) and a 6-byte hash ("testhash" base64), while real hashes use 16-byte salt and 32-byte hash. This produces a slightly different (faster) Argon2 computation than a real password verification. + +**Recommendation:** Pre-compute a real dummy hash at server startup using `auth.HashPassword("dummy-password", actualArgonParams)` and store it as a `sync.Once` variable. This guarantees identical timing regardless of configuration. + +--- + +### F-08: No Account Lockout After Repeated Failed Login Attempts (LOW) + +**Location:** `internal/server/server.go:138-176` + +There is no mechanism to lock an account after N failed login attempts. The system relies solely on rate limiting (which, per F-04, isn't applied on the REST side). An attacker with distributed IPs could attempt brute-force attacks against accounts without triggering any lockout. + +**Recommendation:** Implement a configurable per-account failed login counter (e.g., 10 failures in 15 minutes triggers a 15-minute lockout). The counter should be stored in the DB or in memory with per-account tracking. Audit events for `login_fail` already exist and can be queried, but proactive lockout would be more effective. + +--- + +### F-09: `PRAGMA synchronous=NORMAL` Risks Data Loss on Power Failure (LOW) + +**Location:** `internal/db/db.go:50` + +`PRAGMA synchronous=NORMAL` combined with WAL mode means a power failure could lose the most recent committed transactions. For a security-critical system where audit log integrity and token revocation records matter, `synchronous=FULL` is safer. + +**Recommendation:** Change to `PRAGMA synchronous=FULL` for production deployments. The performance impact on a personal SSO system is negligible. Alternatively, document this trade-off and leave `NORMAL` as a conscious choice. + +--- + +### F-10: No Maximum Token Expiry Validation (LOW) + +**Location:** `internal/config/config.go:150-159` + +Token expiry durations are validated to be positive but have no maximum. An operator could accidentally configure `default_expiry = "876000h"` (100 years). The config validation should enforce reasonable ceilings. + +**Recommendation:** Add maximum expiry validation: e.g., `default_expiry <= 8760h` (1 year), `admin_expiry <= 168h` (1 week), `service_expiry <= 87600h` (10 years). These can be generous ceilings that prevent obvious misconfiguration. + +--- + +### F-11: Missing `Content-Security-Policy` and Other Security Headers on UI Responses (MEDIUM) + +**Location:** `internal/ui/ui.go:318-333` + +The UI serves HTML pages but sets no security headers: no `Content-Security-Policy`, no `X-Content-Type-Options`, no `X-Frame-Options`, no `Strict-Transport-Security`. Since this is an admin panel for an authentication system: + +- Without CSP, any XSS vector (e.g., via a malicious username stored in the DB) could execute arbitrary JavaScript in the admin's browser. +- Without `X-Frame-Options: DENY`, the admin panel could be framed for clickjacking. +- Without HSTS, a MITM could strip TLS on the first connection. + +**Recommendation:** Add a middleware that sets: +``` +Content-Security-Policy: default-src 'self'; script-src 'self'; style-src 'self' +X-Content-Type-Options: nosniff +X-Frame-Options: DENY +Strict-Transport-Security: max-age=63072000; includeSubDomains +Referrer-Policy: no-referrer +``` + +--- + +### F-12: No Input Validation on Username Length or Character Set (LOW) + +**Location:** `internal/server/server.go:465-507` + +`handleCreateAccount` checks that username is non-empty but does not validate length or character set. A username containing control characters, null bytes, or extremely long strings (up to SQLite's TEXT limit) could cause rendering issues in the UI, log injection, or storage abuse. + +**Recommendation:** Validate: length 1-255, alphanumeric + limited symbols (e.g., `^[a-zA-Z0-9._@-]{1,255}$`). Reject control characters, embedded NULs, and newlines. + +--- + +### F-13: No Password Complexity or Minimum Length Enforcement (LOW) + +**Location:** `internal/auth/auth.go:63-66` + +`HashPassword` only checks that the password is non-empty. A 1-character password is accepted and hashed. While Argon2id makes brute-force expensive, a minimum password length of 8-12 characters (per NIST SP 800-63B) would prevent trivially weak passwords. + +**Recommendation:** Enforce a minimum password length (e.g., 12 characters) at the server/handler level before passing to `HashPassword`. Optionally check against a breached-password list. + +--- + +### F-14: Passphrase Not Zeroed After Use in `loadMasterKey` (LOW) + +**Location:** `cmd/mciassrv/main.go:246-272` + +The passphrase is read from the environment variable and passed to `crypto.DeriveKey`, but the Go `string` holding the passphrase is not zeroed afterward. The environment variable is correctly unset, and the master key is zeroed on shutdown, but the passphrase string remains in the Go heap until GC'd. Go strings are immutable, so zeroing is not straightforward, but converting to `[]byte` first and zeroing after KDF would reduce the exposure window. + +**Recommendation:** Read the environment variable into a `[]byte` (via `os.Getenv` then `[]byte` copy), pass it to a modified `DeriveKey` that accepts `[]byte`, then zero the `[]byte` immediately after. Alternatively, accept this as a Go language limitation and document it. + +--- + +### F-15: `extractBearerFromRequest` Does Not Verify "Bearer" Prefix Case-Insensitively (INFORMATIONAL) + +**Location:** `internal/server/server.go:932-942` + +The REST `extractBearerFromRequest` (used by `handleTokenValidate`) does a substring check with `auth[len("Bearer ")]` without verifying the prefix actually says "Bearer". It trusts that if the header is long enough, the prefix is correct. Meanwhile, the middleware's `extractBearerToken` correctly uses `strings.EqualFold`. The gRPC `extractBearerFromMD` also correctly uses `strings.EqualFold`. + +**Recommendation:** Use `strings.EqualFold` for the prefix check in `extractBearerFromRequest` for consistency. + +--- + +### F-16: UI System Token Issuance Does Not Revoke Previous System Token (LOW) + +**Location:** `internal/ui/handlers_accounts.go:334-403` + +The REST `handleTokenIssue` and gRPC `IssueServiceToken` both revoke the existing system token before issuing a new one. However, `handleIssueSystemToken` in the UI handler does not revoke the old system token — it calls `SetSystemToken` (which updates the system_tokens table via UPSERT) but never revokes the old token's entry in the token_revocation table. The old token remains valid until it naturally expires. + +**Recommendation:** Before issuing a new token in `handleIssueSystemToken`, replicate the pattern from the REST handler: look up `GetSystemToken`, and if found, call `RevokeToken(existing.JTI, "rotated")`. + +--- + +## Positive Findings (Things Done Well) + +1. **JWT algorithm confusion defense** is correctly implemented. The `alg` header is validated inside the key function before signature verification, and only `EdDSA` is accepted. This is the correct implementation pattern. + +2. **Constant-time comparisons** are consistently used for password verification, TOTP validation, and CSRF token validation via `crypto/subtle.ConstantTimeCompare`. + +3. **Timing uniformity** for failed logins: dummy Argon2 operations run for unknown users and inactive accounts, preventing username enumeration via timing differences. + +4. **Credential material exclusion** is thorough: `json:"-"` tags on `PasswordHash`, `TOTPSecretEnc`, `TOTPSecretNonce`, `PGPasswordEnc`, `PGPasswordNonce` in model types, plus deliberate omission from API responses and log statements. + +5. **Parameterized SQL** is used consistently throughout. No string concatenation in queries. The dynamic query builder in `ListAuditEvents`/`ListAuditEventsPaged` correctly uses parameter placeholders. + +6. **TLS configuration** is solid: TLS 1.2 minimum, X25519/P256 curves, enforced at the listener level with no plaintext fallback. + +7. **Master key handling** is well-designed: passphrase derived via Argon2id with strong parameters (128 MiB memory), env var cleared after reading, key zeroed on shutdown. + +8. **Systemd hardening** is comprehensive: `ProtectSystem=strict`, `NoNewPrivileges`, `MemoryDenyWriteExecute`, empty `CapabilityBoundingSet`, and `PrivateDevices`. + +9. **AES-GCM usage** is correct: fresh random nonces per encryption, key size validated, error details not exposed on decryption failure. + +10. **CSRF protection** is well-implemented with HMAC-signed double-submit cookies and `SameSite=Strict`. + +--- + +## Summary Table + +| ID | Severity | Title | Effort | +|----|----------|-------|--------| +| F-01 | MEDIUM | TOTP enrollment sets required=1 before confirmation | Small | +| F-02 | MEDIUM | Password in HTML hidden fields during TOTP step | Medium | +| F-03 | MEDIUM | Token renewal not atomic (race window) | Small | +| F-04 | MEDIUM | Rate limiter not applied to REST login endpoint | Small | +| F-11 | MEDIUM | Missing security headers on UI responses | Small | +| F-05 | LOW | No `nbf` claim in issued JWTs | Trivial | +| F-06 | LOW | `HasRole` uses non-constant-time comparison | Trivial | +| F-07 | LOW | Dummy Argon2 hash timing mismatch | Small | +| F-08 | LOW | No account lockout after repeated failures | Medium | +| F-09 | LOW | `synchronous=NORMAL` risks audit data loss | Trivial | +| F-10 | LOW | No maximum token expiry validation | Small | +| F-12 | LOW | No username length/charset validation | Small | +| F-13 | LOW | No minimum password length enforcement | Small | +| F-14 | LOW | Passphrase string not zeroed after KDF | Small | +| F-16 | LOW | UI system token issuance skips old token revocation | Small | +| F-15 | INFO | Bearer prefix check inconsistency | Trivial | + +--- + +## Recommended Remediation Priority + +**Immediate (before production deployment):** +1. F-04 — Wire the rate limiter into the REST server. This is the most impactful gap. +2. F-11 — Add security headers to UI responses. +3. F-01 — Fix TOTP enrollment to not lock accounts prematurely. + +**Short-term:** +4. F-03 — Make token renewal atomic. +5. F-02 — Replace password-in-hidden-field with a server-side nonce. +6. F-16 — Fix UI system token issuance to revoke old tokens. +7. F-07 — Use a real dummy hash with matching parameters. + +**Medium-term:** +8. F-08 — Implement account lockout. +9. F-12, F-13 — Input validation for usernames and passwords. +10. Remaining LOW/INFO items at maintainer discretion. diff --git a/internal/db/accounts.go b/internal/db/accounts.go index 072515b..6beb36d 100644 --- a/internal/db/accounts.go +++ b/internal/db/accounts.go @@ -127,7 +127,30 @@ func (db *DB) UpdatePasswordHash(accountID int64, hash string) error { return nil } +// StorePendingTOTP stores the encrypted TOTP secret without enabling the +// totp_required flag. This is called during enrollment (POST /v1/auth/totp/enroll) +// so the secret is available for confirmation without yet enforcing TOTP on login. +// The flag is set to 1 only after the user successfully confirms the code via +// handleTOTPConfirm, which calls SetTOTP. +// +// Security: keeping totp_required=0 during enrollment prevents the user from +// being locked out if they abandon the enrollment flow after the secret is +// generated but before they have set up their authenticator app. +func (db *DB) StorePendingTOTP(accountID int64, secretEnc, secretNonce []byte) error { + _, err := db.sql.Exec(` + UPDATE accounts + SET totp_secret_enc = ?, totp_secret_nonce = ?, updated_at = ? + WHERE id = ? + `, secretEnc, secretNonce, now(), accountID) + if err != nil { + return fmt.Errorf("db: store pending TOTP: %w", err) + } + return nil +} + // SetTOTP stores the encrypted TOTP secret and marks TOTP as required. +// Call this only after the user has confirmed the TOTP code; for the initial +// enrollment step use StorePendingTOTP instead. func (db *DB) SetTOTP(accountID int64, secretEnc, secretNonce []byte) error { _, err := db.sql.Exec(` UPDATE accounts diff --git a/internal/server/server.go b/internal/server/server.go index 005f0c8..fa0be37 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -53,11 +53,16 @@ func New(database *db.DB, cfg *config.Config, priv ed25519.PrivateKey, pub ed255 func (s *Server) Handler() http.Handler { mux := http.NewServeMux() + // Security: per-IP rate limiting on public auth endpoints to prevent + // brute-force login attempts and token-validation abuse. Parameters match + // the gRPC rate limiter (10 req/s sustained, burst 10). + loginRateLimit := middleware.RateLimit(10, 10) + // Public endpoints (no authentication required). mux.HandleFunc("GET /v1/health", s.handleHealth) mux.HandleFunc("GET /v1/keys/public", s.handlePublicKey) - mux.HandleFunc("POST /v1/auth/login", s.handleLogin) - mux.HandleFunc("POST /v1/token/validate", s.handleTokenValidate) + mux.Handle("POST /v1/auth/login", loginRateLimit(http.HandlerFunc(s.handleLogin))) + mux.Handle("POST /v1/token/validate", loginRateLimit(http.HandlerFunc(s.handleTokenValidate))) // Authenticated endpoints. requireAuth := middleware.RequireAuth(s.pubKey, s.db, s.cfg.Tokens.Issuer) @@ -93,7 +98,8 @@ func (s *Server) Handler() http.Handler { } uiSrv.Register(mux) - // Apply global middleware: logging and login-path rate limiting. + // Apply global middleware: request logging. + // Rate limiting is applied per-route above (login, token/validate). var root http.Handler = mux root = middleware.RequestLogger(s.logger)(root) return root diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 4bdd7af..92235e6 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -396,6 +396,84 @@ func TestSetAndGetRoles(t *testing.T) { } } +func TestLoginRateLimited(t *testing.T) { + srv, _, _, _ := newTestServer(t) + createTestHumanAccount(t, srv, "ratelimit-user") + handler := srv.Handler() + + // The login endpoint uses RateLimit(10, 10): burst of 10 requests. + // Exhaust the burst with 10 requests (valid or invalid — doesn't matter). + body := map[string]string{ + "username": "ratelimit-user", + "password": "wrongpassword", + } + for i := range 10 { + rr := doRequest(t, handler, "POST", "/v1/auth/login", body, "") + if rr.Code == http.StatusTooManyRequests { + t.Fatalf("request %d was rate-limited prematurely", i+1) + } + } + + // The 11th request from the same IP should be rate-limited. + rr := doRequest(t, handler, "POST", "/v1/auth/login", body, "") + if rr.Code != http.StatusTooManyRequests { + t.Errorf("expected 429 after exhausting burst, got %d", rr.Code) + } + + // Verify the Retry-After header is set. + if rr.Header().Get("Retry-After") == "" { + t.Error("expected Retry-After header on 429 response") + } +} + +func TestTokenValidateRateLimited(t *testing.T) { + srv, _, _, _ := newTestServer(t) + handler := srv.Handler() + + // The token/validate endpoint shares the same per-IP rate limiter as login. + // Use a distinct RemoteAddr so we get a fresh bucket. + body := map[string]string{"token": "not.a.valid.token"} + + for i := range 10 { + b, _ := json.Marshal(body) + req := httptest.NewRequest("POST", "/v1/token/validate", bytes.NewReader(b)) + req.Header.Set("Content-Type", "application/json") + req.RemoteAddr = "10.99.99.1:12345" + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code == http.StatusTooManyRequests { + t.Fatalf("request %d was rate-limited prematurely", i+1) + } + } + + // 11th request should be rate-limited. + b, _ := json.Marshal(body) + req := httptest.NewRequest("POST", "/v1/token/validate", bytes.NewReader(b)) + req.Header.Set("Content-Type", "application/json") + req.RemoteAddr = "10.99.99.1:12345" + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusTooManyRequests { + t.Errorf("expected 429 after exhausting burst, got %d", rr.Code) + } +} + +func TestHealthNotRateLimited(t *testing.T) { + srv, _, _, _ := newTestServer(t) + handler := srv.Handler() + + // Health endpoint should not be rate-limited — send 20 rapid requests. + for i := range 20 { + req := httptest.NewRequest("GET", "/v1/health", nil) + req.RemoteAddr = "10.88.88.1:12345" + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusOK { + t.Errorf("health request %d: status = %d, want 200", i+1, rr.Code) + } + } +} + func TestRenewToken(t *testing.T) { srv, _, priv, _ := newTestServer(t) acct := createTestHumanAccount(t, srv, "renew-user") diff --git a/internal/ui/ui.go b/internal/ui/ui.go index 60f7f5b..9df4b2c 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -153,17 +153,23 @@ func New(database *db.DB, cfg *config.Config, priv ed25519.PrivateKey, pub ed255 }, nil } -// Register attaches all UI routes to mux. +// Register attaches all UI routes to mux, wrapped with security headers. +// All UI responses (pages, fragments, redirects, static assets) carry the +// headers added by securityHeaders. func (u *UIServer) Register(mux *http.ServeMux) { + // Build a dedicated child mux for all UI routes so that securityHeaders + // can be applied once as a wrapping middleware rather than per-route. + uiMux := http.NewServeMux() + // Static assets — serve from the web/static/ sub-directory of the embed. staticSubFS, err := fs.Sub(web.StaticFS, "static") if err != nil { panic(fmt.Sprintf("ui: static sub-FS: %v", err)) } - mux.Handle("GET /static/", http.StripPrefix("/static/", http.FileServerFS(staticSubFS))) + uiMux.Handle("GET /static/", http.StripPrefix("/static/", http.FileServerFS(staticSubFS))) // Redirect root to login. - mux.HandleFunc("GET /", func(w http.ResponseWriter, r *http.Request) { + uiMux.HandleFunc("GET /", func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/" { http.Redirect(w, r, "/login", http.StatusFound) return @@ -172,9 +178,9 @@ func (u *UIServer) Register(mux *http.ServeMux) { }) // Auth routes (no session required). - mux.HandleFunc("GET /login", u.handleLoginPage) - mux.HandleFunc("POST /login", u.handleLoginPost) - mux.HandleFunc("POST /logout", u.handleLogout) + uiMux.HandleFunc("GET /login", u.handleLoginPage) + uiMux.HandleFunc("POST /login", u.handleLoginPost) + uiMux.HandleFunc("POST /logout", u.handleLogout) // Protected routes. auth := u.requireCookieAuth @@ -185,19 +191,24 @@ func (u *UIServer) Register(mux *http.ServeMux) { return auth(http.HandlerFunc(h)) } - mux.Handle("GET /dashboard", adminGet(u.handleDashboard)) - mux.Handle("GET /accounts", adminGet(u.handleAccountsList)) - mux.Handle("POST /accounts", admin(u.handleCreateAccount)) - mux.Handle("GET /accounts/{id}", adminGet(u.handleAccountDetail)) - mux.Handle("PATCH /accounts/{id}/status", admin(u.handleUpdateAccountStatus)) - mux.Handle("DELETE /accounts/{id}", admin(u.handleDeleteAccount)) - mux.Handle("GET /accounts/{id}/roles/edit", adminGet(u.handleRolesEditForm)) - mux.Handle("PUT /accounts/{id}/roles", admin(u.handleSetRoles)) - mux.Handle("DELETE /token/{jti}", admin(u.handleRevokeToken)) - mux.Handle("POST /accounts/{id}/token", admin(u.handleIssueSystemToken)) - mux.Handle("GET /audit", adminGet(u.handleAuditPage)) - mux.Handle("GET /audit/rows", adminGet(u.handleAuditRows)) - mux.Handle("GET /audit/{id}", adminGet(u.handleAuditDetail)) + uiMux.Handle("GET /dashboard", adminGet(u.handleDashboard)) + uiMux.Handle("GET /accounts", adminGet(u.handleAccountsList)) + uiMux.Handle("POST /accounts", admin(u.handleCreateAccount)) + uiMux.Handle("GET /accounts/{id}", adminGet(u.handleAccountDetail)) + uiMux.Handle("PATCH /accounts/{id}/status", admin(u.handleUpdateAccountStatus)) + uiMux.Handle("DELETE /accounts/{id}", admin(u.handleDeleteAccount)) + uiMux.Handle("GET /accounts/{id}/roles/edit", adminGet(u.handleRolesEditForm)) + uiMux.Handle("PUT /accounts/{id}/roles", admin(u.handleSetRoles)) + uiMux.Handle("DELETE /token/{jti}", admin(u.handleRevokeToken)) + uiMux.Handle("POST /accounts/{id}/token", admin(u.handleIssueSystemToken)) + uiMux.Handle("GET /audit", adminGet(u.handleAuditPage)) + uiMux.Handle("GET /audit/rows", adminGet(u.handleAuditRows)) + uiMux.Handle("GET /audit/{id}", adminGet(u.handleAuditDetail)) + + // Mount the wrapped UI mux on the parent mux. The "/" pattern acts as a + // catch-all for all UI paths; the more-specific /v1/ API patterns registered + // on the parent mux continue to take precedence per Go's routing rules. + mux.Handle("/", securityHeaders(uiMux)) } // ---- Middleware ---- @@ -362,6 +373,34 @@ func (u *UIServer) renderError(w http.ResponseWriter, r *http.Request, status in // Security: prevents memory exhaustion from oversized POST bodies (gosec G120). const maxFormBytes = 1 << 20 +// securityHeaders returns middleware that adds defensive HTTP headers to every +// UI response. +// +// Security rationale for each header: +// - Content-Security-Policy: restricts resource loading to same origin only, +// mitigating XSS escalation even if an attacker injects HTML (e.g. via a +// malicious stored username rendered in the admin panel). +// - X-Content-Type-Options: prevents MIME-sniffing; browsers must honour the +// declared Content-Type and not guess an executable type from body content. +// - X-Frame-Options: blocks the admin panel from being framed by a third-party +// origin, preventing clickjacking against admin actions. +// - Strict-Transport-Security: instructs browsers to use HTTPS for all future +// requests to this origin for two years, preventing TLS-strip on revisit. +// - Referrer-Policy: suppresses the Referer header on outbound navigations so +// JWTs or session identifiers embedded in URLs are not leaked to third parties. +func securityHeaders(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + h := w.Header() + h.Set("Content-Security-Policy", + "default-src 'self'; script-src 'self'; style-src 'self'; img-src 'self' data:; font-src 'self'") + h.Set("X-Content-Type-Options", "nosniff") + h.Set("X-Frame-Options", "DENY") + h.Set("Strict-Transport-Security", "max-age=63072000; includeSubDomains") + h.Set("Referrer-Policy", "no-referrer") + next.ServeHTTP(w, r) + }) +} + // clientIP extracts the client IP from RemoteAddr (best effort). func clientIP(r *http.Request) string { addr := r.RemoteAddr diff --git a/internal/ui/ui_test.go b/internal/ui/ui_test.go new file mode 100644 index 0000000..7c9aae1 --- /dev/null +++ b/internal/ui/ui_test.go @@ -0,0 +1,183 @@ +package ui + +import ( + "crypto/ed25519" + "crypto/rand" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "git.wntrmute.dev/kyle/mcias/internal/config" + "git.wntrmute.dev/kyle/mcias/internal/db" +) + +const testIssuer = "https://auth.example.com" + +// newTestMux creates a UIServer and returns the http.Handler used in production +// (a ServeMux with all UI routes registered, wrapped with securityHeaders). +func newTestMux(t *testing.T) http.Handler { + t.Helper() + + pub, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generate key: %v", err) + } + + database, err := db.Open(":memory:") + if err != nil { + t.Fatalf("open db: %v", err) + } + if err := db.Migrate(database); err != nil { + t.Fatalf("migrate db: %v", err) + } + t.Cleanup(func() { _ = database.Close() }) + + masterKey := make([]byte, 32) + if _, err := rand.Read(masterKey); err != nil { + t.Fatalf("generate master key: %v", err) + } + + cfg := config.NewTestConfig(testIssuer) + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + uiSrv, err := New(database, cfg, priv, pub, masterKey, logger) + if err != nil { + t.Fatalf("new UIServer: %v", err) + } + + mux := http.NewServeMux() + uiSrv.Register(mux) + return mux +} + +// assertSecurityHeaders verifies all mandatory defensive headers are present in +// resp with acceptable values. The label is used in failure messages to identify +// which endpoint the test was checking. +func assertSecurityHeaders(t *testing.T, h http.Header, label string) { + t.Helper() + + checks := []struct { + header string + wantSub string + }{ + {"Content-Security-Policy", "default-src 'self'"}, + {"X-Content-Type-Options", "nosniff"}, + {"X-Frame-Options", "DENY"}, + {"Strict-Transport-Security", "max-age="}, + {"Referrer-Policy", "no-referrer"}, + } + for _, c := range checks { + val := h.Get(c.header) + if val == "" { + t.Errorf("[%s] missing security header %s", label, c.header) + continue + } + if c.wantSub != "" && !strings.Contains(val, c.wantSub) { + t.Errorf("[%s] %s = %q, want substring %q", label, c.header, val, c.wantSub) + } + } +} + +// TestSecurityHeadersOnLoginPage verifies headers are present on the public login page. +func TestSecurityHeadersOnLoginPage(t *testing.T) { + mux := newTestMux(t) + + req := httptest.NewRequest(http.MethodGet, "/login", nil) + rr := httptest.NewRecorder() + mux.ServeHTTP(rr, req) + + assertSecurityHeaders(t, rr.Result().Header, "GET /login") +} + +// TestSecurityHeadersOnUnauthenticatedDashboard verifies headers are present even +// when the response is a redirect to login (no session cookie supplied). +func TestSecurityHeadersOnUnauthenticatedDashboard(t *testing.T) { + mux := newTestMux(t) + + req := httptest.NewRequest(http.MethodGet, "/dashboard", nil) + rr := httptest.NewRecorder() + mux.ServeHTTP(rr, req) + + assertSecurityHeaders(t, rr.Result().Header, "GET /dashboard (no session)") +} + +// TestSecurityHeadersOnRootRedirect verifies headers on the "/" → "/login" redirect. +func TestSecurityHeadersOnRootRedirect(t *testing.T) { + mux := newTestMux(t) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + rr := httptest.NewRecorder() + mux.ServeHTTP(rr, req) + + assertSecurityHeaders(t, rr.Result().Header, "GET /") +} + +// TestSecurityHeadersOnStaticAsset verifies headers are present on static file responses. +func TestSecurityHeadersOnStaticAsset(t *testing.T) { + mux := newTestMux(t) + + req := httptest.NewRequest(http.MethodGet, "/static/style.css", nil) + rr := httptest.NewRecorder() + mux.ServeHTTP(rr, req) + + // 200 or 404 — either way the securityHeaders wrapper must fire. + assertSecurityHeaders(t, rr.Result().Header, "GET /static/style.css") +} + +// TestCSPDirectives verifies the Content-Security-Policy includes same-origin +// directives for scripts and styles. +func TestCSPDirectives(t *testing.T) { + mux := newTestMux(t) + + req := httptest.NewRequest(http.MethodGet, "/login", nil) + rr := httptest.NewRecorder() + mux.ServeHTTP(rr, req) + + csp := rr.Header().Get("Content-Security-Policy") + for _, directive := range []string{ + "default-src 'self'", + "script-src 'self'", + "style-src 'self'", + } { + if !strings.Contains(csp, directive) { + t.Errorf("CSP missing directive %q; full value: %q", directive, csp) + } + } +} + +// TestHSTSMinAge verifies HSTS max-age is at least two years (63072000 seconds). +func TestHSTSMinAge(t *testing.T) { + mux := newTestMux(t) + + req := httptest.NewRequest(http.MethodGet, "/login", nil) + rr := httptest.NewRecorder() + mux.ServeHTTP(rr, req) + + hsts := rr.Header().Get("Strict-Transport-Security") + if !strings.Contains(hsts, "max-age=63072000") { + t.Errorf("HSTS = %q, want max-age=63072000 (2 years)", hsts) + } +} + +// TestSecurityHeadersMiddlewareUnit tests the securityHeaders middleware in +// isolation, independent of routing, to guard against future refactoring. +func TestSecurityHeadersMiddlewareUnit(t *testing.T) { + reached := false + inner := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + }) + + handler := securityHeaders(inner) + req := httptest.NewRequest(http.MethodGet, "/test", nil) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + if !reached { + t.Error("inner handler was not reached") + } + assertSecurityHeaders(t, rr.Result().Header, "unit test") +}