doc: [plans] Update enhancement analysis with progress tracker

Author: pparage

docs/plans/enhancement-analysis.md

@@ -0,0 +1,267 @@
+# TestingPlatform Enhancement Analysis
+
+Comprehensive audit covering security, feature accuracy, result consistency, and architecture.
+Findings are deduplicated and organized by priority.
+
+## Progress Tracker
+
+**FIXED** (16 commits on `dev`):
+- [x] #1 — Committed credentials removed, .gitignore updated
+- [x] #2 — Auth added to all InfraTesting API endpoints
+- [x] #3 + #10 — File size validation fixed (len()), validators use ValidationError
+- [x] #4 + #6 — CSRF restored on form endpoint, SameSite=Lax, CORS middleware repositioned
+- [x] #5 — SECRET_KEY auto-generated for dev, DEBUG defaults False, ZAP key emptied
+- [x] #7 — API views use serializer.validated_data consistently
+- [x] #8 — ScopedRateThrottle with per-endpoint scopes (auth/infra/expensive/admin)
+- [x] #13 — Email check reports errors instead of silent {} fallback, 60s timeout added
+- [x] #17 — IPv6 reachability uses OR (matching IPv4 logic)
+- [x] #20 — CSP report cache import added, rate limiter init fixed, error messages sanitized
+- [x] #21 — HSTS/SSL/X-Frame-Options/nosniff security headers (production-gated)
+- [x] #22 — UniqueConstraint on (user, domain) for UserDomain and MailDomain
+- [x] #26 — HSTS max-age parsing handles malformed values
+- [x] #27 — CorsMiddleware moved to top of middleware stack
+- [x] #28 — Admin URL configurable via ADMIN_URL env var
+- [x] #35 — CORS_ALLOWED_ORIGINS configurable via env var
+- [x] Bonus: Logout view broken exception handling cleaned up
+
+**REMAINING**:
+- [ ] #9 — Nmap execution timeout
+- [ ] #11 — Pandora polling (busy-wait, seed_expire=0)
+- [ ] #12 — DKIM inconsistent return types
+- [ ] #14 — DMARC upload API key in URL
+- [ ] #15 — SQLite for production
+- [ ] #16 — API always returns HTTP 200
+- [ ] #18 — DNS queries without timeout
+- [ ] #19 — TLS version check fragile nmap parsing
+- [ ] #23 — Cascade delete wipes history
+- [ ] #24 — PDF report crashes on missing keys
+- [ ] #25 — Vulnerability lookup without caching
+- [ ] #29-#34 — Architecture/tech debt (lower priority)
+
+---
+
+## CRITICAL — Fix Before Any Production Use
+
+### 1. Committed Credentials in Git History
+- **File**: `compose/staging/.env` — Contains plaintext ONEKEY API email and password
+- **Impact**: Anyone with repo access has these credentials; they persist in git history even if deleted
+- **Fix**: Remove file from tracking, rotate credentials, add to `.gitignore`
+
+### 2. Unauthenticated Infrastructure Testing API Endpoints
+- **File**: `api/views.py:270-380` — All `InfraTestingXxxApiView` classes
+- **Impact**: No `authentication_classes` or `permission_classes`. Any anonymous user can trigger nmap scans, Pandora file analysis, DNS queries, TLS checks. Enables reconnaissance-as-a-service and resource exhaustion.
+- **Fix**: Add `permission_classes = [IsAuthenticated]` at minimum; add rate limiting
+
+### 3. File Size Validation Uses Wrong Function
+- **File**: `testing/validators.py:12` — `sys.getsizeof(file)` returns Python object overhead (~50-100 bytes), NOT actual file content size
+- **Impact**: File size limit (5MB) is never enforced. Arbitrarily large files can be uploaded.
+- **Fix**: Use `file.getbuffer().nbytes` or `len(file.getvalue())` for BytesIO objects
+
+### 4. CSRF Protection Disabled on Form Endpoints
+- **File**: `testing/views.py:146` (`check_website_security`), `:733` (`dmarc_upload`), `:867` (`receive_csp_report`)
+- **Impact**: `@csrf_exempt` on POST endpoints accepting form data. Combined with `SameSite=None` cookies (settings.py:135-136), this enables cross-site request forgery attacks.
+- **Fix**: Remove `@csrf_exempt` from form endpoints; use CSRF tokens. For webhooks (CSP reports, DMARC), use token-based auth instead.
+
+### 5. Insecure Default Configuration
+- **File**: `testing_platform/settings.py:25,28`
+- **Impact**: `SECRET_KEY` defaults to literal `"secret"`, `DEBUG` defaults to `True`. While there's a startup check for production, `ZAP_API_KEY` also defaults to `"123456"` (line 271).
+- **Fix**: Remove insecure defaults; require explicit env vars; fail fast on missing critical config
+
+### 6. SameSite=None on All Cookies
+- **File**: `testing_platform/settings.py:135-139`
+- **Impact**: `CSRF_COOKIE_SAMESITE = "None"` + `SESSION_COOKIE_SAMESITE = "None"` + `CORS_ALLOW_CREDENTIALS = True` effectively disables browser-side CSRF protections. Combined with #4, this is a direct attack vector.
+- **Fix**: Change to `"Lax"` (minimum) or `"Strict"`. Only use `"None"` if there's a documented cross-origin requirement.
+
+---
+
+## HIGH — Significant Bugs and Gaps
+
+### 7. API Input Validation Bypassed
+- **File**: `api/views.py:280` (`InfraTestingEmailApiView`) — Reads `domain_name` directly from `request.data` without using serializer validation
+- **File**: `api/views.py:316` (`InfraTestingIPv6ApiView`) — Validates serializer but then reads from `request.data` anyway (ignoring validated data)
+- **File**: `api/views.py:332` (`InfraTestingSOAApiView`) — Creates a *new* serializer instance with raw domain string instead of using validated data
+- **Impact**: Unvalidated user input passed directly to DNS queries, nmap, and network operations
+- **Fix**: Use `serializer.validated_data['domain_name']` consistently after `is_valid()`
+
+### 8. No Rate Limiting Anywhere
+- **File**: `testing_platform/settings.py` — No `DEFAULT_THROTTLE_CLASSES` in `REST_FRAMEWORK` config
+- **Impact**: Expensive operations (nmap scans, Pandora submissions, DNS enumeration) can be triggered at unlimited rate. Enables DoS against both the platform and scanned targets.
+- **Fix**: Add DRF throttle classes; consider per-endpoint limits for expensive operations
+
+### 9. Nmap Execution Without Timeout
+- **File**: `testing/helpers.py:608-664` — `nmap.nmap_version_detection()` with `--script vulners` has no timeout
+- **Impact**: Scans can run indefinitely, exhausting worker threads. With no auth (#2) and no rate limits (#8), this is a DoS vector.
+- **Fix**: Add explicit timeouts; run in async task queue; implement per-user concurrency limits
+
+### 10. Validators Raise `Exception` Instead of `ValidationError`
+- **File**: `testing/validators.py:13,33,36,51` — All validation failures raise bare `Exception`
+- **Impact**: DRF cannot format these as 400 responses; they bubble up as 500 Internal Server Errors. Users see generic error pages instead of helpful validation messages.
+- **Fix**: Use `rest_framework.exceptions.ValidationError` or `django.core.exceptions.ValidationError`
+
+### 11. Pandora File Analysis Polling Issues
+- **File**: `testing/helpers.py:315-353`
+- **Issues**:
+  - Busy-wait loop with `sleep(0.1)` for up to 500 iterations (~50s) blocks the worker thread
+  - On exception, returns last (possibly stale) `analysis_result`
+  - `seed_expire=0` creates permanent public links to analysis results
+- **Impact**: Worker thread exhaustion under load; incomplete results served as final; analysis results publicly accessible
+- **Fix**: Use async task queue; implement proper status callbacks; set reasonable seed expiration
+
+### 12. DKIM Check Returns Inconsistent Types
+- **File**: `testing/helpers.py` — `check_dkim()` function
+- **Impact**: Returns `(record_text, boolean_tuple)` for single selector but `{"dkim": boolean}` for multiple selectors. Callers in `views.py` expect tuple unpacking — type mismatch causes runtime `TypeError`.
+- **Fix**: Standardize return type to always be a dict
+
+### 13. Email Check Silent Failures
+- **File**: `testing/helpers.py:239-241`
+- **Impact**: If `checkdmarc` subprocess output is invalid JSON, result is silently set to `{}`. Users see empty results with no error indication — a "clean bill of health" when the check actually failed.
+- **Fix**: Detect and report parsing failures; check stderr; validate subprocess exit code
+
+### 14. Missing DMARC Upload Authentication
+- **File**: `testing/views.py:733-750`
+- **Impact**: API key passed in URL query string (exposed in logs, referrer headers, browser history). Combined with `@csrf_exempt`, this endpoint is weakly protected.
+- **Fix**: Move API key to `Authorization` header; add CSRF protection or proper token auth
+
+### 15. SQLite in Production
+- **File**: `testing_platform/settings.py:164-169`
+- **Impact**: SQLite cannot handle concurrent writes; "database is locked" errors under load. No connection pooling, no replication, no horizontal scaling.
+- **Fix**: Configure PostgreSQL for staging/production via `DATABASE_URL` env var
+
+---
+
+## MEDIUM — Accuracy, Consistency, and Hardening
+
+### 16. API Always Returns HTTP 200
+- **File**: `api/views.py:282,318,334,349,365,380`
+- **Impact**: All check endpoints return `200 OK` regardless of whether the check succeeded or the input was invalid. API consumers cannot distinguish success from failure via status code.
+- **Fix**: Return 400 for validation errors, 422 for processing failures, 200 only for successful checks
+
+### 17. IPv6 Check Logic Inconsistency
+- **File**: `testing/helpers.py:428-431,470-473`
+- **Impact**: IPv4 reachability requires TCP *or* UDP, but IPv6 reachability requires TCP *and* UDP. This asymmetry means IPv6 is incorrectly marked unreachable when only one protocol responds.
+- **Fix**: Use consistent logic (either protocol sufficient) for both
+
+### 18. DNS Queries Without Timeout or Error Handling
+- **File**: `testing/helpers.py:392,406,442`
+- **Impact**: `dns.query.udp()` calls without timeout parameter can hang indefinitely. Only `Timeout` and `OSError` caught — misses `DNSException`, `NXDOMAIN`, connection refused.
+- **Fix**: Add explicit timeouts; catch `dns.exception.DNSException` broadly
+
+### 19. TLS Version Check Fragile Nmap Parsing
+- **File**: `testing/helpers.py:858`
+- **Impact**: Assumes exact nmap script output structure (`results[k]["ciphers"]["children"]`). Different nmap versions may produce different structures — causes `KeyError` crashes.
+- **Fix**: Validate structure before accessing nested keys; handle missing data gracefully
+
+### 20. CSP Report Endpoint Broken
+- **File**: `testing/views.py:874`
+- **Impact**: Uses `cache.get()` and `cache.incr()` but `cache` is not imported. First CSP report received crashes with `NameError`.
+- **Fix**: Add `from django.core.cache import cache` import
+
+### 21. Missing Security Headers in Django Config
+- **File**: `testing_platform/settings.py`
+- **Missing**:
+  - `SECURE_HSTS_SECONDS` (no HSTS enforcement)
+  - `SECURE_SSL_REDIRECT` (no HTTP->HTTPS redirect)
+  - `X_FRAME_OPTIONS = 'DENY'` (clickjacking, defaults to SAMEORIGIN)
+  - `SECURE_CONTENT_TYPE_NOSNIFF` not explicit
+- **Impact**: Ironic for a security testing platform to not follow its own recommendations
+- **Fix**: Add all security headers; make them environment-configurable
+
+### 22. No Database Unique Constraints on Domains
+- **File**: `testing/models.py:9-14`
+- **Impact**: `Domain` model has no `unique_together` on `(user, domain)`. Users can add the same domain multiple times. Race condition in `authentication/views.py:139-175` (TOCTOU).
+- **Fix**: Add `unique_together = [('user', 'domain')]`; handle `IntegrityError`
+
+### 23. Cascade Delete Wipes All History
+- **File**: `testing/models.py:10,37,93,113,146` — All FKs use `on_delete=CASCADE`
+- **Impact**: Deleting a user cascades to all their domains, DMARC records, CSP endpoints, CSP reports, and test history. No soft delete, no audit trail.
+- **Fix**: Use `PROTECT` or `SET_NULL` for forensic data; consider soft deletes
+
+### 24. PDF Report Generation Crashes on Missing Keys
+- **File**: `testing/views.py:780-791`
+- **Impact**: Email test PDF assumes `spf`, `dmarc`, `dnssec` keys exist in results. If any check failed, keys may be missing — causing `KeyError` during PDF generation.
+- **Fix**: Use `.get()` with defaults for all result keys
+
+### 25. Vulnerability Lookup Without Caching or Rate Limiting
+- **File**: `testing/helpers.py:678-737`
+- **Impact**: Individual CVE lookups to external API with no caching, no batch queries, no rate limiting. API downtime causes silent failures; repeated scans re-fetch the same CVEs.
+- **Fix**: Cache CVE results; implement batch lookups; handle API unavailability explicitly
+
+### 26. HSTS Parsing Crashes on Malformed Headers
+- **File**: `testing/helpers.py:1830`
+- **Impact**: `int()` conversion on `max-age` value without error handling. Malformed header like `max-age=invalid` crashes the check.
+- **Fix**: Wrap in try/except; validate before conversion
+
+### 27. CorsMiddleware Position in Middleware Stack
+- **File**: `testing_platform/settings.py:127`
+- **Impact**: `CorsMiddleware` is last in the stack but [should be placed as high as possible](https://github.com/adamchainz/django-cors-headers#setup), ideally before `CommonMiddleware`.
+- **Fix**: Move to top of `MIDDLEWARE` list
+
+### 28. Admin Exposed at Default `/admin/` Path
+- **File**: `testing_platform/urls.py:30`
+- **Impact**: Django admin at predictable URL with no additional protections (IP allowlist, 2FA)
+- **Fix**: Use obscure admin URL; add IP restrictions; require 2FA
+
+---
+
+## LOW — Technical Debt and Improvements
+
+### 29. Monolithic helpers.py (~1900 Lines, ~41 Functions)
+- Single file containing all security check logic — DNS, TLS, IPv6, email, web server, CSP, cookies, CORS, HSTS, SRI, etc.
+- Hard to test individual functions; changes risk regressions; no separation of concerns
+- **Suggestion**: Split into domain-specific modules (`checks/dns.py`, `checks/tls.py`, `checks/headers.py`, etc.)
+
+### 30. No Async Task Queue
+- All checks (nmap, Pandora, DNS) run synchronously in request/response cycle
+- Long-running scans block gunicorn worker threads
+- No task status tracking, no retry mechanism, no progress reporting
+- **Suggestion**: Integrate Celery or Django-Q for background processing
+
+### 31. Inconsistent Error Return Formats
+- Some functions return `{"error": "msg"}`, others `{"status": False}`, others `{"result": {"error": "msg"}}`
+- Views and API must handle multiple formats; no contract between helpers and consumers
+- **Suggestion**: Define a standard result envelope (`{success, data, error}`)
+
+### 32. Mixed Authentication Patterns
+- Some API views use 3 auth methods (Session + Basic + JWT), some use JWT only, some are fully public
+- No consistent pattern; no documentation of which endpoints need what
+- **Suggestion**: Define auth tiers in code and docs
+
+### 33. SRI Check Fetches External Resources Without Circuit Breaker
+- **File**: `testing/helpers.py:1547-1632`
+- Fetches every cross-origin resource; slow CDNs make the check hang
+- Uses `print()` instead of `logging`
+- **Suggestion**: Add circuit breaker pattern; use proper logging
+
+### 34. checkdmarc Dependency Not Validated at Startup
+- **File**: `testing/helpers.py:227-244`
+- Calls system binary via subprocess; no check that it's installed or correct version
+- **Suggestion**: Validate external tool availability at startup (`AppConfig.ready()`)
+
+### 35. CORS Hardcoded for localhost
+- **File**: `testing_platform/settings.py:129-132`
+- Only allows `localhost:3000` — works for dev but needs environment-based config for production
+- **Suggestion**: Make `CORS_ALLOWED_ORIGINS` configurable via env var
+
+---
+
+## Summary by Category
+
+| Category | Critical | High | Medium | Low |
+|----------|----------|------|--------|-----|
+| Authentication & Authorization | 2 | 1 | 1 | 1 |
+| Input Validation | 1 | 2 | 2 | 0 |
+| CSRF / Cookie Security | 2 | 1 | 0 | 0 |
+| Feature Accuracy | 0 | 3 | 5 | 2 |
+| Configuration & Deployment | 1 | 1 | 2 | 1 |
+| API Design | 0 | 1 | 1 | 2 |
+| Data Integrity | 0 | 0 | 2 | 1 |
+| **Total** | **6** | **9** | **13** | **7** |
+
+## Recommended Fix Order
+
+1. **Immediate**: #1 (credentials), #2 (unauth endpoints), #3 (file size), #5 (defaults)
+2. **Week 1**: #4+#6 (CSRF/cookies), #7 (validation bypass), #8 (rate limiting), #10 (validators)
+3. **Week 2**: #9 (nmap timeout), #11 (Pandora), #13 (silent failures), #14 (DMARC auth), #15 (database)
+4. **Week 3**: #16-#21 (accuracy fixes, headers, broken endpoints)
+5. **Month 1**: #22-#28 (hardening, data integrity)
+6. **Ongoing**: #29-#35 (architecture, async, tech debt)