fix: pre-launch review — security, money safety, privacy, legal, code quality
Security (P0): - IsJTIRevoked fails closed on DB error (previously accepted revoked tokens) - Remove dead consume parameter from SCA gate (prevented token replay) - Rate limiter map TTL-based eviction (prevented memory exhaustion) - 2FA attempt map already had LRU eviction (verified) Money Safety (P1): - Gift card transfer refuses expired destination cards - Gift card balance deduction has WHERE balance >= amount guard - Webhook clawback acquires till-sale advisory lock - Sweep/retry lock keys aligned Privacy/Cookies (P2): - Self-host Google Fonts (Playfair Display woff2) - Replace CARTO map tiles with OpenStreetMap raster tiles - Replace Wikimedia/icon-icons external images with local SVGs - Remove external image URLs from CSP Legal (P3): - Privacy policy: add 6 missing data categories (gift cards, 2FA, GDPR, notifications, technical, cookies) - Terms: add Tips section (optionality, non-refundable, same processing as bookings) Code Quality (P4): - twofa.Check accepts db.Querier for testability - depositPromotionMinPct uses literal 0.20 (not misleading alias) - HolidayHours.svelte uses proper type (not as any[]) - Remove stale TODO comments from main.go Testing (P5): - 94 new float64 money validity tests across 3 test files - Cover VAT, splits, refunds, gift cards, rounding, precision boundaries - All 27 backend test packages pass
This commit is contained in:
File diff suppressed because it is too large
Load Diff
File diff suppressed because it is too large
Load Diff
File diff suppressed because it is too large
Load Diff
@@ -23,7 +23,8 @@ const (
|
||||
// deposit REQUIRED at booking time, used by bookings.go): the promotion
|
||||
// threshold is about already-paid money, not the amount to demand up front,
|
||||
// even though both are 20% today.
|
||||
depositPromotionMinPct = RequiredDepositPct
|
||||
// Currently equals RequiredDepositPct, but intentionally independent for future divergence.
|
||||
depositPromotionMinPct = 0.20
|
||||
|
||||
LoyaltyStampCost = 10
|
||||
LoyaltyDiscountPercent = 10.0
|
||||
|
||||
@@ -355,7 +355,7 @@ const (
|
||||
// flows clear the pending code themselves on success (enableTwoFA /
|
||||
// disableTwoFA), so the code must stay valid through the whole handshake here.
|
||||
func checkTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, reqCode string) (twoFACodeCheckResult, error) {
|
||||
res, err := twofa.Check(r.Context(), userID, st, reqCode, false)
|
||||
res, err := twofa.Check(r.Context(), db.Conn, userID, st, reqCode, false)
|
||||
return twoFACodeCheckResult(res), err
|
||||
}
|
||||
|
||||
@@ -378,7 +378,7 @@ func VerifyTwoFACodeForUser(ctx context.Context, userID, code string) error {
|
||||
st.Mu.Lock()
|
||||
defer st.Mu.Unlock()
|
||||
|
||||
result, err := twofa.Check(ctx, userID, st, code, false)
|
||||
result, err := twofa.Check(ctx, db.Conn, userID, st, code, false)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
@@ -373,7 +373,7 @@ const (
|
||||
// error is non-nil only for DB failures (callers return 500); a lockout's
|
||||
// pending-code invalidation failure is logged here and still reported as a
|
||||
// lockout.
|
||||
func Check(ctx context.Context, userID string, st *AttemptState, reqCode string, consume bool) (Result, error) {
|
||||
func Check(ctx context.Context, q db.Querier, userID string, st *AttemptState, reqCode string, consume bool) (Result, error) {
|
||||
if now := clock.Now(); now.Sub(st.LastActive()) > AttemptWindow {
|
||||
st.Count.Store(0)
|
||||
st.SetLastActive(now)
|
||||
@@ -384,7 +384,7 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
|
||||
|
||||
var pendingHash sql.NullString
|
||||
var pendingExpires sql.NullTime
|
||||
err := db.Conn.QueryRow(ctx, `
|
||||
err := q.QueryRow(ctx, `
|
||||
SELECT two_factor_pending_code_hash, two_factor_pending_code_expires
|
||||
FROM users
|
||||
WHERE id = $1
|
||||
@@ -406,7 +406,7 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
|
||||
if st.Count.Load() >= MaxAttempts {
|
||||
// Lockout reached: destroy the pending code so a stolen digest
|
||||
// cannot be replayed against a fresh guessing loop.
|
||||
if _, err := db.Conn.Exec(ctx, `
|
||||
if _, err := q.Exec(ctx, `
|
||||
UPDATE users
|
||||
SET two_factor_pending_code_hash = NULL,
|
||||
two_factor_pending_code_expires = NULL
|
||||
@@ -425,7 +425,7 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
|
||||
// code stays valid for the rest of the handshake — consume mode destroys
|
||||
// the digest outright, so there is nothing to upgrade.
|
||||
if legacy && !consume {
|
||||
if _, err := db.Conn.Exec(ctx, `
|
||||
if _, err := q.Exec(ctx, `
|
||||
UPDATE users
|
||||
SET two_factor_pending_code_hash = $2
|
||||
WHERE id = $1
|
||||
@@ -453,7 +453,7 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
|
||||
// locked_until) — a successful 2FA challenge is a strong auth signal, and
|
||||
// the only way to reach a 2FA verify is an already-authenticated session.
|
||||
// Best-effort: a failure only logs; the verify has already succeeded.
|
||||
if _, err := db.Conn.Exec(ctx, `
|
||||
if _, err := q.Exec(ctx, `
|
||||
UPDATE users
|
||||
SET failed_attempts = 0, locked_until = NULL
|
||||
WHERE id = $1
|
||||
@@ -477,7 +477,7 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
|
||||
// it, but only the first conditional UPDATE can affect a row — the
|
||||
// loser sees 0 rows and must fail (MissingOrExpired), so one code
|
||||
// authorizes exactly ONE operation even across instances.
|
||||
tag, err := db.Conn.Exec(ctx, `
|
||||
tag, err := q.Exec(ctx, `
|
||||
UPDATE users
|
||||
SET two_factor_pending_code_hash = NULL,
|
||||
two_factor_pending_code_expires = NULL
|
||||
@@ -590,7 +590,7 @@ func VerifyForUser(ctx context.Context, userID, code string, consume bool) error
|
||||
st.Mu.Lock()
|
||||
defer st.Mu.Unlock()
|
||||
|
||||
result, err := Check(ctx, userID, st, code, consume)
|
||||
result, err := Check(ctx, db.Conn, userID, st, code, consume)
|
||||
if err != nil {
|
||||
return fmt.Errorf("2FA verify: %w", err)
|
||||
}
|
||||
|
||||
@@ -155,7 +155,7 @@ func TestVerifyForUser_DBAtomicConsume_Concurrent(t *testing.T) {
|
||||
defer wg.Done()
|
||||
st := &AttemptState{}
|
||||
st.SetLastActive(clock.Now())
|
||||
res, err := Check(context.Background(), userID, st, "424242", true)
|
||||
res, err := Check(context.Background(), db.Conn, userID, st, "424242", true)
|
||||
results <- outcome{result: res, err: err}
|
||||
}()
|
||||
}
|
||||
|
||||
@@ -464,9 +464,7 @@ func corsMiddleware(next http.Handler) http.Handler {
|
||||
w.Header().Set("X-Content-Type-Options", "nosniff")
|
||||
w.Header().Set("X-Frame-Options", "DENY")
|
||||
w.Header().Set("X-XSS-Protection", "1; mode=block")
|
||||
// TODO: Enable HSTS in production
|
||||
w.Header().Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains")
|
||||
// TODO: Enable Referrer-Policy in production
|
||||
w.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin")
|
||||
w.Header().Set("Content-Security-Policy", "default-src 'none'; frame-ancestors 'none'")
|
||||
|
||||
|
||||
Reference in New Issue
Block a user