Comprehensive security hardening from audit findings: - Add validation tags to all DTO request structs (max lengths, ranges, enums) - Replace unsafe type assertions with MustGetAuthUser helper across all handlers - Remove query-param token auth from admin middleware (prevents URL token leakage) - Add request validation calls in handlers that were missing c.Validate() - Remove goroutines in handlers (timezone update now synchronous) - Add sanitize middleware and path traversal protection (path_utils) - Stop resetting admin passwords on migration restart - Warn on well-known default SECRET_KEY - Add ~30 new test files covering security regressions, auth safety, repos, and services - Add deploy/ config, audit digests, and AUDIT_FINDINGS documentation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1528 lines
87 KiB
Markdown
1528 lines
87 KiB
Markdown
# MyCrib Go Backend — Deep Audit Findings
|
|
|
|
**Date**: 2026-03-01
|
|
**Scope**: All non-test `.go` files under `myCribAPI-go/`
|
|
**Agents**: 9 parallel audit agents covering security, authorization, data integrity, concurrency, performance, error handling, architecture compliance, API contracts, and cross-cutting logic
|
|
|
|
---
|
|
|
|
## Summary
|
|
|
|
| Audit Area | Critical | Bug | Race Cond. | Logic Error | Silent Failure | Warning | Fragile | Performance | Total |
|
|
|---|---|---|---|---|---|---|---|---|---|
|
|
| Security & Input Validation | 7 | 12 | — | — | — | 17 | — | — | 36 |
|
|
| Authorization & Access Control | 6 | 6 | — | — | — | 9 | — | — | 21 |
|
|
| Data Integrity (GORM) | 7 | 7 | 3 | — | — | 11 | — | — | 28 |
|
|
| Concurrency & Race Conditions | 1 | 4 | 3 | — | — | 10 | — | — | 18 |
|
|
| Performance & Query Efficiency | — | — | — | — | — | 1 | — | 19 | 20 |
|
|
| Error Handling & Panic Safety | 17 | 10 | — | — | 9 | 12 | — | — | 48 |
|
|
| Architecture Compliance | — | 12 | — | — | — | 11 | — | — | 23 |
|
|
| API Contract & Validation | — | 19 | — | — | — | 30 | — | — | 49 |
|
|
| Cross-Cutting Logic | 5 | 5 | 3 | 3 | 1 | — | 4 | — | 21 |
|
|
| **Total (raw)** | **43** | **75** | **9** | **3** | **10** | **101** | **4** | **19** | **264** |
|
|
|
|
---
|
|
|
|
## Audit 1: Security & Input Validation
|
|
|
|
### SEC-01 | CRITICAL | Apple JWS payload decoded without signature verification
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:190-192`
|
|
- **What**: `decodeAppleSignedPayload` splits the JWS token and base64-decodes the payload directly. The comment on line 190 explicitly says "we're trusting Apple's signature for now." `VerifyAppleSignature` exists but is never called from the handler flow.
|
|
- **Impact**: An attacker can craft a fake Apple webhook with arbitrary notification data (subscribe/renew/refund), granting or revoking Pro subscriptions for any user who has ever made a purchase.
|
|
|
|
### SEC-02 | CRITICAL | Google Pub/Sub webhook always returns true (unauthenticated)
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:787-793`
|
|
- **What**: `VerifyGooglePubSubToken` unconditionally returns `true`. The Google webhook endpoint `HandleGoogleWebhook` never calls this function at all. Any HTTP client can POST arbitrary subscription events.
|
|
- **Impact**: An attacker can forge Google subscription events to grant themselves Pro access, cancel other users' subscriptions, or trigger arbitrary downgrades.
|
|
|
|
### SEC-03 | CRITICAL | GoAdmin password reset to "admin" on every migration
|
|
- **File**: `internal/database/database.go:372-382`
|
|
- **What**: Line 373 does `INSERT ON CONFLICT DO NOTHING`, but line 379-382 unconditionally `UPDATE goadmin_users SET password = <bcrypt of "admin"> WHERE username = 'admin'`. Every time migrations run, the GoAdmin password is reset to "admin".
|
|
- **Impact**: The admin panel is permanently accessible with `admin/admin` credentials. Even if the password is changed, the next deploy resets it.
|
|
|
|
### SEC-04 | CRITICAL | Next.js admin password reset to "admin123" on every migration
|
|
- **File**: `internal/database/database.go:447-463`
|
|
- **What**: Lines 458-463 unconditionally update the admin@mycrib.com password to the bcrypt hash of "admin123" on every migration. The log message on line 463 even says "Updated admin@mycrib.com password to admin123."
|
|
- **Impact**: The admin API is permanently accessible with hardcoded credentials. Any attacker who discovers the endpoint can access full admin functionality.
|
|
|
|
### SEC-05 | CRITICAL | SQL injection via SortBy in all admin list endpoints
|
|
- **File**: `internal/admin/handlers/admin_user_handler.go:86-88`
|
|
- **What**: `sortBy = filters.SortBy` is concatenated directly into `query.Order(sortBy + " " + filters.GetSortDir())` without any allowlist validation. This pattern is repeated across every admin list handler (admin_user, auth_token, completion, contractor, document, device, notification, etc.).
|
|
- **Impact**: An authenticated admin can inject arbitrary SQL via the `sort_by` query parameter, e.g., `sort_by=created_at; DROP TABLE auth_user; --`, achieving full database read/write.
|
|
|
|
### SEC-06 | CRITICAL | Apple validation failure grants 1 month free Pro
|
|
- **File**: `internal/services/subscription_service.go:371`
|
|
- **What**: When Apple receipt validation returns a non-fatal error (network timeout, transient failure), the code falls through to `expiresAt = time.Now().UTC().AddDate(0, 1, 0)` -- granting 1 month of Pro. Line 381 grants 1 year when Apple IAP is not configured.
|
|
- **Impact**: An attacker can send any invalid receipt data, trigger a validation error, and receive free Pro access. Repeating monthly yields indefinite free Pro.
|
|
|
|
### SEC-07 | CRITICAL | Google validation failure grants 1 month free Pro; not configured grants 1 year
|
|
- **File**: `internal/services/subscription_service.go:429-449`
|
|
- **What**: Same pattern as Apple. Line 430: non-fatal Google validation error gives 1-month fallback. Line 449: unconfigured Google client gives 1 year. An attacker sending a garbage `purchaseToken` with `platform=android` triggers the fallback.
|
|
- **Impact**: Free Pro subscription for any user by sending invalid purchase data.
|
|
|
|
### SEC-08 | BUG | Token slice panic on short tokens
|
|
- **File**: `internal/middleware/auth.go:66`
|
|
- **What**: `token[:8]+"..."` in the debug log message will panic with an index-out-of-range if the token string is fewer than 8 characters. There is no length check before slicing.
|
|
- **Impact**: A malformed Authorization header with a valid scheme but very short token causes a server panic (500) and potential DoS.
|
|
|
|
### SEC-09 | BUG | Path traversal in resolveFilePath
|
|
- **File**: `internal/handlers/media_handler.go:156-171`
|
|
- **What**: `resolveFilePath` uses `strings.TrimPrefix` followed by `filepath.Join(uploadDir, relativePath)`. If the stored URL contains `../` sequences (e.g., `/uploads/../../../etc/passwd`), `filepath.Join` resolves them and `c.File()` serves the resulting path. There is no `filepath.Abs` containment check (unlike `storage_service.Delete`).
|
|
- **Impact**: If an attacker can control a stored URL (e.g., via SQL injection or a compromised document record), they can read arbitrary files from the server filesystem.
|
|
|
|
### SEC-10 | BUG | Path traversal in resolveImageFilePath (task service)
|
|
- **File**: `internal/services/task_service.go:850-862`
|
|
- **What**: Identical path traversal vulnerability to media_handler's `resolveFilePath`. No validation that the resolved path stays within the upload directory.
|
|
- **Impact**: Arbitrary file read if stored URLs are manipulated.
|
|
|
|
### SEC-11 | BUG | Path traversal check bypassed when filepath.Abs errors
|
|
- **File**: `internal/services/storage_service.go:137-138`
|
|
- **What**: `absUploadDir, _ := filepath.Abs(s.cfg.UploadDir)` and `absFilePath, _ := filepath.Abs(fullPath)` both silently discard errors. If `filepath.Abs` fails, both return empty strings, `strings.HasPrefix("", "")` is true, and the path traversal check passes.
|
|
- **Impact**: Under unusual filesystem conditions, the path containment check becomes ineffective, allowing deletion of arbitrary files.
|
|
|
|
### SEC-12 | BUG | Nil pointer panic after WebSocket upgrade failure
|
|
- **File**: `internal/monitoring/handler.go:116-119`
|
|
- **What**: When `upgrader.Upgrade` fails, `conn` is nil but execution continues to `defer conn.Close()`, causing a nil pointer panic.
|
|
- **Impact**: Server panic on any failed WebSocket upgrade attempt.
|
|
|
|
### SEC-13 | BUG | Missing return after context cancellation causes goroutine spin
|
|
- **File**: `internal/monitoring/handler.go:177`
|
|
- **What**: The `case <-ctx.Done():` block has no `return` statement, so after the context is cancelled, the `for` loop immediately re-enters the `select` and spins indefinitely.
|
|
- **Impact**: 100% CPU usage on one goroutine for every WebSocket connection that disconnects. The goroutine never exits, leaking resources.
|
|
|
|
### SEC-14 | BUG | Nil pointer dereference when cache is nil
|
|
- **File**: `internal/admin/handlers/lookup_handler.go:30-32`
|
|
- **What**: `cache := services.GetCache(); if cache == nil { }` has an empty body, then immediately calls `cache.CacheCategories()`. If cache is nil, this panics. Same pattern at lines 50-52 for priorities.
|
|
- **Impact**: Server panic in admin lookup handlers when Redis is unavailable.
|
|
|
|
### SEC-15 | BUG | Panic on short reset tokens
|
|
- **File**: `internal/admin/handlers/password_reset_code_handler.go:85`
|
|
- **What**: `code.ResetToken[:8] + "..." + code.ResetToken[len-4:]` panics with index out of range if the reset token is fewer than 8 characters.
|
|
- **Impact**: Admin panel crash when viewing short reset codes.
|
|
|
|
### SEC-16 | BUG | Race condition in Apple legacy receipt validation
|
|
- **File**: `internal/services/iap_validation.go:381-386`
|
|
- **What**: `c.sandbox = true` mutates the struct field, calls `validateLegacyReceipt`, then `c.sandbox = false`. If two concurrent requests hit this path, one may read a sandbox flag set by the other, causing production receipts to validate against sandbox or vice versa.
|
|
- **Impact**: Intermittent validation failures or sandbox receipts being accepted in production.
|
|
|
|
### SEC-17 | BUG | Index out of bounds in FCM response parsing
|
|
- **File**: `internal/push/fcm.go:119`
|
|
- **What**: `for i, result := range fcmResp.Results` iterates results and accesses `tokens[i]`. If FCM returns fewer results than tokens sent, this is safe, but if the response is malformed with more results than tokens, it panics.
|
|
- **Impact**: Server panic on malformed FCM responses.
|
|
|
|
### SEC-18 | BUG | DeleteFile endpoint allows deleting any file without ownership check
|
|
- **File**: `internal/handlers/upload_handler.go:78-91`
|
|
- **What**: The `DELETE /api/uploads/` endpoint accepts a `url` field in the request body and passes it directly to `storageService.Delete`. There is no check that the authenticated user owns or has access to the resource associated with that file.
|
|
- **Impact**: Any authenticated user can delete other users' uploaded files (images, documents, completion photos).
|
|
|
|
### SEC-19 | BUG | Unchecked type assertion throughout handlers
|
|
- **File**: `internal/handlers/contractor_handler.go:28` (and 60+ other locations)
|
|
- **What**: `c.Get(middleware.AuthUserKey).(*models.User)` is used without the comma-ok pattern across contractor_handler (7 instances), document_handler (10 instances), residence_handler (14 instances), task_handler (18 instances), and media_handler (3 instances). If the auth middleware is misconfigured or bypassed, this panics.
|
|
- **Impact**: Server panic (500) on any request where the context value is not the expected type.
|
|
|
|
### SEC-20 | WARNING | Admin JWT accepted via query parameter
|
|
- **File**: `internal/middleware/admin_auth.go:49-50`
|
|
- **What**: `tokenString = c.QueryParam("token")` allows passing the admin JWT as a URL query parameter. URL parameters are logged by web servers, proxies, and browser history.
|
|
- **Impact**: Admin tokens leaked into access logs, proxy logs, and referrer headers. A compromised log grants full admin access.
|
|
|
|
### SEC-21 | WARNING | Hardcoded debug secret key
|
|
- **File**: `internal/config/config.go:339`
|
|
- **What**: When `SECRET_KEY` is not set and `DEBUG=true`, the secret key defaults to `"change-me-in-production-secret-key-12345"`. If debug mode is accidentally enabled in production, all JWT signatures use this predictable key.
|
|
- **Impact**: Trivially forgeable admin JWTs if debug mode leaks to production.
|
|
|
|
### SEC-22 | WARNING | XSS in admin email HTML template
|
|
- **File**: `internal/admin/handlers/notification_handler.go:351-363`
|
|
- **What**: `req.Subject` and `req.Body` are concatenated directly into HTML via string concatenation (`+ req.Subject +`), with no HTML escaping. If the admin enters `<script>` tags, they are rendered in the email.
|
|
- **Impact**: Stored XSS in emails sent to users. A compromised admin account can inject malicious scripts into user-facing emails.
|
|
|
|
### SEC-23 | WARNING | User-controlled category parameter in upload path
|
|
- **File**: `internal/handlers/upload_handler.go:31`
|
|
- **What**: `category := c.QueryParam("category")` is passed to `storageService.Upload()`. While `Upload()` sanitizes to known subdirectories ("images", "documents", "completions"), the `category` value itself is not validated at the handler level and could be exploited if the sanitization logic changes.
|
|
- **Impact**: Low risk currently due to service-layer sanitization, but fragile defense.
|
|
|
|
### SEC-24 | WARNING | `binding` tag instead of `validate` on DeleteFile request
|
|
- **File**: `internal/handlers/upload_handler.go:80`
|
|
- **What**: The `DeleteFile` request struct uses `binding:"required"` (Gin format) instead of `validate:"required"` (Echo format). Echo's validator does not process `binding` tags, so the `url` field is never validated as required.
|
|
- **Impact**: Empty URL strings pass validation and could trigger unexpected behavior in the delete flow.
|
|
|
|
### SEC-25 | WARNING | No upper bound on notification list limit parameter
|
|
- **File**: `internal/handlers/notification_handler.go:29-33`
|
|
- **What**: The `limit` query parameter is parsed from user input with only a `> 0` check. A user can pass `limit=999999999` to retrieve an unbounded number of notifications in a single query.
|
|
- **Impact**: Denial of service via memory exhaustion on large notification tables.
|
|
|
|
### SEC-26 | WARNING | Raw error message returned to client
|
|
- **File**: `internal/handlers/contractor_handler.go:31`
|
|
- **What**: `err.Error()` is returned directly in JSON responses at contractor_handler:31, document_handler:92, and multiple other locations. Internal error messages may contain database details, file paths, or stack traces.
|
|
- **Impact**: Information disclosure of internal implementation details to attackers.
|
|
|
|
### SEC-27 | WARNING | Missing c.Validate() call on CreateContractor
|
|
- **File**: `internal/handlers/contractor_handler.go:55`
|
|
- **What**: After `c.Bind(&req)`, there is no `c.Validate(&req)` call, so `validate` tags on the request struct are never enforced.
|
|
- **Impact**: Invalid data bypasses server-side validation and reaches the service layer.
|
|
|
|
### SEC-28 | WARNING | WebSocket CheckOrigin always returns true
|
|
- **File**: `internal/monitoring/handler.go:19-22`
|
|
- **What**: The WebSocket upgrader accepts connections from any origin, with no CSRF or origin validation.
|
|
- **Impact**: Cross-site WebSocket hijacking. A malicious website can connect to the monitoring WebSocket from any origin and stream admin logs.
|
|
|
|
### SEC-29 | WARNING | Client-supplied X-Request-ID accepted without validation
|
|
- **File**: `internal/middleware/request_id.go:21`
|
|
- **What**: The middleware accepts any value from the `X-Request-ID` header without sanitization. An attacker can inject newlines or control characters.
|
|
- **Impact**: Log injection if the request ID is written to structured logs without escaping.
|
|
|
|
### SEC-30 | WARNING | Raw SQL execution from seed files
|
|
- **File**: `internal/admin/handlers/settings_handler.go:378`
|
|
- **What**: SQL statements from seed files are executed via `sqlDB.Exec(stmt)` with no parameterization.
|
|
- **Impact**: If seed file contents are ever derived from untrusted sources, full SQL injection.
|
|
|
|
### SEC-31 | WARNING | ClearAllData has no secondary confirmation
|
|
- **File**: `internal/admin/handlers/settings_handler.go:529-539`
|
|
- **What**: The `ClearAllData` endpoint deletes all user data with a single POST request. There is no confirmation token, password re-entry, or audit trail.
|
|
- **Impact**: A single compromised admin session can destroy all production data.
|
|
|
|
### SEC-32 | WARNING | RestoreSubscription missing receipt validation
|
|
- **File**: `internal/handlers/subscription_handler.go:159-163`
|
|
- **What**: Unlike `ProcessPurchase` which validates that receipt data or transaction ID is present, `RestoreSubscription` calls `ProcessApplePurchase` with potentially empty receipt data and transaction ID when platform is "ios".
|
|
- **Impact**: Compounded with the "validation failure grants Pro" bug, this creates another avenue for free Pro access.
|
|
|
|
### SEC-33 | WARNING | Fire-and-forget goroutines in handler (6 instances)
|
|
- **File**: `internal/handlers/auth_handler.go:83`
|
|
- **What**: Lines 83, 178, 207, 241, 329, 370 spawn goroutines with `go func()` for email sending. These violate the "no goroutines in handlers" rule and have no error context, no timeout, and no tracking.
|
|
- **Impact**: Silent email failures, goroutine leaks on shutdown, and potential resource exhaustion under load.
|
|
|
|
### SEC-34 | WARNING | UUID truncated to 8 characters
|
|
- **File**: `internal/services/storage_service.go:75`
|
|
- **What**: `uuid.New().String()[:8]` reduces the UUID from 128 bits to approximately 32 bits of randomness.
|
|
- **Impact**: Increased collision probability. At scale, filename collisions could overwrite existing uploads.
|
|
|
|
### SEC-35 | WARNING | TOCTOU race in ToggleFavorite
|
|
- **File**: `internal/repositories/contractor_repo.go:89-101`
|
|
- **What**: `ToggleFavorite` reads the current favorite state, then writes the opposite. Without a transaction, two concurrent requests can both read "not favorite" and both write "favorite", or vice versa.
|
|
- **Impact**: Inconsistent favorite state under concurrent access.
|
|
|
|
### SEC-36 | WARNING | Non-pointer bool `IsActive` defaults to false on PATCH
|
|
- **File**: `internal/admin/handlers/share_code_handler.go:155-162`
|
|
- **What**: `IsActive` is a `bool` (not `*bool`), so when the field is absent from the JSON body, it defaults to `false`. A PATCH request that only wants to update other fields will inadvertently deactivate the share code.
|
|
- **Impact**: Share codes silently deactivated by partial updates.
|
|
|
|
---
|
|
|
|
## Audit 2: Authorization & Access Control
|
|
|
|
### AUTH-01 | CRITICAL | Apple JWS payload decoded without signature verification
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:190-192`
|
|
- **What**: `decodeAppleSignedPayload` splits the JWS token and decodes the payload directly via base64, but never verifies the cryptographic signature. The comment on line 191 says "trusting Apple's signature for now."
|
|
- **Impact**: Attacker can upgrade any user to Pro, downgrade any user to Free, or revoke any subscription by sending forged webhook payloads to the public `/api/subscription/webhook/apple/` endpoint.
|
|
|
|
### AUTH-02 | CRITICAL | Google Pub/Sub webhook authentication always returns true
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:787-793`
|
|
- **What**: `VerifyGooglePubSubToken` unconditionally returns `true`. The Google webhook endpoint at `/api/subscription/webhook/google/` is publicly accessible (no auth middleware) and performs no caller verification.
|
|
- **Impact**: Attacker can upgrade any user to Pro, downgrade users to Free, or manipulate subscription state by sending forged Google webhook payloads.
|
|
|
|
### AUTH-03 | CRITICAL | IAP validation failure or misconfiguration grants free Pro subscription
|
|
- **File**: `internal/services/subscription_service.go:370-381`
|
|
- **What**: When Apple IAP validation fails with a non-fatal error (line 371), the service falls back to granting 1 month of free Pro. When the Apple client is not configured at all (line 381), the service grants 1 year of free Pro. The same applies to Google on lines 429 and 449.
|
|
- **Impact**: Complete subscription paywall bypass. Any user can get Pro for free by sending invalid receipt data to `POST /api/subscription/purchase/`.
|
|
|
|
### AUTH-04 | CRITICAL | Path traversal in resolveFilePath
|
|
- **File**: `internal/handlers/media_handler.go:156-171`
|
|
- **What**: `resolveFilePath` uses `filepath.Join(uploadDir, relativePath)` where `relativePath` comes from stored URLs in the database after `TrimPrefix`. The function lacks the `filepath.Abs` + `HasPrefix` guard used in `StorageService.Delete`.
|
|
- **Impact**: If an attacker can control a stored file URL, they can read arbitrary files from the server filesystem through the authenticated media proxy.
|
|
|
|
### AUTH-05 | CRITICAL | Subscription tier limit check commented out for residence creation
|
|
- **File**: `internal/services/residence_service.go:155`
|
|
- **What**: The `CreateResidence` method has the subscription tier limit check completely commented out (lines 155-160) with a TODO. The `CheckLimit` function exists in `SubscriptionService` but is never called from any create operation.
|
|
- **Impact**: Free-tier users can create unlimited residences, tasks, contractors, and documents, completely bypassing the subscription paywall.
|
|
|
|
### AUTH-06 | CRITICAL | Hardcoded admin credentials reset on every migration
|
|
- **File**: `internal/database/database.go:372-382,447-463`
|
|
- **What**: Hardcoded admin credentials (`admin@mycrib.com` / `admin123` and GoAdmin password of `admin`) are re-applied on every server restart/migration, overwriting any password changes.
|
|
- **Impact**: If these endpoints are accessible in production, any attacker with knowledge of these default credentials can gain full admin access.
|
|
|
|
### AUTH-07 | BUG | User-controlled category parameter enables storage path manipulation
|
|
- **File**: `internal/handlers/upload_handler.go:31`
|
|
- **What**: `UploadImage` reads the `category` query parameter from the user and passes it directly to `storageService.Upload`. The storage service's switch statement defaults to "images" for unknown categories, which is safe, but the `DeleteFile` endpoint passes user-supplied URLs to `storageService.Delete`.
|
|
- **Impact**: Low risk due to the safe default, but the delete endpoint accepts any URL and relies solely on the `filepath.Abs`/`HasPrefix` check which silently ignores errors.
|
|
|
|
### AUTH-08 | BUG | DeleteDevice does not verify device ownership
|
|
- **File**: `internal/services/notification_service.go:359-372`
|
|
- **What**: `DeleteDevice` accepts a `deviceID` and `platform` from the URL path, and the `userID` is extracted from the auth token. However, the method calls `DeactivateAPNSDevice(deviceID)` or `DeactivateGCMDevice(deviceID)` without first checking that the device belongs to `userID`.
|
|
- **Impact**: User A can disable push notifications for User B by guessing/enumerating device IDs via `DELETE /api/notifications/devices/:id/`.
|
|
|
|
### AUTH-09 | BUG | CreateContractor missing validation call
|
|
- **File**: `internal/handlers/contractor_handler.go:55`
|
|
- **What**: The `CreateContractor` handler binds the request body but never calls `c.Validate(&req)`. This means any `validate` tags on `CreateContractorRequest` fields are not enforced.
|
|
- **Impact**: Malformed or incomplete contractor data can be persisted to the database.
|
|
|
|
### AUTH-10 | BUG | Raw error string leaked to client in ListContractors
|
|
- **File**: `internal/handlers/contractor_handler.go:31`
|
|
- **What**: When `ListContractors` encounters an error, it returns `err.Error()` directly in the JSON response rather than using apperrors.
|
|
- **Impact**: Information disclosure of internal server state.
|
|
|
|
### AUTH-11 | BUG | UpdateContractor allows reassigning contractor to any residence without access check
|
|
- **File**: `internal/services/contractor_service.go:200`
|
|
- **What**: When updating a contractor, if `req.ResidenceID` is provided, the contractor's `ResidenceID` is set unconditionally. There is no check that the authenticated user has access to the new residence. The access check only validates the contractor's current residence.
|
|
- **Impact**: A user who has access to a contractor can reassign it to a residence they do not have access to, effectively injecting data into another user's residence.
|
|
|
|
### AUTH-12 | BUG | CreateTask missing validation call
|
|
- **File**: `internal/handlers/task_handler.go:112-115`
|
|
- **What**: The `CreateTask` handler binds the request but does not call `c.Validate(&req)`, so `validate` tags on `CreateTaskRequest` fields are not enforced.
|
|
- **Impact**: Invalid task data can be persisted.
|
|
|
|
### AUTH-13 | WARNING | UpdateTask missing validation call
|
|
- **File**: `internal/handlers/task_handler.go:134-137`
|
|
- **What**: Same issue as CreateTask -- `c.Validate(&req)` is never called for update requests.
|
|
- **Impact**: Invalid task update data can be persisted.
|
|
|
|
### AUTH-14 | WARNING | No DocumentType validation in form upload path
|
|
- **File**: `internal/handlers/document_handler.go:137`
|
|
- **What**: When creating a document via multipart form, the `document_type` form value is cast directly to `models.DocumentType` without validating against allowed enum values.
|
|
- **Impact**: Inconsistent document type data in the database.
|
|
|
|
### AUTH-15 | WARNING | No upper bound on notification list limit parameter
|
|
- **File**: `internal/handlers/notification_handler.go:29-34`
|
|
- **What**: The `limit` query parameter is parsed from user input with only a `> 0` check.
|
|
- **Impact**: Denial of service via memory exhaustion on large notification tables.
|
|
|
|
### AUTH-16 | WARNING | RestoreSubscription does not validate receipt/transaction presence
|
|
- **File**: `internal/handlers/subscription_handler.go:159-163`
|
|
- **What**: Unlike `ProcessPurchase`, `RestoreSubscription` calls `ProcessApplePurchase` with potentially empty receipt data.
|
|
- **Impact**: Empty receipt data stored; webhook user lookup can fail.
|
|
|
|
### AUTH-17 | WARNING | Token logging panics on short tokens
|
|
- **File**: `internal/middleware/auth.go:66`
|
|
- **What**: `token[:8]+"..."` panics if the token is fewer than 8 characters.
|
|
- **Impact**: DoS via short auth tokens.
|
|
|
|
### AUTH-18 | WARNING | Admin JWT accepted via query parameter
|
|
- **File**: `internal/middleware/admin_auth.go:50`
|
|
- **What**: Admin auth middleware falls back to reading the JWT token from the `token` query parameter. Query parameters are logged in access logs, proxy logs, and browser history.
|
|
- **Impact**: Admin JWT tokens leak into server logs.
|
|
|
|
### AUTH-19 | WARNING | DeleteFile endpoint allows deleting any file without ownership check
|
|
- **File**: `internal/handlers/upload_handler.go:78-91`
|
|
- **What**: The `DELETE /api/uploads/` endpoint accepts a `url` field and passes it to `storageService.Delete` with no ownership check.
|
|
- **Impact**: Any authenticated user can delete any uploaded file.
|
|
|
|
### AUTH-20 | WARNING | ClearAllData has no double-authentication check
|
|
- **File**: `internal/admin/handlers/settings_handler.go:529-793`
|
|
- **What**: The admin `ClearAllData` endpoint deletes all data with only standard admin JWT authentication.
|
|
- **Impact**: A compromised admin session can wipe all production data.
|
|
|
|
### AUTH-21 | WARNING | JoinWithCode missing validation call
|
|
- **File**: `internal/handlers/residence_handler.go:224`
|
|
- **What**: The `JoinWithCode` handler binds the request but never calls `c.Validate(&req)`.
|
|
- **Impact**: Empty share codes may be passed to the service layer.
|
|
|
|
---
|
|
|
|
## Audit 3: Data Integrity (GORM)
|
|
|
|
### DATA-01 | CRITICAL | Completion created but task update failure only logged, not returned
|
|
- **File**: `internal/services/task_service.go:601`
|
|
- **What**: After `CreateCompletion` succeeds at line 563, the subsequent `taskRepo.Update(task)` at line 601 can fail. When it does, the error is logged but NOT returned to the caller.
|
|
- **Impact**: One-time task appears as "not completed" in kanban. Recurring task shows wrong next occurrence. Data permanently inconsistent.
|
|
|
|
### DATA-02 | CRITICAL | Completion creation and task update not wrapped in a transaction
|
|
- **File**: `internal/services/task_service.go:563-606`
|
|
- **What**: `CreateCompletion` and `Update(task)` are two separate database operations with no transaction. If the process crashes between them, partial writes corrupt task state.
|
|
- **Impact**: A one-time task can have a completion record but still show as "active".
|
|
|
|
### DATA-03 | CRITICAL | ToggleFavorite read-then-write without transaction (TOCTOU)
|
|
- **File**: `internal/repositories/contractor_repo.go:89-101`
|
|
- **What**: `ToggleFavorite` reads the contractor, negates `IsFavorite`, then writes back as two separate operations. Also does not filter `is_active`.
|
|
- **Impact**: Race condition causes wrong favorite state. Soft-deleted contractors can be toggled.
|
|
|
|
### DATA-04 | CRITICAL | GetOrCreatePreferences has race condition creating duplicates
|
|
- **File**: `internal/repositories/notification_repo.go:137-161`
|
|
- **What**: `FindPreferencesByUser` and `CreatePreferences` are not atomic. Also uses `==` instead of `errors.Is` for `gorm.ErrRecordNotFound`.
|
|
- **Impact**: Duplicate key error returned to user on concurrent first access.
|
|
|
|
### DATA-05 | CRITICAL | GetOrCreate race condition for subscriptions
|
|
- **File**: `internal/repositories/subscription_repo.go:34-53`
|
|
- **What**: Same read-then-create pattern as notification_repo without transaction or ON CONFLICT clause.
|
|
- **Impact**: Concurrent first-time access causes duplicate subscription records or unique constraint errors.
|
|
|
|
### DATA-06 | CRITICAL | GoAdmin password reset on every migration run
|
|
- **File**: `internal/database/database.go:379-382`
|
|
- **What**: Unconditionally updates GoAdmin password to "admin" on every migration. Overwrites any password changes.
|
|
- **Impact**: Admin password reverts to known value on every deployment.
|
|
|
|
### DATA-07 | CRITICAL | Next.js admin password reset on every migration run
|
|
- **File**: `internal/database/database.go:458-463`
|
|
- **What**: Unconditionally updates admin@mycrib.com password to "admin123" on every migration.
|
|
- **Impact**: Same persistent backdoor.
|
|
|
|
### DATA-08 | BUG | GetAllUsers/HasAccess silently wrong when associations not preloaded
|
|
- **File**: `internal/models/residence.go:65-70`
|
|
- **What**: `GetAllUsers()` returns `Owner + Users` but assumes both are preloaded. If called on a Residence loaded without preloads, `Owner` is zero-value (ID=0) and `Users` is empty.
|
|
- **Impact**: Access checks return wrong results. GetAllUsers includes a phantom user with ID=0.
|
|
|
|
### DATA-09 | BUG | IsRecurring requires Frequency preloaded, returns false without it
|
|
- **File**: `internal/task/predicates/predicates.go:209-211`
|
|
- **What**: `IsRecurring` checks `task.Frequency != nil && task.Frequency.Days != nil`. If Frequency is not preloaded, always returns false.
|
|
- **Impact**: Code paths using `IsRecurring` on unpreloaded tasks get incorrect results.
|
|
|
|
### DATA-10 | BUG | DeleteCompletion ignores image deletion error
|
|
- **File**: `internal/repositories/task_repo.go:706-709`
|
|
- **What**: Image deletion error is discarded before deleting the completion itself.
|
|
- **Impact**: Orphaned TaskCompletionImage records remain in the database.
|
|
|
|
### DATA-11 | BUG | AddUser uses PostgreSQL-specific ON CONFLICT syntax
|
|
- **File**: `internal/repositories/residence_repo.go:125-128`
|
|
- **What**: `ON CONFLICT DO NOTHING` is PostgreSQL-specific. Tests use SQLite.
|
|
- **Impact**: Test coverage gap — join-with-code never truly tested against production SQL dialect.
|
|
|
|
### DATA-12 | BUG | generateUniqueCode ignores Count error
|
|
- **File**: `internal/repositories/residence_repo.go:298-301`
|
|
- **What**: `r.db.Model(...).Count(&count)` without checking the returned error. If query fails, `count` stays 0.
|
|
- **Impact**: Duplicate share codes during transient DB errors.
|
|
|
|
### DATA-13 | BUG | Task template Save without Omit could corrupt Category/Frequency
|
|
- **File**: `internal/repositories/task_template_repo.go:79-81`
|
|
- **What**: `r.db.Save(template)` without `Omit("Category", "Frequency")`. GORM's Save may attempt to update lookup records.
|
|
- **Impact**: Lookup data could be accidentally modified when updating a template.
|
|
|
|
### DATA-14 | BUG | IsActive/IsPro ignore IsFree admin override field
|
|
- **File**: `internal/models/subscription.go:57-65`
|
|
- **What**: `IsActive()` and `IsPro()` only check `Tier == TierPro` and expiry. They don't account for the `IsFree` admin override.
|
|
- **Impact**: Model methods misleading for admin-overridden users.
|
|
|
|
### DATA-15 | RACE_CONDITION | Unbounded goroutine in QuickComplete
|
|
- **File**: `internal/services/task_service.go:735`
|
|
- **What**: `go s.sendTaskCompletedNotification(task, completion)` spawns fire-and-forget goroutines. O(users * completions) with no backpressure.
|
|
- **Impact**: Memory exhaustion under heavy quick-complete load.
|
|
|
|
### DATA-16 | RACE_CONDITION | Unbounded goroutine per user in notification loop
|
|
- **File**: `internal/services/task_service.go:773`
|
|
- **What**: Spawns `go func(userID uint)` for each residence user, plus another goroutine for email. 2N goroutines per completion.
|
|
- **Impact**: Goroutine explosion for shared residences.
|
|
|
|
### DATA-17 | RACE_CONDITION | WithTransaction uses package-level global db
|
|
- **File**: `internal/database/database.go:100-102`
|
|
- **What**: `WithTransaction` calls `db.Transaction(fn)` where `db` is the package-level variable. If `Connect()` is called again, the pointer changes.
|
|
- **Impact**: Fragile coupling to mutable global.
|
|
|
|
### DATA-18 | WARNING | LIKE wildcards in search not escaped
|
|
- **File**: `internal/repositories/document_repo.go:91-93`
|
|
- **What**: User-provided `filter.Search` concatenated with `%` for `ILIKE`. `%` or `_` in search get unexpected wildcard matching.
|
|
- **Impact**: Unexpected search results.
|
|
|
|
### DATA-19 | WARNING | Same LIKE wildcard escape issue in template search
|
|
- **File**: `internal/repositories/task_template_repo.go:48`
|
|
- **What**: Same pattern as document_repo.
|
|
- **Impact**: Unexpected search results with special characters.
|
|
|
|
### DATA-20 | WARNING | GORM v1 query option pattern for SELECT FOR UPDATE
|
|
- **File**: `internal/repositories/subscription_repo.go:66`
|
|
- **What**: `tx.Set("gorm:query_option", "FOR UPDATE")` is GORM v1 pattern. GORM v2 recommends `tx.Clauses(clause.Locking{Strength: "UPDATE"})`.
|
|
- **Impact**: Row locking may silently fail to acquire locks.
|
|
|
|
### DATA-21 | WARNING | N+1 queries in getUserUsage
|
|
- **File**: `internal/services/subscription_service.go:186-204`
|
|
- **What**: 3 separate COUNT queries per residence for subscription usage check.
|
|
- **Impact**: Performance degradation for users with many properties.
|
|
|
|
### DATA-22 | WARNING | Apple validation failure grants free Pro (duplicate cross-ref)
|
|
- **File**: `internal/services/subscription_service.go:371`
|
|
- **What**: Non-fatal Apple error grants 1-month free Pro. Unconfigured client grants 1 year.
|
|
- **Impact**: Subscription paywall bypass.
|
|
|
|
### DATA-23 | WARNING | Same free-Pro-on-error for Google (duplicate cross-ref)
|
|
- **File**: `internal/services/subscription_service.go:429-449`
|
|
- **What**: Same pattern for Google.
|
|
- **Impact**: Same.
|
|
|
|
### DATA-24 | WARNING | resolveImageFilePath has no path traversal validation
|
|
- **File**: `internal/services/task_service.go:857-862`
|
|
- **What**: Path traversal via `filepath.Join` without containment check.
|
|
- **Impact**: Arbitrary file read if stored URLs manipulated.
|
|
|
|
### DATA-25 | WARNING | UpdatePreferences uses bare Save without Omit
|
|
- **File**: `internal/repositories/notification_repo.go:133`
|
|
- **What**: `r.db.Save(prefs)` could update related models if associations added later.
|
|
- **Impact**: Low risk currently but fragile.
|
|
|
|
### DATA-26 | WARNING | Subscription Update uses bare Save
|
|
- **File**: `internal/repositories/subscription_repo.go:57`
|
|
- **What**: `r.db.Save(sub)` without Omit. Has a `User` relation that could be written.
|
|
- **Impact**: Potential unintended User table writes.
|
|
|
|
### DATA-27 | WARNING | GetKanbanColumnWithTimezone duplicates categorization chain logic
|
|
- **File**: `internal/models/task.go:189-264`
|
|
- **What**: Reimplements categorization chain inline to avoid circular dependency.
|
|
- **Impact**: Logic drift between implementations.
|
|
|
|
### DATA-28 | WARNING | DeactivateShareCode error silently ignored
|
|
- **File**: `internal/repositories/residence_repo.go:272`
|
|
- **What**: Error from deactivation discarded. Expired code stays active.
|
|
- **Impact**: Expired share codes not properly cleaned up.
|
|
|
|
---
|
|
|
|
## Audit 4: Concurrency & Race Conditions
|
|
|
|
### CONC-01 | CRITICAL | Unbounded goroutine creation per residence user for push notifications
|
|
- **File**: `internal/services/task_service.go:773`
|
|
- **What**: `go func(userID uint)` per user for push, plus `go func(u models.User, images []EmbeddedImage)` per user for email. 2N goroutines per completion with no upper bound.
|
|
- **Impact**: Memory exhaustion under heavy completion load. No panic recovery.
|
|
|
|
### CONC-02 | BUG | Nil pointer dereference after failed WebSocket upgrade
|
|
- **File**: `internal/monitoring/handler.go:117-120`
|
|
- **What**: After `upgrader.Upgrade` fails, `conn` is nil but execution continues to `defer conn.Close()`.
|
|
- **Impact**: Guaranteed nil pointer panic.
|
|
|
|
### CONC-03 | BUG | Missing return after context cancellation causes goroutine spin
|
|
- **File**: `internal/monitoring/handler.go:176-178`
|
|
- **What**: `case <-ctx.Done():` has no `return`. Goroutine spins indefinitely.
|
|
- **Impact**: 100% CPU per disconnected WebSocket client.
|
|
|
|
### CONC-04 | BUG | Token slice panic for short tokens
|
|
- **File**: `internal/middleware/auth.go:66`
|
|
- **What**: `token[:8]` panics if token is fewer than 8 characters.
|
|
- **Impact**: DoS via short auth tokens.
|
|
|
|
### CONC-05 | BUG | Index out of bounds in FCM response parsing
|
|
- **File**: `internal/push/fcm.go:119-126`
|
|
- **What**: Loop iterates results and indexes into `tokens[i]`. Malformed response panics.
|
|
- **Impact**: Server panic on malformed FCM responses.
|
|
|
|
### CONC-06 | RACE_CONDITION | ToggleFavorite read-then-write without transaction
|
|
- **File**: `internal/repositories/contractor_repo.go:89-101`
|
|
- **What**: Separate read and write without transaction. Concurrent toggles produce incorrect state.
|
|
- **Impact**: Wrong favorite state under concurrent usage.
|
|
|
|
### CONC-07 | RACE_CONDITION | GetOrCreatePreferences race condition
|
|
- **File**: `internal/repositories/notification_repo.go:137-161`
|
|
- **What**: Read-then-create without transaction. Also uses `==` instead of `errors.Is`.
|
|
- **Impact**: Duplicate key error on concurrent first access.
|
|
|
|
### CONC-08 | RACE_CONDITION | GetOrCreate for subscriptions race condition
|
|
- **File**: `internal/repositories/subscription_repo.go:34-53`
|
|
- **What**: Same pattern. Also uses `==` instead of `errors.Is`.
|
|
- **Impact**: Duplicate records on concurrent first login.
|
|
|
|
### CONC-09 | WARNING | Fire-and-forget goroutine for welcome email (6 instances)
|
|
- **File**: `internal/handlers/auth_handler.go:83`
|
|
- **What**: Six `go func()` calls for email sending with no `defer recover()` and no context.
|
|
- **Impact**: Unrecovered panic kills the server. Violates "no goroutines in handlers" rule.
|
|
|
|
### CONC-10 | WARNING | Fire-and-forget goroutine for UpdateUserTimezone
|
|
- **File**: `internal/handlers/task_handler.go:41`
|
|
- **What**: `go h.taskService.UpdateUserTimezone(user.ID, tzHeader)` with no error handling.
|
|
- **Impact**: Silent crash if service panics.
|
|
|
|
### CONC-11 | WARNING | Fire-and-forget goroutine for email open tracking
|
|
- **File**: `internal/handlers/tracking_handler.go:34`
|
|
- **What**: `go func() { _ = h.onboardingService.RecordEmailOpened(trackingID) }()`.
|
|
- **Impact**: Error discarded, goroutine untracked.
|
|
|
|
### CONC-12 | WARNING | QuickComplete sends notifications in goroutine
|
|
- **File**: `internal/services/task_service.go:735`
|
|
- **What**: Spawns nested goroutines with no tracking or recovery.
|
|
- **Impact**: Notifications silently lost on server shutdown.
|
|
|
|
### CONC-13 | WARNING | Global singleton `cacheInstance` without synchronization
|
|
- **File**: `internal/services/cache_service.go:21-53`
|
|
- **What**: `var cacheInstance *CacheService` set by `NewCacheService` and read by `GetCache` without mutex.
|
|
- **Impact**: Data race if accessed concurrently during init.
|
|
|
|
### CONC-14 | WARNING | Global Bundle variable set without synchronization
|
|
- **File**: `internal/i18n/i18n.go:16-46`
|
|
- **What**: `var Bundle *i18n.Bundle` nil until `Init()`. `NewLocalizer` dereferences without nil check. `MustParseMessageFileBytes` panics on malformed files.
|
|
- **Impact**: Panic if handler invoked before Init completes.
|
|
|
|
### CONC-15 | WARNING | runtime.ReadMemStats called twice per collection cycle
|
|
- **File**: `internal/monitoring/collector.go:96-110`
|
|
- **What**: `collectMemory` and `collectRuntime` each independently call `runtime.ReadMemStats`. Both cause stop-the-world pauses.
|
|
- **Impact**: Double STW pause per collection interval.
|
|
|
|
### CONC-16 | WARNING | SetAsynqInspector writes without synchronization
|
|
- **File**: `internal/monitoring/service.go:85-87`
|
|
- **What**: Sets `s.collector.asynqClient` which may be read concurrently by the collector goroutine.
|
|
- **Impact**: Data race window during initialization.
|
|
|
|
### CONC-17 | WARNING | Unbounded goroutine creation for every log line
|
|
- **File**: `internal/monitoring/writer.go:90`
|
|
- **What**: `go func() { _ = w.buffer.Push(entry) }()` per log line. Under high load, goroutines accumulate.
|
|
- **Impact**: Memory exhaustion if Redis is slow. No backpressure.
|
|
|
|
### CONC-18 | WARNING | log.Fatal in goroutines kills process without graceful shutdown
|
|
- **File**: `internal/cmd/worker/main.go:185-197`
|
|
- **What**: Scheduler and worker goroutines call `log.Fatal()` on error, which calls `os.Exit(1)` immediately.
|
|
- **Impact**: Bypasses deferred cleanup, graceful shutdown.
|
|
|
|
---
|
|
|
|
## Audit 5: Performance & Query Efficiency
|
|
|
|
### PERF-01 | PERFORMANCE | N+1 queries in getUserUsage: 3 COUNT queries per residence in loop
|
|
- **File**: `internal/services/subscription_service.go:186-204`
|
|
- **What**: 3 separate COUNT queries per residence. 5 residences = 16 DB queries.
|
|
- **Impact**: Should be a single aggregate query with `WHERE residence_id IN (...)`.
|
|
|
|
### PERF-02 | PERFORMANCE | Kanban board fires 5 separate queries instead of 1 batched query
|
|
- **File**: `internal/repositories/task_repo.go:504-553`
|
|
- **What**: `GetKanbanData` executes 5 independent queries for overdue, in-progress, due-soon, upcoming, completed.
|
|
- **Impact**: 5x database round-trips per kanban load. Main API endpoint on every app open.
|
|
|
|
### PERF-03 | PERFORMANCE | runtime.ReadMemStats called twice per collection cycle
|
|
- **File**: `internal/monitoring/collector.go:96-132`
|
|
- **What**: Two separate calls to `runtime.ReadMemStats` per collection interval.
|
|
- **Impact**: Double stop-the-world pause.
|
|
|
|
### PERF-04 | PERFORMANCE | cpu.Percent blocks for 1 full second on every collection
|
|
- **File**: `internal/monitoring/collector.go:82`
|
|
- **What**: `cpu.Percent(time.Second, false)` blocks 1 second. For 5-second interval, 20% of time spent blocking.
|
|
- **Impact**: Collector goroutine unresponsive for 1 second each cycle.
|
|
|
|
### PERF-05 | PERFORMANCE | Unbounded goroutine creation per log line
|
|
- **File**: `internal/monitoring/writer.go:90-92`
|
|
- **What**: New goroutine per log line for Redis LPUSH.
|
|
- **Impact**: Goroutine spike under heavy logging. Overwhelms Redis.
|
|
|
|
### PERF-06 | PERFORMANCE | WebSocket handler continues after upgrade failure
|
|
- **File**: `internal/monitoring/handler.go:116-119`
|
|
- **What**: No return after upgrade failure causes nil conn panic.
|
|
- **Impact**: Wasted resources on every failed upgrade.
|
|
|
|
### PERF-07 | PERFORMANCE | Goroutine spin after ctx.Done
|
|
- **File**: `internal/monitoring/handler.go:176-178`
|
|
- **What**: Missing return causes tight spin loop.
|
|
- **Impact**: CPU spin per disconnected WebSocket.
|
|
|
|
### PERF-08 | PERFORMANCE | N+1 query in admin document images list
|
|
- **File**: `internal/admin/handlers/document_image_handler.go:223-244`
|
|
- **What**: `toResponse` queries DB per image for Document+Residence. 20 images = 20 extra queries.
|
|
- **Impact**: Slow admin document images page.
|
|
|
|
### PERF-09 | PERFORMANCE | O(N*M) linear scan to match users to eligible list
|
|
- **File**: `internal/worker/jobs/handler.go:154-159`
|
|
- **What**: Nested loop checking user membership. 500 tasks * 100 users = 50,000 comparisons.
|
|
- **Impact**: Should use `map[uint]bool` for O(1) lookup.
|
|
|
|
### PERF-10 | PERFORMANCE | Daily digest fires 4 DB queries per user in loop
|
|
- **File**: `internal/worker/jobs/handler.go:321-401`
|
|
- **What**: 4 queries per eligible user. 100 users = 400 queries.
|
|
- **Impact**: Should batch task queries across all users.
|
|
|
|
### PERF-11 | PERFORMANCE | Unbounded goroutine creation per notification recipient
|
|
- **File**: `internal/services/task_service.go:773`
|
|
- **What**: 2N goroutines per completion (N = residence users).
|
|
- **Impact**: No rate limiting. Should use Asynq worker.
|
|
|
|
### PERF-12 | PERFORMANCE | LIKE search on apple_receipt_data blob column (unindexed)
|
|
- **File**: `internal/repositories/subscription_repo.go:126-134`
|
|
- **What**: `WHERE apple_receipt_data LIKE '%transactionID%'` does full table scan on blob column.
|
|
- **Impact**: Scales poorly. Should store transaction IDs in indexed column.
|
|
|
|
### PERF-13 | PERFORMANCE | No upper bound on notification query limit
|
|
- **File**: `internal/handlers/notification_handler.go:29-34`
|
|
- **What**: `limit=999999999` causes unbounded query.
|
|
- **Impact**: OOM from single request.
|
|
|
|
### PERF-14 | PERFORMANCE | FindByUser fetches all documents with no LIMIT
|
|
- **File**: `internal/repositories/document_repo.go:56-65`
|
|
- **What**: All documents across all residences with full preloads, no pagination.
|
|
- **Impact**: Memory bloat for power users.
|
|
|
|
### PERF-15 | PERFORMANCE | FindByUser fetches all contractors with no LIMIT
|
|
- **File**: `internal/repositories/contractor_repo.go:48-67`
|
|
- **What**: All contractors with full preloads, no pagination.
|
|
- **Impact**: Unbounded query result.
|
|
|
|
### PERF-16 | PERFORMANCE | GetEmailStats fires 4 separate COUNT queries
|
|
- **File**: `internal/services/onboarding_email_service.go:128-149`
|
|
- **What**: 4 individual COUNT queries. Could be 1 with conditional aggregation.
|
|
- **Impact**: Minor — admin-only endpoint.
|
|
|
|
### PERF-17 | PERFORMANCE | HasSentReminder called per-task in smart reminder loop
|
|
- **File**: `internal/worker/jobs/handler.go:740-741`
|
|
- **What**: Individual COUNT query per active task. 200 tasks = 200 queries.
|
|
- **Impact**: Should batch-check all tuples in single query.
|
|
|
|
### PERF-18 | PERFORMANCE | FindByResidence preloads Completions.Images for all tasks
|
|
- **File**: `internal/repositories/task_repo.go:267-278`
|
|
- **What**: Eagerly preloads `Completions.Images` and `Completions.CompletedBy` for every task.
|
|
- **Impact**: Over-fetching for list endpoints. 50 tasks * 3 completions * 2 images = 300+ objects.
|
|
|
|
### PERF-19 | WARNING | Task update failure after completion silently swallowed
|
|
- **File**: `internal/services/task_service.go:601-606`
|
|
- **What**: Error only logged, not returned. Task state becomes inconsistent.
|
|
- **Impact**: Stale NextDueDate persists until manual edit.
|
|
|
|
### PERF-20 | WARNING | WithTransaction uses global db variable
|
|
- **File**: `internal/database/database.go:100-102`
|
|
- **What**: Captures global `db` pointer. Reconnection could cause stale reference.
|
|
- **Impact**: Fragile coupling.
|
|
|
|
---
|
|
|
|
## Audit 6: Error Handling & Panic Safety
|
|
|
|
### ERR-01 | CRITICAL | Unchecked type assertion on auth user — task_handler.go
|
|
- **File**: `internal/handlers/task_handler.go:35`
|
|
- **What**: `c.Get(middleware.AuthUserKey).(*models.User)` without comma-ok across 14+ methods (lines 35, 65, 80, 109, 126, 148, 163, 180, 197, 214, 231, 249, 266, 281, 291, 307, 379, 399).
|
|
- **Impact**: Nil pointer panic on middleware misconfiguration.
|
|
|
|
### ERR-02 | CRITICAL | Unchecked type assertion — contractor_handler.go
|
|
- **File**: `internal/handlers/contractor_handler.go:28`
|
|
- **What**: Same pattern across 10 locations (lines 28, 38, 53, 68, 88, 103, 118, 133).
|
|
- **Impact**: Nil pointer panic.
|
|
|
|
### ERR-03 | CRITICAL | Unchecked type assertion — document_handler.go
|
|
- **File**: `internal/handlers/document_handler.go:37`
|
|
- **What**: Same pattern at lines 37, 74, 89, 100, 210, 229, 244, 259, 275, 319.
|
|
- **Impact**: Nil pointer panic.
|
|
|
|
### ERR-04 | CRITICAL | Unchecked type assertion — residence_handler.go
|
|
- **File**: `internal/handlers/residence_handler.go:38`
|
|
- **What**: Same pattern at lines 38, 50, 64, 77, 94, 114, 139, 157, 178, 200, 221, 238, 255, 292.
|
|
- **Impact**: Nil pointer panic.
|
|
|
|
### ERR-05 | CRITICAL | Unchecked type assertion — notification_handler.go
|
|
- **File**: `internal/handlers/notification_handler.go:27`
|
|
- **What**: Same pattern at lines 27, 55, 67, 84, 96, 108, 125, 142, 155, 181.
|
|
- **Impact**: Nil pointer panic.
|
|
|
|
### ERR-06 | CRITICAL | Unchecked type assertion — subscription_handler.go
|
|
- **File**: `internal/handlers/subscription_handler.go:26`
|
|
- **What**: Same pattern at lines 26, 38, 82, 94, 132, 147.
|
|
- **Impact**: Nil pointer panic.
|
|
|
|
### ERR-07 | CRITICAL | Unchecked type assertion — media_handler.go
|
|
- **File**: `internal/handlers/media_handler.go:43`
|
|
- **What**: Same pattern at lines 43, 76, 114.
|
|
- **Impact**: Nil pointer panic.
|
|
|
|
### ERR-08 | CRITICAL | Unchecked type assertion — user_handler.go
|
|
- **File**: `internal/handlers/user_handler.go:29`
|
|
- **What**: Same pattern at lines 29, 45, 63.
|
|
- **Impact**: Nil pointer panic.
|
|
|
|
### ERR-09 | CRITICAL | Apple JWS decoded without signature verification
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:190-192`
|
|
- **What**: Decodes payload without verifying signature.
|
|
- **Impact**: Forged webhook payloads can manipulate subscriptions.
|
|
|
|
### ERR-10 | CRITICAL | Google webhook always returns true
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:787-793`
|
|
- **What**: `VerifyGooglePubSubToken` returns `true` unconditionally.
|
|
- **Impact**: Forged subscription events accepted.
|
|
|
|
### ERR-11 | CRITICAL | Unchecked type assertion in VerifyAppleSignature
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:759`
|
|
- **What**: `x5c[0].(string)` and `cert.PublicKey.(*ecdsa.PublicKey)` unchecked. RSA cert panics.
|
|
- **Impact**: Panic on unexpected certificate format.
|
|
|
|
### ERR-12 | CRITICAL | Apple validation failure grants free Pro
|
|
- **File**: `internal/services/subscription_service.go:371`
|
|
- **What**: Non-fatal error fallback grants 1-month Pro.
|
|
- **Impact**: Free Pro on transient errors.
|
|
|
|
### ERR-13 | CRITICAL | Apple not configured grants 1-year free Pro
|
|
- **File**: `internal/services/subscription_service.go:381`
|
|
- **What**: `s.appleClient == nil` grants 1-year Pro.
|
|
- **Impact**: Free Pro on misconfigured deployments.
|
|
|
|
### ERR-14 | CRITICAL | Google validation failure/not configured grants free Pro
|
|
- **File**: `internal/services/subscription_service.go:429-449`
|
|
- **What**: Same pattern for Google.
|
|
- **Impact**: Same.
|
|
|
|
### ERR-15 | CRITICAL | WebSocket nil pointer dereference
|
|
- **File**: `internal/monitoring/handler.go:116-119`
|
|
- **What**: No return after upgrade failure. `conn` is nil.
|
|
- **Impact**: Guaranteed panic.
|
|
|
|
### ERR-16 | CRITICAL | Unchecked type assertion in GetAuthUser
|
|
- **File**: `internal/middleware/auth.go:209`
|
|
- **What**: `return user.(*models.User)` — panics on type mismatch.
|
|
- **Impact**: Panic if middleware sets wrong type.
|
|
|
|
### ERR-17 | CRITICAL | Unchecked type assertion in RequireSuperAdmin
|
|
- **File**: `internal/middleware/admin_auth.go:124`
|
|
- **What**: `adminUser := admin.(*models.AdminUser)` after only nil check.
|
|
- **Impact**: Panic on type mismatch.
|
|
|
|
### ERR-18 | BUG | Token slice panic on short tokens
|
|
- **File**: `internal/middleware/auth.go:66`
|
|
- **What**: `token[:8]` without length check.
|
|
- **Impact**: Panic on < 8 char tokens.
|
|
|
|
### ERR-19 | BUG | Raw error leaked — contractor_handler.go:31
|
|
- **File**: `internal/handlers/contractor_handler.go:31`
|
|
- **What**: `err.Error()` in JSON response.
|
|
- **Impact**: Information disclosure.
|
|
|
|
### ERR-20 | BUG | Raw error leaked — contractor_handler.go:150
|
|
- **File**: `internal/handlers/contractor_handler.go:150`
|
|
- **What**: Same pattern for GetSpecialties.
|
|
- **Impact**: Information disclosure.
|
|
|
|
### ERR-21 | BUG | Raw error leaked — document_handler.go:92
|
|
- **File**: `internal/handlers/document_handler.go:92`
|
|
- **What**: Same pattern for ListWarranties.
|
|
- **Impact**: Information disclosure.
|
|
|
|
### ERR-22 | BUG | Task update error after completion only logged
|
|
- **File**: `internal/services/task_service.go:601-606`
|
|
- **What**: Error logged but not returned. Stale task state.
|
|
- **Impact**: Data integrity issue.
|
|
|
|
### ERR-23 | BUG | WebSocket goroutine spin after ctx.Done
|
|
- **File**: `internal/monitoring/handler.go:177`
|
|
- **What**: Missing return. Tight spin loop.
|
|
- **Impact**: CPU exhaustion per disconnected client.
|
|
|
|
### ERR-24 | BUG | ToggleFavorite race condition
|
|
- **File**: `internal/repositories/contractor_repo.go:89-101`
|
|
- **What**: Read-then-write without transaction.
|
|
- **Impact**: Inconsistent state.
|
|
|
|
### ERR-25 | BUG | GetOrCreatePreferences race condition
|
|
- **File**: `internal/repositories/notification_repo.go:137-161`
|
|
- **What**: Read-then-create. Uses `==` instead of `errors.Is`.
|
|
- **Impact**: Duplicate key errors.
|
|
|
|
### ERR-26 | BUG | GetOrCreate subscription race condition
|
|
- **File**: `internal/repositories/subscription_repo.go:34-53`
|
|
- **What**: Same pattern. `==` instead of `errors.Is`.
|
|
- **Impact**: Duplicate records.
|
|
|
|
### ERR-27 | BUG | UTF-8 byte-level truncation breaks multi-byte characters
|
|
- **File**: `internal/services/pdf_service.go:131-132`
|
|
- **What**: `title = title[:32] + "..."` truncates at byte position, splitting multi-byte chars.
|
|
- **Impact**: Corrupted UTF-8 in PDFs for non-ASCII titles.
|
|
|
|
### ERR-28 | BUG | FCM index out of bounds
|
|
- **File**: `internal/push/fcm.go:119-126`
|
|
- **What**: Malformed FCM response panics.
|
|
- **Impact**: Server panic.
|
|
|
|
### ERR-29 | SILENT_FAILURE | HasSentEmail ignores Count error
|
|
- **File**: `internal/services/onboarding_email_service.go:43-46`
|
|
- **What**: DB error unchecked. Count stays 0, returns false.
|
|
- **Impact**: Duplicate onboarding emails during DB issues.
|
|
|
|
### ERR-30 | SILENT_FAILURE | GetEmailStats ignores 4 Count errors
|
|
- **File**: `internal/services/onboarding_email_service.go:128-146`
|
|
- **What**: Four count queries with no error checking.
|
|
- **Impact**: Admin dashboard shows zeros on failure.
|
|
|
|
### ERR-31 | SILENT_FAILURE | Delete error silently ignored
|
|
- **File**: `internal/services/onboarding_email_service.go:354`
|
|
- **What**: `s.db.Where(...).Delete(...)` return unchecked.
|
|
- **Impact**: Old record persists, subsequent Create may fail.
|
|
|
|
### ERR-32 | SILENT_FAILURE | DeactivateShareCode error ignored
|
|
- **File**: `internal/repositories/residence_repo.go:272`
|
|
- **What**: Error from deactivation discarded.
|
|
- **Impact**: Expired codes remain active.
|
|
|
|
### ERR-33 | SILENT_FAILURE | generateUniqueCode Count error unchecked
|
|
- **File**: `internal/repositories/residence_repo.go:298-301`
|
|
- **What**: DB error unchecked. Code considered unique on failure.
|
|
- **Impact**: Duplicate share codes possible.
|
|
|
|
### ERR-34 | SILENT_FAILURE | DeleteCompletion ignores image deletion error
|
|
- **File**: `internal/repositories/task_repo.go:708`
|
|
- **What**: Image deletion error discarded before completion delete.
|
|
- **Impact**: Orphaned image records.
|
|
|
|
### ERR-35 | SILENT_FAILURE | GetAllStats error silently ignored in sendStats
|
|
- **File**: `internal/monitoring/handler.go:183-184`
|
|
- **What**: `if err != nil { }` — empty error handler.
|
|
- **Impact**: Nil or stale stats sent to clients.
|
|
|
|
### ERR-36 | SILENT_FAILURE | WriteJSON error unchecked
|
|
- **File**: `internal/monitoring/handler.go:192`
|
|
- **What**: `conn.WriteJSON(wsMsg)` return unchecked.
|
|
- **Impact**: Broken connections not detected.
|
|
|
|
### ERR-37 | SILENT_FAILURE | Bind error silently discarded
|
|
- **File**: `internal/handlers/residence_handler.go:187`
|
|
- **What**: `c.Bind(&req)` return discarded. Zero-value request used.
|
|
- **Impact**: Silent use of zero-value struct.
|
|
|
|
### ERR-38 | WARNING | Fire-and-forget goroutines in auth_handler (6 instances)
|
|
- **File**: `internal/handlers/auth_handler.go:83,178,207,241,329,370`
|
|
- **What**: Untracked goroutines for email. No context, no recovery.
|
|
- **Impact**: Goroutine leaks, silent failures.
|
|
|
|
### ERR-39 | WARNING | Fire-and-forget goroutine for timezone
|
|
- **File**: `internal/handlers/task_handler.go:41`
|
|
- **What**: `go h.taskService.UpdateUserTimezone(...)`.
|
|
- **Impact**: Silent crash on panic.
|
|
|
|
### ERR-40 | WARNING | Fire-and-forget goroutine for tracking
|
|
- **File**: `internal/handlers/tracking_handler.go:34`
|
|
- **What**: Error discarded, goroutine untracked.
|
|
- **Impact**: Silent failures.
|
|
|
|
### ERR-41 | WARNING | Missing c.Validate() — contractor_handler.go:55
|
|
- **File**: `internal/handlers/contractor_handler.go:55`
|
|
- **What**: Bind without Validate.
|
|
- **Impact**: Validation tags ignored.
|
|
|
|
### ERR-42 | WARNING | Missing c.Validate() — task_handler.go:113,135
|
|
- **File**: `internal/handlers/task_handler.go:113`
|
|
- **What**: CreateTask and UpdateTask skip Validate.
|
|
- **Impact**: Validation tags ignored.
|
|
|
|
### ERR-43 | WARNING | RestoreSubscription missing receipt validation
|
|
- **File**: `internal/handlers/subscription_handler.go:159`
|
|
- **What**: No receipt/token presence check before calling service.
|
|
- **Impact**: Empty data reaches validation.
|
|
|
|
### ERR-44 | WARNING | Admin JWT via query parameter
|
|
- **File**: `internal/middleware/admin_auth.go:50`
|
|
- **What**: JWT in URL params logged by servers/proxies.
|
|
- **Impact**: Token leakage.
|
|
|
|
### ERR-45 | WARNING | No notification limit cap
|
|
- **File**: `internal/handlers/notification_handler.go:29`
|
|
- **What**: Unbounded limit parameter.
|
|
- **Impact**: Memory exhaustion.
|
|
|
|
### ERR-46 | WARNING | Path traversal in task service resolveImageFilePath
|
|
- **File**: `internal/services/task_service.go:857-862`
|
|
- **What**: No containment check.
|
|
- **Impact**: Arbitrary file read via email embedding.
|
|
|
|
### ERR-47 | WARNING | filepath.Abs errors ignored in storage_service
|
|
- **File**: `internal/services/storage_service.go:137-138`
|
|
- **What**: Errors discarded, security check ineffective.
|
|
- **Impact**: Path traversal on Abs failure.
|
|
|
|
### ERR-48 | WARNING | MustParseMessageFileBytes panics on malformed translations
|
|
- **File**: `internal/i18n/i18n.go:37`
|
|
- **What**: Panics on invalid JSON in embedded files.
|
|
- **Impact**: Server fails to start on corrupt embed.
|
|
|
|
### ERR-49 | CRITICAL | Unchecked type assertions in timezone middleware
|
|
- **File**: `internal/middleware/timezone.go:88,99`
|
|
- **What**: `loc.(*time.Location)` and `now.(time.Time)` without comma-ok.
|
|
- **Impact**: Panic on type mismatch.
|
|
|
|
---
|
|
|
|
## Audit 7: Architecture Compliance
|
|
|
|
### ARCH-01 | WARNING | Goroutine in auth_handler for email (6 instances)
|
|
- **File**: `internal/handlers/auth_handler.go:83,178,206,240,328,370`
|
|
- **What**: Violates "no goroutines in handlers" rule.
|
|
- **Impact**: Untracked goroutines, no error recovery.
|
|
|
|
### ARCH-02 | WARNING | Goroutine in tracking_handler
|
|
- **File**: `internal/handlers/tracking_handler.go:34`
|
|
- **What**: Same violation.
|
|
- **Impact**: Error discarded.
|
|
|
|
### ARCH-03 | WARNING | Goroutine in task_handler for timezone
|
|
- **File**: `internal/handlers/task_handler.go:41`
|
|
- **What**: Same violation.
|
|
- **Impact**: No error handling.
|
|
|
|
### ARCH-04 | BUG | MediaHandler accesses repositories directly
|
|
- **File**: `internal/handlers/media_handler.go:19`
|
|
- **What**: Holds `*repositories.DocumentRepository`, `*repositories.TaskRepository`, `*repositories.ResidenceRepository` directly. Calls repo methods from handler.
|
|
- **Impact**: Violates "Handlers MUST NOT access database directly".
|
|
|
|
### ARCH-05 | BUG | Raw error string leaked — contractor_handler.go:31
|
|
- **File**: `internal/handlers/contractor_handler.go:31`
|
|
- **What**: `err.Error()` in JSON response. Same at line 150.
|
|
- **Impact**: Violates "NEVER return raw error strings to clients".
|
|
|
|
### ARCH-06 | BUG | Raw error string leaked — document_handler.go:92
|
|
- **File**: `internal/handlers/document_handler.go:92`
|
|
- **What**: Same violation.
|
|
- **Impact**: Internal details exposed.
|
|
|
|
### ARCH-07 | WARNING | Missing c.Validate() — contractor_handler.go:55
|
|
- **File**: `internal/handlers/contractor_handler.go:55`
|
|
- **What**: Violates "All request bodies MUST use validate tags. NEVER trust raw input."
|
|
- **Impact**: Validation tags dead code.
|
|
|
|
### ARCH-08 | WARNING | Missing c.Validate() — task_handler.go:113,135
|
|
- **File**: `internal/handlers/task_handler.go:113`
|
|
- **What**: CreateTask and UpdateTask skip validation.
|
|
- **Impact**: Validation tags dead code.
|
|
|
|
### ARCH-09 | WARNING | RestoreSubscription missing validation
|
|
- **File**: `internal/handlers/subscription_handler.go:159`
|
|
- **What**: No receipt/token check.
|
|
- **Impact**: Empty data reaches service.
|
|
|
|
### ARCH-10 | BUG | Path traversal — media_handler.go:156
|
|
- **File**: `internal/handlers/media_handler.go:156`
|
|
- **What**: No `filepath.Abs` containment check.
|
|
- **Impact**: Arbitrary file read.
|
|
|
|
### ARCH-11 | WARNING | 72 unchecked type assertions across all handlers
|
|
- **File**: `internal/handlers/*.go`
|
|
- **What**: Systemic `c.Get(AuthUserKey).(*models.User)` without comma-ok.
|
|
- **Impact**: Panic on middleware misconfiguration.
|
|
|
|
### ARCH-12 | BUG | OnboardingEmailService bypasses repository layer
|
|
- **File**: `internal/services/onboarding_email_service.go:17`
|
|
- **What**: Holds `*gorm.DB` directly. Performs all DB operations without repository.
|
|
- **Impact**: Violates layered architecture. No centralized query management.
|
|
|
|
### ARCH-13 | WARNING | Goroutine in QuickComplete service
|
|
- **File**: `internal/services/task_service.go:735`
|
|
- **What**: Spawns goroutines from service. Creates cascade of untracked goroutines.
|
|
- **Impact**: Unbounded goroutine creation.
|
|
|
|
### ARCH-14 | BUG | Task update failure not returned
|
|
- **File**: `internal/services/task_service.go:601`
|
|
- **What**: Error logged but not returned after completion.
|
|
- **Impact**: Data integrity issue.
|
|
|
|
### ARCH-15 | BUG | Apple IAP validation failure grants free Pro
|
|
- **File**: `internal/services/subscription_service.go:371`
|
|
- **What**: Non-fatal errors grant Pro access.
|
|
- **Impact**: Paywall bypass.
|
|
|
|
### ARCH-16 | WARNING | stdlib log instead of zerolog in subscription_service
|
|
- **File**: `internal/services/subscription_service.go:7`
|
|
- **What**: Uses `"log"` instead of `"github.com/rs/zerolog/log"`.
|
|
- **Impact**: Subscription logs bypass structured logging pipeline.
|
|
|
|
### ARCH-17 | WARNING | resolveImageFilePath no path traversal validation
|
|
- **File**: `internal/services/task_service.go:850`
|
|
- **What**: No containment check.
|
|
- **Impact**: Potential file read escape.
|
|
|
|
### ARCH-18 | BUG | Apple JWS not verified
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:190`
|
|
- **What**: Payload decoded without signature verification.
|
|
- **Impact**: Forged webhooks accepted.
|
|
|
|
### ARCH-19 | BUG | Google webhook always returns true
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:787`
|
|
- **What**: `VerifyGooglePubSubToken` returns true unconditionally.
|
|
- **Impact**: Forged events accepted.
|
|
|
|
### ARCH-20 | WARNING | stdlib log in webhook handler
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:11`
|
|
- **What**: Uses `"log"` instead of zerolog.
|
|
- **Impact**: Unstructured webhook processing logs.
|
|
|
|
### ARCH-21 | WARNING | Webhook handler bypasses service layer
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:26`
|
|
- **What**: Holds repos directly. Business logic duplicated.
|
|
- **Impact**: Violates "Handlers MUST NOT access database directly".
|
|
|
|
### ARCH-22 | BUG | GetKanbanColumnWithTimezone duplicates categorization chain
|
|
- **File**: `internal/models/task.go:201`
|
|
- **What**: 65-line method re-implements categorization chain inline due to circular dependency.
|
|
- **Impact**: Two implementations must be kept in sync. Violates "NEVER write inline task logic".
|
|
|
|
### ARCH-23 | WARNING | Worker handler accesses DB directly
|
|
- **File**: `internal/worker/jobs/handler.go:35`
|
|
- **What**: Holds `*gorm.DB` directly. Does raw GORM queries.
|
|
- **Impact**: Inconsistent data access patterns.
|
|
|
|
### ARCH-24 | BUG | Hardcoded admin password reset every migration
|
|
- **File**: `internal/database/database.go:372-382,446-463`
|
|
- **What**: Admin passwords reset to known defaults on every deploy.
|
|
- **Impact**: Persistent backdoor.
|
|
|
|
### ARCH-25 | WARNING | Hardcoded debug fallback secret key
|
|
- **File**: `internal/config/config.go:339`
|
|
- **What**: Deterministic key in debug mode.
|
|
- **Impact**: Forgeable JWTs if debug leaks to production.
|
|
|
|
### ARCH-26 | BUG | Token slice panic
|
|
- **File**: `internal/middleware/auth.go:66`
|
|
- **What**: `token[:8]` without length check.
|
|
- **Impact**: DoS via short tokens.
|
|
|
|
### ARCH-27 | BUG | WebSocket nil pointer after upgrade failure
|
|
- **File**: `internal/monitoring/handler.go:117`
|
|
- **What**: No return after failure. Nil conn used.
|
|
- **Impact**: Guaranteed panic.
|
|
|
|
---
|
|
|
|
## Audit 8: API Contract & Validation
|
|
|
|
### API-01 | BUG | CreateContractor skips validation after Bind
|
|
- **File**: `internal/handlers/contractor_handler.go:55`
|
|
- **What**: `c.Bind(&req)` without `c.Validate(&req)`. `CreateContractorRequest` has `required`, `min`, `max`, `email` tags.
|
|
- **Impact**: Empty names, invalid emails, oversized strings accepted.
|
|
|
|
### API-02 | BUG | UpdateContractor skips validation after Bind
|
|
- **File**: `internal/handlers/contractor_handler.go:75`
|
|
- **What**: Same issue.
|
|
- **Impact**: Max-length, email format constraints unenforced on updates.
|
|
|
|
### API-03 | BUG | UpdateDocument skips validation after Bind
|
|
- **File**: `internal/handlers/document_handler.go:217`
|
|
- **What**: Same issue. DTO has max length constraints.
|
|
- **Impact**: Oversized filenames/URLs persisted.
|
|
|
|
### API-04 | BUG | CreateDocument (JSON path) skips validation
|
|
- **File**: `internal/handlers/document_handler.go:196`
|
|
- **What**: JSON code path skips validation. Multipart path does manual partial validation.
|
|
- **Impact**: Required fields and max-length constraints unenforced.
|
|
|
|
### API-05 | BUG | CreateTask skips validation after Bind
|
|
- **File**: `internal/handlers/task_handler.go:113`
|
|
- **What**: `CreateTaskRequest` has `validate:"required"` on ResidenceID and Title, `min=1,max=200` on Title.
|
|
- **Impact**: Empty titles and zero residence_id accepted.
|
|
|
|
### API-06 | BUG | UpdateTask skips validation after Bind
|
|
- **File**: `internal/handlers/task_handler.go:135`
|
|
- **What**: `UpdateTaskRequest` has `validate:"omitempty,min=1,max=200"` on Title.
|
|
- **Impact**: Empty or oversized titles accepted.
|
|
|
|
### API-07 | BUG | CreateCompletion (JSON path) skips validation
|
|
- **File**: `internal/handlers/task_handler.go:365`
|
|
- **What**: `CreateTaskCompletionRequest` has `validate:"required"` on TaskID.
|
|
- **Impact**: `task_id: 0` completions created.
|
|
|
|
### API-08 | BUG | UpdateCompletion skips validation
|
|
- **File**: `internal/handlers/task_handler.go:386`
|
|
- **What**: Pattern inconsistency. No validate tags currently but pattern sets maintenance trap.
|
|
- **Impact**: Low immediate risk.
|
|
|
|
### API-09 | BUG | UpdatePreferences skips validation
|
|
- **File**: `internal/handlers/notification_handler.go:111`
|
|
- **What**: No validate tags on DTO currently. Missing Validate call.
|
|
- **Impact**: Maintenance trap.
|
|
|
|
### API-10 | BUG | RegisterDevice skips validation
|
|
- **File**: `internal/handlers/notification_handler.go:128`
|
|
- **What**: DTO has `binding:"required"` (Gin-style), which Echo ignores.
|
|
- **Impact**: Empty device records saved. Push notifications fail silently.
|
|
|
|
### API-11 | BUG | ProcessPurchase skips validation
|
|
- **File**: `internal/handlers/subscription_handler.go:97`
|
|
- **What**: DTO uses `binding:"required,oneof=ios android"` (Gin-style).
|
|
- **Impact**: Platform `oneof` never enforced (switch default catches it).
|
|
|
|
### API-12 | BUG | RestoreSubscription skips validation
|
|
- **File**: `internal/handlers/subscription_handler.go:150`
|
|
- **What**: No receipt/token presence check unlike ProcessPurchase.
|
|
- **Impact**: Empty data to Apple/Google validation.
|
|
|
|
### API-13 | BUG | JoinWithCode skips validation
|
|
- **File**: `internal/handlers/residence_handler.go:224`
|
|
- **What**: `JoinWithCodeRequest` has `validate:"required,len=6"` on Code.
|
|
- **Impact**: Empty or wrong-length codes reach service.
|
|
|
|
### API-14 | BUG | RegisterDeviceRequest uses `binding` tags (Gin)
|
|
- **File**: `internal/services/notification_service.go:552-554`
|
|
- **What**: Echo's validator only reads `validate` tags. `binding` tags are dead.
|
|
- **Impact**: No validation ever enforced on device registration.
|
|
|
|
### API-15 | BUG | ProcessPurchaseRequest uses `binding` tag
|
|
- **File**: `internal/services/subscription_service.go:657`
|
|
- **What**: Gin-style tag on Platform field.
|
|
- **Impact**: `oneof` constraint documentary-only.
|
|
|
|
### API-16 | BUG | DeleteFile uses `binding` tag
|
|
- **File**: `internal/handlers/upload_handler.go:80`
|
|
- **What**: `binding:"required"` on URL field. Echo ignores it.
|
|
- **Impact**: Empty URL passes through.
|
|
|
|
### API-17 | BUG | Raw error leaked — contractor_handler.go:31
|
|
- **File**: `internal/handlers/contractor_handler.go:31`
|
|
- **What**: `err.Error()` in JSON response.
|
|
- **Impact**: Internal details exposed.
|
|
|
|
### API-18 | BUG | Raw error leaked — contractor_handler.go:150
|
|
- **File**: `internal/handlers/contractor_handler.go:150`
|
|
- **What**: Same.
|
|
- **Impact**: Same.
|
|
|
|
### API-19 | BUG | Raw error leaked — document_handler.go:92
|
|
- **File**: `internal/handlers/document_handler.go:92`
|
|
- **What**: Same.
|
|
- **Impact**: Same.
|
|
|
|
### API-20 | BUG | Unchecked type assertions (72+ locations)
|
|
- **File**: `internal/handlers/*.go`
|
|
- **What**: Systemic `c.Get(AuthUserKey).(*models.User)` without comma-ok across all handlers.
|
|
- **Impact**: Panic on middleware misconfiguration.
|
|
|
|
### API-21 | BUG | Path traversal in resolveFilePath
|
|
- **File**: `internal/handlers/media_handler.go:156-171`
|
|
- **What**: No containment check after filepath.Join.
|
|
- **Impact**: Arbitrary file read.
|
|
|
|
### API-22 | BUG | Apple JWS not verified
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:190-192`
|
|
- **What**: Signature never checked.
|
|
- **Impact**: Forged webhooks accepted.
|
|
|
|
### API-23 | BUG | Google webhook always returns true
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:787-793`
|
|
- **What**: Unconditional `return true`.
|
|
- **Impact**: Forged events accepted.
|
|
|
|
### API-24 | BUG | Hardcoded admin credentials
|
|
- **File**: `internal/database/database.go:372-382,447-463`
|
|
- **What**: Reset on every migration.
|
|
- **Impact**: Permanent backdoor.
|
|
|
|
### API-25 | WARNING | Rating field — no min/max
|
|
- **File**: `internal/dto/requests/contractor.go:17`
|
|
- **What**: `Rating *float64` unbounded. Accepts -999 or 1000000.
|
|
- **Impact**: Garbage ratings stored.
|
|
|
|
### API-26 | WARNING | UpdateContractorRequest Rating unbounded
|
|
- **File**: `internal/dto/requests/contractor.go:34`
|
|
- **What**: Same issue on update path.
|
|
- **Impact**: Same.
|
|
|
|
### API-27 | WARNING | Completion Rating — no min/max
|
|
- **File**: `internal/dto/requests/task.go:93`
|
|
- **What**: `Rating *int` unbounded. Comment says "1-5 star" but nothing enforces it.
|
|
- **Impact**: Ratings of 0, -1, or 999 stored.
|
|
|
|
### API-28 | WARNING | UpdateCompletion Rating unbounded
|
|
- **File**: `internal/dto/requests/task.go:101`
|
|
- **What**: Same on update path.
|
|
- **Impact**: Same.
|
|
|
|
### API-29 | WARNING | CustomIntervalDays — no min validation
|
|
- **File**: `internal/dto/requests/task.go:63`
|
|
- **What**: Accepts 0 or negative values.
|
|
- **Impact**: Infinite loops or incorrect next-due-date calculations.
|
|
|
|
### API-30 | WARNING | Bedrooms and SquareFootage accept negatives
|
|
- **File**: `internal/dto/requests/residence.go:19-21`
|
|
- **What**: No `min=0` validation.
|
|
- **Impact**: -3 bedrooms, -500 sqft stored.
|
|
|
|
### API-31 | WARNING | ExpiresInHours — no validation
|
|
- **File**: `internal/dto/requests/residence.go:58`
|
|
- **What**: 0 or negative produces already-expired or never-expiring codes.
|
|
- **Impact**: Unexpected share code lifetimes.
|
|
|
|
### API-32 | WARNING | FileSize — no min validation
|
|
- **File**: `internal/dto/requests/document.go:19`
|
|
- **What**: Negative file sizes accepted.
|
|
- **Impact**: Cosmetic/reporting issue.
|
|
|
|
### API-33 | WARNING | ImageURLs array — no length limit
|
|
- **File**: `internal/dto/requests/document.go:28`
|
|
- **What**: Unbounded string array.
|
|
- **Impact**: DoS via database write amplification.
|
|
|
|
### API-34 | WARNING | CreateCompletion ImageURLs unbounded
|
|
- **File**: `internal/dto/requests/task.go:94`
|
|
- **What**: Same issue.
|
|
- **Impact**: Same.
|
|
|
|
### API-35 | WARNING | UpdateCompletion ImageURLs unbounded
|
|
- **File**: `internal/dto/requests/task.go:102`
|
|
- **What**: Same.
|
|
- **Impact**: Same.
|
|
|
|
### API-36 | WARNING | SpecialtyIDs array unbounded
|
|
- **File**: `internal/dto/requests/contractor.go:16`
|
|
- **What**: `SpecialtyIDs []uint` no length limit.
|
|
- **Impact**: Bulk insert DoS.
|
|
|
|
### API-37 | WARNING | ListNotifications — no upper bound on limit
|
|
- **File**: `internal/handlers/notification_handler.go:29-34`
|
|
- **What**: `limit=999999999` accepted.
|
|
- **Impact**: OOM from single request.
|
|
|
|
### API-38 | WARNING | ListContractors — no pagination
|
|
- **File**: `internal/handlers/contractor_handler.go:28`
|
|
- **What**: Returns all contractors, no limit/offset.
|
|
- **Impact**: Unbounded response payload.
|
|
|
|
### API-39 | WARNING | ListDocuments — no pagination
|
|
- **File**: `internal/handlers/document_handler.go:36`
|
|
- **What**: Returns all documents matching filter.
|
|
- **Impact**: Same.
|
|
|
|
### API-40 | WARNING | ListCompletions — no pagination
|
|
- **File**: `internal/handlers/task_handler.go:280`
|
|
- **What**: Returns all completions.
|
|
- **Impact**: Same.
|
|
|
|
### API-41 | WARNING | Inconsistent error response formats
|
|
- **File**: `internal/handlers/contractor_handler.go:31`
|
|
- **What**: `map[string]interface{}{"error":...}` vs `responses.ErrorResponse` vs `validator.FormatValidationErrors`. 3+ different shapes.
|
|
- **Impact**: Client must handle multiple error formats.
|
|
|
|
### API-42 | WARNING | Activate/Deactivate returns hybrid response
|
|
- **File**: `internal/handlers/document_handler.go:255`
|
|
- **What**: Returns `map[string]interface{}{"message":"...", "document": response}` mixing message with DTO.
|
|
- **Impact**: Inconsistent response shape.
|
|
|
|
### API-43 | WARNING | UploadImage accepts any file type
|
|
- **File**: `internal/handlers/upload_handler.go:24-42`
|
|
- **What**: No Content-Type or extension validation. Accepts executables, HTML, etc.
|
|
- **Impact**: Stored XSS risk; non-image files stored as images.
|
|
|
|
### API-44 | WARNING | User-controlled category in upload
|
|
- **File**: `internal/handlers/upload_handler.go:31`
|
|
- **What**: `category` query param passed to Upload without validation.
|
|
- **Impact**: Low risk due to service-layer sanitization.
|
|
|
|
### API-45 | WARNING | UploadDocumentImage accepts any file type
|
|
- **File**: `internal/handlers/document_handler.go:282-293`
|
|
- **What**: Same issue as UploadImage.
|
|
- **Impact**: Non-image files stored as images.
|
|
|
|
### API-46 | WARNING | DocumentType accepts arbitrary strings
|
|
- **File**: `internal/dto/requests/document.go:16`
|
|
- **What**: No `validate:"oneof=..."` tag.
|
|
- **Impact**: Unknown document types in database.
|
|
|
|
### API-47 | WARNING | Multipart DocumentType not validated
|
|
- **File**: `internal/handlers/document_handler.go:136-138`
|
|
- **What**: `models.DocumentType(docType)` casts any string.
|
|
- **Impact**: Same.
|
|
|
|
### API-48 | WARNING | Description fields — no max length (4 locations)
|
|
- **File**: `internal/dto/requests/task.go:59`, `document.go:15`, `contractor.go:11`, `residence.go:24`
|
|
- **What**: Description/Notes fields have no `max` validation.
|
|
- **Impact**: Multi-megabyte descriptions stored.
|
|
|
|
### API-49 | WARNING | Template search — no max query length
|
|
- **File**: `internal/handlers/task_template_handler.go:50-57`
|
|
- **What**: Minimum length (2) but no maximum. 10MB query string accepted.
|
|
- **Impact**: Slow LIKE queries, memory exhaustion.
|
|
|
|
---
|
|
|
|
## Audit 9: Cross-Cutting Logic
|
|
|
|
### CROSS-01 | CRITICAL | Task update error after completion logged but not returned
|
|
- **File**: `internal/services/task_service.go:601-606`
|
|
- **What**: `taskRepo.Update(task)` failure only logged. Version conflicts propagate but other errors swallowed. Inconsistent behavior.
|
|
- **Impact**: One-time tasks remain in kanban with stale NextDueDate. Recurring tasks show wrong next occurrence.
|
|
|
|
### CROSS-02 | CRITICAL | Apple IAP validation failure grants 1-month free Pro
|
|
- **File**: `internal/services/subscription_service.go:371`
|
|
- **What**: Non-fatal errors (network timeouts, server errors) fall through to 1-month Pro grant.
|
|
- **Impact**: Free Pro on transient Apple API errors.
|
|
|
|
### CROSS-03 | CRITICAL | Unconfigured Apple IAP grants 1-year free Pro
|
|
- **File**: `internal/services/subscription_service.go:378-381`
|
|
- **What**: `s.appleClient == nil` grants 1-year Pro.
|
|
- **Impact**: Free Pro on any deployment without Apple IAP configured.
|
|
|
|
### CROSS-04 | CRITICAL | Apple JWS not verified
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:190-192`
|
|
- **What**: `VerifyAppleSignature` exists but is never called. Returns nil when certs not loaded.
|
|
- **Impact**: Forged webhooks accepted.
|
|
|
|
### CROSS-05 | CRITICAL | Google webhook always returns true
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:787-793`
|
|
- **What**: Unconditional `return true`.
|
|
- **Impact**: Forged events accepted.
|
|
|
|
### CROSS-06 | BUG | resolveImageFilePath — path traversal
|
|
- **File**: `internal/services/task_service.go:857-862`
|
|
- **What**: No containment check. `os.ReadFile` could read arbitrary files.
|
|
- **Impact**: Arbitrary files embedded in notification emails.
|
|
|
|
### CROSS-07 | BUG | Media handler resolveFilePath — path traversal
|
|
- **File**: `internal/handlers/media_handler.go:156-171`
|
|
- **What**: Same issue. Stored URL controls file path.
|
|
- **Impact**: Arbitrary file serving via media endpoint.
|
|
|
|
### CROSS-08 | BUG | Subscription tier limits commented out
|
|
- **File**: `internal/services/residence_service.go:155`
|
|
- **What**: `CheckLimit` never called from any create operation.
|
|
- **Impact**: Free-tier users have unlimited everything.
|
|
|
|
### CROSS-09 | BUG | CreateTask handler missing c.Validate()
|
|
- **File**: `internal/handlers/task_handler.go:108-121`
|
|
- **What**: Validation tags dead code.
|
|
- **Impact**: Empty titles, missing residence_id accepted.
|
|
|
|
### CROSS-10 | BUG | Raw error leaked — contractor_handler.go:31
|
|
- **File**: `internal/handlers/contractor_handler.go:31`
|
|
- **What**: `err.Error()` in response.
|
|
- **Impact**: Internal details exposed.
|
|
|
|
### CROSS-11 | LOGIC_ERROR | Google renewal extends by guessing duration from product ID string
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:636-651`
|
|
- **What**: `strings.Contains(notification.SubscriptionID, "monthly")` determines duration. Uses `time.Now()` instead of current expiry.
|
|
- **Impact**: Wrong renewal duration if product renamed. Late renewals get extra time.
|
|
|
|
### CROSS-12 | LOGIC_ERROR | Google recovered/restarted hardcode 1-month regardless of plan
|
|
- **File**: `internal/handlers/subscription_webhook_handler.go:657,694`
|
|
- **What**: Both use `AddDate(0, 1, 0)` regardless of monthly vs yearly.
|
|
- **Impact**: Yearly subscribers get only 1 month after recovery/restart.
|
|
|
|
### CROSS-13 | LOGIC_ERROR | DeleteCompletion does not recalculate NextDueDate
|
|
- **File**: `internal/services/task_service.go:980-1007`
|
|
- **What**: Deletes completion but never recalculates task state. One-time tasks stay "Completed" with no completions.
|
|
- **Impact**: Permanent kanban column mismatch. Only manual due date edit fixes it.
|
|
|
|
### CROSS-14 | RACE_CONDITION | ToggleFavorite read-then-write
|
|
- **File**: `internal/repositories/contractor_repo.go:89-101`
|
|
- **What**: No transaction. Also doesn't filter `is_active`.
|
|
- **Impact**: Concurrent toggles produce wrong state. Soft-deleted contractors toggleable.
|
|
|
|
### CROSS-15 | RACE_CONDITION | GetOrCreatePreferences race
|
|
- **File**: `internal/repositories/notification_repo.go:137-161`
|
|
- **What**: Read-then-create. `==` instead of `errors.Is`.
|
|
- **Impact**: Duplicate key errors.
|
|
|
|
### CROSS-16 | RACE_CONDITION | GetOrCreate subscription race
|
|
- **File**: `internal/repositories/subscription_repo.go:34-53`
|
|
- **What**: Same pattern. Same error comparison issue.
|
|
- **Impact**: Duplicate records.
|
|
|
|
### CROSS-17 | SILENT_FAILURE | Share code deactivation error ignored
|
|
- **File**: `internal/services/residence_service.go:447-450`
|
|
- **What**: Empty `if err` block. No logging despite comment.
|
|
- **Impact**: Multi-use of one-time share codes.
|
|
|
|
### CROSS-18 | FRAGILE | QuickComplete goroutine cascade
|
|
- **File**: `internal/services/task_service.go:735`
|
|
- **What**: Spawns goroutines from service. Violates architecture rule. No tracking.
|
|
- **Impact**: Unbounded goroutines. Notifications lost on shutdown.
|
|
|
|
### CROSS-19 | FRAGILE | N+1 queries in getUserUsage
|
|
- **File**: `internal/services/subscription_service.go:186-204`
|
|
- **What**: 3 COUNT queries per residence.
|
|
- **Impact**: Linear query growth. Called on every subscription check.
|
|
|
|
### CROSS-20 | FRAGILE | Unchecked type assertions (72+ locations)
|
|
- **File**: `internal/handlers/*.go`
|
|
- **What**: Systemic pattern. `MustGetAuthUser` exists but unused outside auth_handler.
|
|
- **Impact**: Panic on middleware misconfiguration.
|
|
|
|
### CROSS-21 | FRAGILE | stdlib "log" instead of zerolog in subscription_service
|
|
- **File**: `internal/services/subscription_service.go:7`
|
|
- **What**: ~15 `log.Printf` calls bypass structured logging.
|
|
- **Impact**: Payment debugging harder. Logs not captured in pipeline.
|
|
|
|
---
|
|
|
|
## Deduplicated Priority List
|
|
|
|
### P0 — Exploitable Now (Fix Immediately)
|
|
|
|
| ID | Finding | Files |
|
|
|---|---|---|
|
|
| P0-1 | Apple JWS webhook not verified | `subscription_webhook_handler.go:190` |
|
|
| P0-2 | Google webhook auth always true | `subscription_webhook_handler.go:787` |
|
|
| P0-3 | IAP validation failure grants free Pro (Apple) | `subscription_service.go:371` |
|
|
| P0-4 | IAP not configured grants 1-year free Pro (Apple) | `subscription_service.go:381` |
|
|
| P0-5 | IAP validation failure grants free Pro (Google) | `subscription_service.go:429` |
|
|
| P0-6 | IAP not configured grants 1-year free Pro (Google) | `subscription_service.go:449` |
|
|
| P0-7 | Hardcoded admin "admin/admin" reset every migration | `database.go:372-382` |
|
|
| P0-8 | Hardcoded admin "admin123" reset every migration | `database.go:447-463` |
|
|
| P0-9 | SQL injection in admin sort_by params | `admin/handlers/*.go` |
|
|
| P0-10 | Path traversal — media handler resolveFilePath | `media_handler.go:156` |
|
|
| P0-11 | Path traversal — task service resolveImageFilePath | `task_service.go:857` |
|
|
| P0-12 | Subscription tier limits commented out | `residence_service.go:155` |
|
|
|
|
### P1 — Crashes & Data Corruption
|
|
|
|
| ID | Finding | Files |
|
|
|---|---|---|
|
|
| P1-1 | 72+ unchecked type assertions (panic risk) | All handlers |
|
|
| P1-2 | Token slice panic on < 8 char tokens | `auth.go:66` |
|
|
| P1-3 | WebSocket nil deref after upgrade failure | `monitoring/handler.go:117` |
|
|
| P1-4 | WebSocket goroutine spin after ctx.Done | `monitoring/handler.go:177` |
|
|
| P1-5 | Completion + task update not in transaction | `task_service.go:563-606` |
|
|
| P1-6 | Task update error after completion swallowed | `task_service.go:601` |
|
|
| P1-7 | DeleteCompletion doesn't recalculate NextDueDate | `task_service.go:980` |
|
|
| P1-8 | 3 TOCTOU race conditions (GetOrCreate patterns) | `notification_repo.go:137`, `subscription_repo.go:34`, `contractor_repo.go:89` |
|
|
| P1-9 | ToggleFavorite on soft-deleted contractors | `contractor_repo.go:91` |
|
|
| P1-10 | UpdateContractor allows cross-residence reassignment | `contractor_service.go:200` |
|
|
| P1-11 | DeleteDevice no ownership check | `notification_service.go:359` |
|
|
| P1-12 | DeleteFile no ownership check | `upload_handler.go:78` |
|
|
| P1-13 | FCM response index out of bounds | `fcm.go:119` |
|
|
| P1-14 | Admin lookup handler nil cache panic | `admin/lookup_handler.go:30` |
|
|
| P1-15 | Apple legacy receipt sandbox race condition | `iap_validation.go:381` |
|
|
|
|
### P2 — Resource Exhaustion & Performance
|
|
|
|
| ID | Finding | Files |
|
|
|---|---|---|
|
|
| P2-1 | Unbounded goroutines per notification (2N per completion) | `task_service.go:773` |
|
|
| P2-2 | 8+ fire-and-forget goroutines in handlers | `auth_handler.go:83`, `task_handler.go:41`, `tracking_handler.go:34` |
|
|
| P2-3 | No limit cap on notification queries | `notification_handler.go:29` |
|
|
| P2-4 | N+1 queries: getUserUsage (3N+1 per subscription check) | `subscription_service.go:186` |
|
|
| P2-5 | Kanban 5 separate queries instead of 1 | `task_repo.go:504` |
|
|
| P2-6 | Daily digest 4 queries per user | `worker/jobs/handler.go:321` |
|
|
| P2-7 | Smart reminder N queries per active task | `worker/jobs/handler.go:740` |
|
|
| P2-8 | LIKE search on unindexed receipt blob | `subscription_repo.go:126` |
|
|
| P2-9 | FindByUser unbounded (documents + contractors) | `document_repo.go:56`, `contractor_repo.go:48` |
|
|
| P2-10 | FindByResidence over-preloads Completions.Images | `task_repo.go:267` |
|
|
| P2-11 | Unbounded goroutines per log line | `monitoring/writer.go:90` |
|
|
| P2-12 | cpu.Percent blocks 1 second per collection | `monitoring/collector.go:82` |
|
|
| P2-13 | Double ReadMemStats per collection | `monitoring/collector.go:96` |
|
|
| P2-14 | O(N*M) linear scan in reminder jobs | `worker/jobs/handler.go:154` |
|
|
| P2-15 | Admin document images N+1 query | `admin/document_image_handler.go:223` |
|
|
| P2-16 | 4 separate COUNT queries for email stats | `onboarding_email_service.go:128` |
|
|
|
|
### P3 — Validation & Hardening
|
|
|
|
| ID | Finding | Files |
|
|
|---|---|---|
|
|
| P3-1 | 13 handlers missing c.Validate() after Bind | Multiple handlers |
|
|
| P3-2 | 3 DTOs use Gin `binding` tags instead of Echo `validate` | `notification_service.go:552`, `subscription_service.go:657`, `upload_handler.go:80` |
|
|
| P3-3 | 3 handlers leak raw err.Error() to clients | `contractor_handler.go:31,150`, `document_handler.go:92` |
|
|
| P3-4 | Rating fields unbounded (contractor + completion) | `contractor.go:17,34`, `task.go:93,101` |
|
|
| P3-5 | CustomIntervalDays accepts 0/negative | `task.go:63` |
|
|
| P3-6 | Bedrooms/SquareFootage accept negatives | `residence.go:19-21` |
|
|
| P3-7 | ExpiresInHours no validation | `residence.go:58` |
|
|
| P3-8 | FileSize accepts negative | `document.go:19` |
|
|
| P3-9 | Array fields unbounded (ImageURLs, SpecialtyIDs) | `document.go:28`, `task.go:94,102`, `contractor.go:16` |
|
|
| P3-10 | Description/Notes no max length (4 locations) | `task.go:59`, `document.go:15`, `contractor.go:11`, `residence.go:24` |
|
|
| P3-11 | DocumentType no enum validation | `document.go:16`, `document_handler.go:136` |
|
|
| P3-12 | Upload accepts any Content-Type | `upload_handler.go:24`, `document_handler.go:282` |
|
|
| P3-13 | Search query no max length | `task_template_handler.go:50` |
|
|
| P3-14 | No pagination on 3 list endpoints | `contractor_handler.go:28`, `document_handler.go:36`, `task_handler.go:280` |
|
|
| P3-15 | Inconsistent error response formats | Multiple handlers |
|
|
| P3-16 | Google renewal duration guessed from product ID string | `subscription_webhook_handler.go:636` |
|
|
| P3-17 | Google recovered/restarted hardcode 1-month | `subscription_webhook_handler.go:657,694` |
|
|
| P3-18 | LIKE wildcards not escaped | `document_repo.go:91`, `task_template_repo.go:48` |
|
|
| P3-19 | GORM v1 FOR UPDATE pattern | `subscription_repo.go:66` |
|
|
| P3-20 | Admin JWT via query parameter | `admin_auth.go:50` |
|
|
| P3-21 | WebSocket CheckOrigin always true | `monitoring/handler.go:19` |
|
|
| P3-22 | X-Request-ID not sanitized (log injection) | `request_id.go:21` |
|
|
| P3-23 | ClearAllData no secondary confirmation | `admin/settings_handler.go:529` |
|
|
| P3-24 | XSS in admin email template | `admin/notification_handler.go:351` |
|
|
| P3-25 | Admin seed SQL not parameterized | `admin/settings_handler.go:378` |
|
|
| P3-26 | UUID truncated to 8 chars | `storage_service.go:75` |
|
|
| P3-27 | Non-pointer bool IsActive defaults false on PATCH | `admin/share_code_handler.go:155` |
|
|
| P3-28 | Share code deactivation silently ignored | `residence_service.go:447` |
|
|
| P3-29 | Hardcoded debug secret key | `config.go:339` |
|
|
| P3-30 | stdlib log instead of zerolog (subscription) | `subscription_service.go:7` |
|
|
| P3-31 | stdlib log instead of zerolog (webhook) | `subscription_webhook_handler.go:11` |
|
|
| P3-32 | 9 silent failures (unchecked DB errors) | Multiple files |
|
|
| P3-33 | MediaHandler bypasses service layer | `media_handler.go:19` |
|
|
| P3-34 | OnboardingEmailService bypasses repository | `onboarding_email_service.go:17` |
|
|
| P3-35 | Webhook handler bypasses service layer | `subscription_webhook_handler.go:26` |
|
|
| P3-36 | Worker handler raw DB access | `worker/jobs/handler.go:35` |
|
|
| P3-37 | GetKanbanColumnWithTimezone duplicates chain | `models/task.go:201` |
|
|
| P3-38 | IsActive/IsPro ignore IsFree override | `models/subscription.go:57` |
|
|
| P3-39 | GetAllUsers wrong when not preloaded | `models/residence.go:65` |
|
|
| P3-40 | IsRecurring wrong when Frequency not preloaded | `predicates/predicates.go:209` |
|
|
| P3-41 | Template Save without Omit | `task_template_repo.go:79` |
|
|
| P3-42 | AddUser uses PostgreSQL-specific ON CONFLICT | `residence_repo.go:125` |
|
|
| P3-43 | Bare Save without Omit (notification prefs + subscription) | `notification_repo.go:133`, `subscription_repo.go:57` |
|
|
| P3-44 | `errors.Is` not used for ErrRecordNotFound (2 locations) | `notification_repo.go:143`, `subscription_repo.go:40` |
|
|
| P3-45 | UTF-8 byte-level truncation in PDF | `pdf_service.go:131` |
|
|
| P3-46 | Global cache singleton without sync | `cache_service.go:21` |
|
|
| P3-47 | i18n Bundle nil dereference risk | `i18n.go:16` |
|
|
| P3-48 | log.Fatal in worker goroutines | `cmd/worker/main.go:185` |
|
|
| P3-49 | WithTransaction uses mutable global db | `database.go:100` |
|
|
| P3-50 | filepath.Abs errors ignored in storage Delete | `storage_service.go:137` |
|
|
| P3-51 | Bind error silently discarded (residence_handler) | `residence_handler.go:187` |
|
|
| P3-52 | SetAsynqInspector writes without sync | `monitoring/service.go:85` |
|
|
| P3-53 | WebSocket reader doesn't stop on write errors | `monitoring/handler.go:131` |
|
|
| P3-54 | Activate/Deactivate hybrid response format | `document_handler.go:255` |
|
|
| P3-55 | nil vs empty slice inconsistency | `user_service.go`, `responses/*.go` |
|
|
| P3-56 | applyFilterOptions applies no scope on empty filter | `task_repo.go:60` |
|
|
| P3-57 | Multiple db.Exec errors unchecked for index creation | `database.go:182+` |
|