feat: Add /api/v1 prefix and security hardening#16
Conversation
API Versioning: - Add /api/v1 prefix to all user data endpoints - Keep /health and /oauth at root level (no version prefix) Security Fixes (High Priority): - Tighten CSRF exclusions - remove /admin/sync, /admin/api-keys/, /admin/settings (HTMX already sends CSRF token) - Add callback URL validation for OAuth - require HTTPS in production, validate URL is well-formed - Add login rate limiting - lock out after 5 failed attempts for 15 min Security Fixes (Medium Priority): - Bound OAuth state dictionaries with maxsize=1000 to prevent memory exhaustion attacks - Add explicit session cookie security: secure, httponly, samesite=lax - Add SRI integrity hashes to CDN scripts (htmx, chart.js, date-fns) Documentation: - Update README API examples with /api/v1 prefix Stu Mason + AI <me@stumason.dev>
Stu Mason + AI <me@stumason.dev>
Pull Request Review: Add /api/v1 prefix and security hardeningSummaryThis PR implements significant security improvements and introduces API versioning with:
Security Improvements (Strengths)1. Login Rate Limiting (admin/routes.py:57-145)Excellent addition with IP-based tracking, proxy header support, periodic cleanup, and successful login clearing. 2. Bounded OAuth State CacheBoth BoundedTTLCache and BoundedOAuthStateCache prevent memory exhaustion with LRU eviction (max 1000 entries) and automatic TTL-based expiry (10 minutes). 3. OAuth Callback URL Validation (api/keys.py:103-133)Enforces HTTPS in production, validates URL structure, prevents open redirect vulnerabilities. 4. Session Cookie Security (app.py:101-110)Implements secure=True (HTTPS only), httponly=True (prevents XSS), samesite=lax (CSRF protection). 5. CSRF Protection Tightening (app.py:113-132)Reduced exclusion list with proper handling of external OAuth callbacks and API key auth endpoints. 6. SRI Integrity Checks (templates/base.html)Added integrity hashes for HTMX, Chart.js, and adapter to prevent supply chain attacks. Security ConcernsCRITICAL: State Validation Logic ErrorLocation: src/polar_flow_server/admin/routes.py:988-993 The current code has a bug where if pop returns None (state not found), it skips the expiry check and continues processing. This should fail immediately when state is not found. HIGH: Missing Test CoverageThe new security classes lack unit tests for LoginRateLimiter, BoundedTTLCache, BoundedOAuthStateCache, and OAuth callback validation. Security regressions could be introduced without detection. MEDIUM: IP Spoofing RiskLocation: src/polar_flow_server/admin/routes.py:483-496 The _get_client_ip function trusts X-Forwarded-For header unconditionally. If not behind a proxy, attackers can spoof this header to bypass rate limiting. Recommend adding TRUST_PROXY_HEADERS setting. MEDIUM: Weak Lockout Duration15-minute lockout after 5 attempts is relatively weak for admin login. Consider increasing to 30-60 minutes or implementing exponential backoff. Overall AssessmentStrengths: Addresses real security concerns, clean implementation, API versioning, excellent SRI checks. Concerns: Critical state validation bug, missing test coverage, IP spoofing risk, code duplication. Recommendation: REQUEST CHANGESRequired before merge:
Recommended (can be follow-up PR):
Great work on the security hardening! The approach is sound and addresses real vulnerabilities. With the bug fix and test coverage, this will be a strong addition. |
Pull Request ReviewThis PR introduces API versioning with 🔴 Critical Issues1. IP Spoofing Vulnerability in Rate Limiting (src/polar_flow_server/admin/routes.py:483-496)The def _get_client_ip(request: Request[Any, Any, Any]) -> str:
forwarded_for = request.headers.get("x-forwarded-for")
if forwarded_for:
return forwarded_for.split(",")[0].strip() # ⚠️ VULNERABLEAttack scenario:
Fix: Only trust proxy headers when behind a known proxy. Add configuration to specify trusted proxy IPs and validate: # Only trust X-Forwarded-For if request comes from trusted proxy
TRUSTED_PROXIES = {"127.0.0.1", "::1"} # Add nginx/Coolify IPs
if request.client and request.client.host in TRUSTED_PROXIES:
# Then trust X-Forwarded-ForOr use Litestar's built-in client IP handling with configured trusted proxies. 2. Missing CSRF Validation on Critical Admin Endpoints (src/polar_flow_server/app.py:118-131)The CSRF exclusion list is too broad: exclude=[
"/admin/settings", # ⚠️ Should require CSRF - modifies OAuth credentials
"/admin/sync", # ⚠️ Should require CSRF - triggers data sync
"/admin/api-keys/", # ⚠️ Should require CSRF - regenerates/revokes keys
]Attack scenario:
Fix: Remove these from CSRF exclusions. The PR description says "HTMX already sends CSRF token" - if that's true, these endpoints should work fine with CSRF enabled. 3. Callback URL Validation Bypass (src/polar_flow_server/api/keys.py:104-113)The callback URL validation allows is_debug = settings.log_level == "DEBUG"
if parsed.scheme == "http":
netloc_lower = parsed.netloc.lower()
if not (netloc_lower.startswith("localhost") or ...):
if not is_debug: # ⚠️ This check is too late
return False, "HTTP only allowed for localhost"Issues:
Fix: # Use explicit environment flag
is_production = settings.deployment_mode == DeploymentMode.SAAS or settings.base_url is not None
if parsed.scheme == "http":
# Only allow http for actual localhost in dev
if is_production:
return False, "HTTPS required in production"
if not (netloc_lower == "localhost" or netloc_lower.startswith("localhost:")
or netloc_lower == "127.0.0.1" or netloc_lower.startswith("127.0.0.1:")):
return False, "HTTP only allowed for localhost"🟡 High Priority Issues4. Race Conditions in Rate Limiter Cleanup (src/polar_flow_server/admin/routes.py:116-132)The cleanup logic modifies dictionaries during iteration: for ip in list(self._attempts.keys()):
self._attempts[ip] = [t for t in self._attempts[ip] if t > cutoff]
if not self._attempts[ip]:
del self._attempts[ip] # ⚠️ Modifying during iteration (even with list())While Fix: Use dictionary comprehension for atomic updates: self._attempts = {
ip: [t for t in attempts if t > cutoff]
for ip, attempts in self._attempts.items()
if any(t > cutoff for t in attempts)
}5. Missing Rate Limiter Synchronization (src/polar_flow_server/admin/routes.py:188)_login_rate_limiter = LoginRateLimiter(max_attempts=5, lockout_minutes=15)This global instance is accessed by multiple async requests concurrently without any locking. While individual dictionary operations are atomic due to GIL, compound operations like "check + increment" are not thread-safe. Attack scenario:
Fix: Add async lock for critical sections: import asyncio
class LoginRateLimiter:
def __init__(self, ...):
self._lock = asyncio.Lock()
async def record_failure(self, ip: str) -> bool:
async with self._lock:
# existing logicSame issue exists in 6. SRI Hash Rotation Risk (src/polar_flow_server/templates/base.html:7-31)The PR adds SRI integrity hashes for CDN scripts: <script src="https://unpkg.com/htmx.org@2.0.4"
integrity="sha384-HGfztofotfshcF7+8n44JQL2oJmowVChPTg48S+jvZoztPfvwD79OC/LTtG6dMp+"
crossorigin="anonymous"></script>Issue: If the CDN silently updates the file at that URL (e.g., for a security patch), the SRI hash will break and the app will stop working. Unpkg and jsDelivr sometimes update patch versions. Recommendations:
7. OAuth State Cache Size Still Vulnerable (src/polar_flow_server/admin/routes.py:42-50, keys.py:43-59)Both caches set _oauth_states = BoundedTTLCache(maxsize=1000, ttl_minutes=10)
_saas_oauth_states = BoundedOAuthStateCache(maxsize=1000, ttl_minutes=10)Issue: 1000 concurrent OAuth flows is unrealistic for most deployments. An attacker can still fill the cache with 1000 fake states, evicting legitimate OAuth flows. Fix:
🟢 Medium Priority Issues8. Missing Input Validation (src/polar_flow_server/api/keys.py:211)The Risk: Attacker could send extremely long URLs to cause memory issues or logging DoS. Fix: Add validation: if len(callback_url) > 2048:
raise ValidationException("callback_url too long")
if client_id and len(client_id) > 255:
raise ValidationException("client_id too long")9. Error Message Information Disclosure (src/polar_flow_server/admin/routes.py:510-514)The rate limiting error reveals the exact lockout duration: "error": "Too many failed attempts. Please try again in 15 minutes."Minor issue: This helps attackers time their attacks. Consider generic message: "Too many failed attempts. Please try again later." 10. Missing Security HeadersThe app doesn't set security headers like:
Recommendation: Add Litestar middleware for security headers, especially important given the admin dashboard serves HTML. 📝 Code Quality & Style11. Inconsistent Error Handling (src/polar_flow_server/api/keys.py:272-282)OAuth callback tries to redirect with error, but has multiple code paths that could fail silently: if oauth_state and oauth_state in _saas_oauth_states:
state_data = _saas_oauth_states.pop(oauth_state)
if state_data: # ⚠️ Could be None after pop
callback_url = state_data["callback_url"]
# ...redirect
# Falls through to raise exception if state_data is NoneStyle issue: Fix: Simplify: if oauth_state in _saas_oauth_states:
state_data = _saas_oauth_states.pop(oauth_state)
callback_url = state_data["callback_url"] # Safe, we know it exists12. Missing Tests for Security FeaturesNone of the new security classes have tests:
Critical gaps:
Recommendation: Add comprehensive tests for all security-critical code paths. 13. Magic Numbers (src/polar_flow_server/admin/routes.py:188)_login_rate_limiter = LoginRateLimiter(max_attempts=5, lockout_minutes=15)These should be configurable via settings, not hardcoded. ✅ Positive Aspects
📋 Summary & Recommendation
Overall Assessment: This PR makes significant security improvements, but the IP spoofing vulnerability and missing CSRF protection are critical issues that must be fixed before merge. The rate limiter synchronization issues could also lead to bypasses under load. Must Fix Before Merge:
Recommended for Follow-up PR:
Great work on the security hardening! The approach is solid - just needs these fixes to be production-ready. 🔒 |
Security fixes: - Fix IP spoofing: only trust proxy headers from localhost (reverse proxy) - Fix callback URL validation: explicit production check, strict localhost - Add async locks to rate limiter and OAuth caches (thread safety) - Fix race conditions: use dict comprehension for atomic cleanup - Lower OAuth state cache to 100 entries, log when full - Add client_id length validation (max 255 chars) - Simplify OAuth callback error handling The CSRF exclusion concern in the review was a misread - we already removed /admin/sync, /admin/api-keys/, /admin/settings from exclusions. Stu Mason + AI <me@stumason.dev>
Pull Request Review: Security Hardening & API VersioningSummaryThis PR adds significant security improvements and introduces API versioning with the
File-by-File Analysis✅ src/polar_flow_server/admin/routes.py (lines 58-150)Changes:
Security - EXCELLENT:
Bugs: None identified Style: ✅ Excellent documentation, clear separation of concerns Improvements:
✅ src/polar_flow_server/admin/routes.py (lines 491-568)Changes:
Security - EXCELLENT:
Bugs: None Style: ✅ Clean integration with existing flow ✅ src/polar_flow_server/admin/routes.py (lines 964-1019)Changes:
Security - EXCELLENT:
Bugs: None Style: ✅ Much cleaner than manual dict management ✅ src/polar_flow_server/api/keys.py (lines 37-147)Changes:
Security - OUTSTANDING:
Critical Security Note: This is clever but could be more explicit. Consider: # More explicit production check
is_production = settings.deployment_mode == DeploymentMode.SAAS or (
settings.base_url is not None and not settings.base_url.startswith("http://localhost")
)Bugs: None identified Style: ✅ Excellent defensive coding ✅ src/polar_flow_server/api/keys.py (lines 231-309)Changes:
Security - EXCELLENT:
Bugs: None Style: ✅ Clean error handling flow ✅ src/polar_flow_server/app.py (lines 101-156)Changes:
Security - EXCELLENT:
Potential Issue: Bugs: None Style: ✅ Excellent comments, clear reasoning ✅ src/polar_flow_server/api/init.pyChanges:
Security: ✅ Clean separation of versioned vs unversioned endpoints Design:
Bugs: None Style: ✅ Excellent comments explaining routing structure Improvement: Consider adding a root redirect or API docs at @get("/api")
async def api_index() -> dict[str, Any]:
return {"versions": ["v1"], "current": "v1", "docs": "/api/v1/docs"}✅ src/polar_flow_server/templates/base.htmlChanges:
Security - EXCELLENT:
Verification Needed: # Verify HTMX
curl -s https://unpkg.com/htmx.org@2.0.4 | openssl dgst -sha384 -binary | openssl base64 -A
# Verify Chart.js
curl -s https://cdn.jsdelivr.net/npm/chart.js@4.4.0 | openssl dgst -sha384 -binary | openssl base64 -A
# Verify chartjs-adapter
curl -s https://cdn.jsdelivr.net/npm/chartjs-adapter-date-fns@3.0.0 | openssl dgst -sha384 -binary | openssl base64 -ABugs: None (assuming hashes are correct) Style: ✅ Clean, well-commented ✅ README.mdChanges:
Bugs: None Style: ✅ Consistent, clear Overall AssessmentSecurity Improvements: 🟢 OUTSTANDING
Code Quality: 🟢 EXCELLENT
Breaking Changes:
|
SRI Hash Verification ✅All three SRI hashes have been verified: Verification command used: curl -sL "https://unpkg.com/htmx.org@2.0.4" | openssl dgst -sha384 -binary | openssl base64 -AReady to merge! 🚀 |
Summary
/api/v1prefix to all user data endpoints (breaking change for API consumers)Changes
API Versioning
/api/v1/users/{user_id}/.../healthand/oauth/*remain at root level (no version prefix)Security Fixes (High Priority)
/admin/sync,/admin/api-keys/,/admin/settingsnow require CSRF token (HTMX already sends it)Security Fixes (Medium Priority)
secure,httponly,samesite=laxflagsTest plan
uv run pytest- 74 tests passuv run ruff check src/- All checks passeduv run mypy src/polar_flow_server- No errors/api/v1prefixStu Mason + AI me@stumason.dev