Complete security hardening implementation including HTTPS enforcement, security headers, rate limiting, and comprehensive security test suite. Key features: - HTTPS enforcement with HSTS support - Security headers (CSP, X-Frame-Options, X-Content-Type-Options) - Rate limiting for all critical endpoints - Enhanced email template security - 87% test coverage with security-specific tests Architect approval: 9.5/10 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
1812 lines
62 KiB
Markdown
1812 lines
62 KiB
Markdown
# Phase 4b: Security Hardening
|
||
|
||
**Architect**: Claude (Architect Agent)
|
||
**Date**: 2025-11-20
|
||
**Status**: Design Complete - Clarifications Provided
|
||
**Phase**: 4b (Security Hardening)
|
||
**Design References**:
|
||
- `/docs/reports/2025-11-20-gap-analysis-v1.0.0.md` - Gap analysis Components 4-5
|
||
- `/docs/designs/phase-4-5-critical-components.md` - Phase 4-5 overview
|
||
- `/docs/architecture/security.md` - Security architecture
|
||
- `/docs/roadmap/v1.0.0.md` - Phase 4 requirements (lines 189-218)
|
||
- `/docs/designs/phase-4b-clarifications.md` - Implementation clarifications (2025-11-20)
|
||
|
||
---
|
||
|
||
## Purpose
|
||
|
||
Implement production-grade security hardening to protect against common web vulnerabilities and ensure compliance with OAuth 2.0 and IndieAuth security best practices. This phase adds the final security layer required for v1.0.0 production readiness.
|
||
|
||
**Critical Requirements**:
|
||
1. Security headers middleware (HSTS, CSP, X-Frame-Options, etc.)
|
||
2. HTTPS enforcement in production mode
|
||
3. Comprehensive security test suite
|
||
4. PII logging audit and remediation
|
||
5. Input validation verification
|
||
|
||
**Success Outcome**: Application is production-ready with defense-in-depth security measures, passing all security tests, and compliant with OWASP Top 10 recommendations.
|
||
|
||
---
|
||
|
||
## Specification References
|
||
|
||
**W3C IndieAuth**:
|
||
- Security Considerations section (HTTPS requirement)
|
||
- Token security (hashing, constant-time comparison)
|
||
|
||
**OAuth 2.0 Security Best Practices** (RFC 8252):
|
||
- HTTPS enforcement for non-localhost
|
||
- State parameter CSRF protection
|
||
- Open redirect prevention
|
||
- Token theft mitigation
|
||
|
||
**OWASP Top 10**:
|
||
- A01:2021 Broken Access Control (token validation)
|
||
- A03:2021 Injection (SQL injection, XSS)
|
||
- A05:2021 Security Misconfiguration (headers, HTTPS)
|
||
- A07:2021 Identification and Authentication Failures (timing attacks)
|
||
|
||
**v1.0.0 Roadmap Requirements**:
|
||
- Phase 4, lines 198-203: HTTPS enforcement, security headers, constant-time comparison, input sanitization, PII logging audit
|
||
- Exit criteria, lines 212-218: All security tests passing, no PII in logs
|
||
|
||
---
|
||
|
||
## Design Overview
|
||
|
||
Phase 4b implements four major components:
|
||
|
||
1. **Security Headers Middleware**: FastAPI middleware that adds security-related HTTP headers to all responses
|
||
2. **HTTPS Enforcement**: Production-mode middleware that redirects HTTP → HTTPS and validates TLS usage
|
||
3. **Security Test Suite**: Comprehensive tests covering timing attacks, injection prevention, XSS, open redirects, CSRF
|
||
4. **PII Logging Audit**: Review all logging statements and ensure no personally identifiable information is logged
|
||
|
||
**Architecture Pattern**: Defense-in-depth with multiple layers:
|
||
- Layer 1: Network (HTTPS/TLS)
|
||
- Layer 2: HTTP headers (prevent common attacks)
|
||
- Layer 3: Input validation (already implemented via Pydantic)
|
||
- Layer 4: Token security (hashing, constant-time comparison)
|
||
- Layer 5: Logging (no sensitive data exposure)
|
||
|
||
**Implementation Approach**:
|
||
- Middleware-based for headers and HTTPS enforcement (FastAPI middleware pattern)
|
||
- Pytest-based security tests using standard pytest markers
|
||
- Logging audit via grep and manual code review
|
||
- Configuration-driven (production vs development mode)
|
||
|
||
---
|
||
|
||
## Component 4: Security Headers Middleware
|
||
|
||
### Purpose
|
||
|
||
Add HTTP security headers to all responses to protect against clickjacking, XSS, MIME sniffing, and other client-side attacks. Headers are the first line of defense for browser-based vulnerabilities.
|
||
|
||
### Security Headers to Implement
|
||
|
||
| Header | Value | Purpose | Standard |
|
||
|--------|-------|---------|----------|
|
||
| `X-Frame-Options` | `DENY` | Prevent clickjacking attacks | RFC 7034 |
|
||
| `X-Content-Type-Options` | `nosniff` | Prevent MIME type sniffing | WHATWG |
|
||
| `X-XSS-Protection` | `1; mode=block` | Enable XSS filter (legacy browsers) | Non-standard but widely supported |
|
||
| `Strict-Transport-Security` | `max-age=31536000; includeSubDomains` | Force HTTPS for 1 year (production only) | RFC 6797 |
|
||
| `Content-Security-Policy` | `default-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' https:; frame-ancestors 'none'` | Restrict resource loading | W3C CSP Level 3 |
|
||
| `Referrer-Policy` | `strict-origin-when-cross-origin` | Control referrer information leakage | W3C Referrer Policy |
|
||
| `Permissions-Policy` | `geolocation=(), microphone=(), camera=()` | Disable unnecessary browser features | W3C Permissions Policy |
|
||
|
||
**Rationale for Values**:
|
||
|
||
- **X-Frame-Options: DENY**: IndieAuth server should never be embedded in frames (phishing risk)
|
||
- **X-Content-Type-Options: nosniff**: Force browser to respect Content-Type header (prevent XSS via misinterpreted files)
|
||
- **X-XSS-Protection: 1; mode=block**: Enable legacy XSS filter for older browsers (modern browsers have CSP)
|
||
- **Strict-Transport-Security**: Enforce HTTPS for 1 year including subdomains (production only, not development)
|
||
- **Content-Security-Policy**:
|
||
- `default-src 'self'`: Only load resources from same origin
|
||
- `style-src 'self' 'unsafe-inline'`: Allow inline styles for simplicity (templates may use inline styles)
|
||
- `img-src 'self' https:`: Allow images from same origin + HTTPS (for client logos from h-app)
|
||
- `frame-ancestors 'none'`: Equivalent to X-Frame-Options DENY
|
||
- **Referrer-Policy: strict-origin-when-cross-origin**: Send origin on cross-origin requests (not full URL, privacy)
|
||
- **Permissions-Policy**: Disable geolocation, microphone, camera (not needed for auth server)
|
||
|
||
### Middleware Design
|
||
|
||
**File**: `/src/gondulf/middleware/security_headers.py`
|
||
|
||
**Implementation**:
|
||
|
||
```python
|
||
"""Security headers middleware for Gondulf IndieAuth server."""
|
||
|
||
import logging
|
||
from typing import Callable
|
||
|
||
from fastapi import Request, Response
|
||
from starlette.middleware.base import BaseHTTPMiddleware
|
||
|
||
from gondulf.config import Config
|
||
|
||
logger = logging.getLogger("gondulf.middleware.security_headers")
|
||
|
||
|
||
class SecurityHeadersMiddleware(BaseHTTPMiddleware):
|
||
"""
|
||
Add security-related HTTP headers to all responses.
|
||
|
||
Headers protect against clickjacking, XSS, MIME sniffing, and other
|
||
client-side attacks. HSTS is only added in production mode (non-DEBUG).
|
||
|
||
References:
|
||
- OWASP Secure Headers Project
|
||
- Mozilla Web Security Guidelines
|
||
"""
|
||
|
||
def __init__(self, app, debug: bool = False):
|
||
"""
|
||
Initialize security headers middleware.
|
||
|
||
Args:
|
||
app: FastAPI application
|
||
debug: If True, skip HSTS header (development mode)
|
||
"""
|
||
super().__init__(app)
|
||
self.debug = debug
|
||
|
||
async def dispatch(self, request: Request, call_next: Callable) -> Response:
|
||
"""
|
||
Process request and add security headers to response.
|
||
|
||
Args:
|
||
request: Incoming HTTP request
|
||
call_next: Next middleware/handler in chain
|
||
|
||
Returns:
|
||
Response with security headers added
|
||
"""
|
||
# Process request
|
||
response = await call_next(request)
|
||
|
||
# Add security headers
|
||
response.headers["X-Frame-Options"] = "DENY"
|
||
response.headers["X-Content-Type-Options"] = "nosniff"
|
||
response.headers["X-XSS-Protection"] = "1; mode=block"
|
||
response.headers["Referrer-Policy"] = "strict-origin-when-cross-origin"
|
||
|
||
# CSP: Allow self, inline styles (for templates), and HTTPS images (for h-app logos)
|
||
response.headers["Content-Security-Policy"] = (
|
||
"default-src 'self'; "
|
||
"style-src 'self' 'unsafe-inline'; "
|
||
"img-src 'self' https:; "
|
||
"frame-ancestors 'none'"
|
||
)
|
||
|
||
# Permissions Policy: Disable unnecessary browser features
|
||
response.headers["Permissions-Policy"] = (
|
||
"geolocation=(), microphone=(), camera=()"
|
||
)
|
||
|
||
# HSTS: Only in production (not development)
|
||
if not self.debug:
|
||
response.headers["Strict-Transport-Security"] = (
|
||
"max-age=31536000; includeSubDomains"
|
||
)
|
||
logger.debug("Added HSTS header (production mode)")
|
||
|
||
return response
|
||
```
|
||
|
||
### Configuration Integration
|
||
|
||
**Update**: `/src/gondulf/main.py`
|
||
|
||
Add middleware registration after app initialization:
|
||
|
||
```python
|
||
from gondulf.middleware.security_headers import SecurityHeadersMiddleware
|
||
|
||
# ... existing imports and setup ...
|
||
|
||
# Add security headers middleware
|
||
app.add_middleware(
|
||
SecurityHeadersMiddleware,
|
||
debug=Config.DEBUG
|
||
)
|
||
logger.info(f"Security headers middleware registered (debug={Config.DEBUG})")
|
||
```
|
||
|
||
**Middleware Order**: Security headers should be added early in the middleware chain, but after any error-handling middleware. FastAPI applies middleware in reverse order of registration, so register security headers early.
|
||
|
||
### Testing Requirements
|
||
|
||
**File**: `/tests/integration/test_security_headers.py`
|
||
|
||
```python
|
||
"""Integration tests for security headers middleware."""
|
||
|
||
import pytest
|
||
from fastapi.testclient import TestClient
|
||
|
||
from gondulf.main import app
|
||
|
||
|
||
@pytest.fixture
|
||
def client():
|
||
"""FastAPI test client."""
|
||
return TestClient(app)
|
||
|
||
|
||
class TestSecurityHeaders:
|
||
"""Test security headers middleware."""
|
||
|
||
def test_x_frame_options_header(self, client):
|
||
"""Test X-Frame-Options header is present."""
|
||
response = client.get("/health")
|
||
assert "X-Frame-Options" in response.headers
|
||
assert response.headers["X-Frame-Options"] == "DENY"
|
||
|
||
def test_x_content_type_options_header(self, client):
|
||
"""Test X-Content-Type-Options header is present."""
|
||
response = client.get("/health")
|
||
assert "X-Content-Type-Options" in response.headers
|
||
assert response.headers["X-Content-Type-Options"] == "nosniff"
|
||
|
||
def test_x_xss_protection_header(self, client):
|
||
"""Test X-XSS-Protection header is present."""
|
||
response = client.get("/health")
|
||
assert "X-XSS-Protection" in response.headers
|
||
assert response.headers["X-XSS-Protection"] == "1; mode=block"
|
||
|
||
def test_csp_header(self, client):
|
||
"""Test Content-Security-Policy header is present and configured correctly."""
|
||
response = client.get("/health")
|
||
assert "Content-Security-Policy" in response.headers
|
||
|
||
csp = response.headers["Content-Security-Policy"]
|
||
assert "default-src 'self'" in csp
|
||
assert "style-src 'self' 'unsafe-inline'" in csp
|
||
assert "img-src 'self' https:" in csp
|
||
assert "frame-ancestors 'none'" in csp
|
||
|
||
def test_referrer_policy_header(self, client):
|
||
"""Test Referrer-Policy header is present."""
|
||
response = client.get("/health")
|
||
assert "Referrer-Policy" in response.headers
|
||
assert response.headers["Referrer-Policy"] == "strict-origin-when-cross-origin"
|
||
|
||
def test_permissions_policy_header(self, client):
|
||
"""Test Permissions-Policy header is present."""
|
||
response = client.get("/health")
|
||
assert "Permissions-Policy" in response.headers
|
||
|
||
policy = response.headers["Permissions-Policy"]
|
||
assert "geolocation=()" in policy
|
||
assert "microphone=()" in policy
|
||
assert "camera=()" in policy
|
||
|
||
def test_hsts_header_not_in_debug_mode(self, client, monkeypatch):
|
||
"""Test HSTS header is NOT present in debug mode."""
|
||
# This test assumes DEBUG=True in test environment
|
||
# In production, DEBUG=False and HSTS should be present
|
||
response = client.get("/health")
|
||
|
||
# Check current mode from Config
|
||
from gondulf.config import Config
|
||
if Config.DEBUG:
|
||
# HSTS should NOT be present in debug mode
|
||
assert "Strict-Transport-Security" not in response.headers
|
||
else:
|
||
# HSTS should be present in production mode
|
||
assert "Strict-Transport-Security" in response.headers
|
||
assert "max-age=31536000" in response.headers["Strict-Transport-Security"]
|
||
assert "includeSubDomains" in response.headers["Strict-Transport-Security"]
|
||
|
||
def test_headers_on_all_endpoints(self, client):
|
||
"""Test security headers are present on all endpoints."""
|
||
endpoints = [
|
||
"/",
|
||
"/health",
|
||
"/.well-known/oauth-authorization-server",
|
||
]
|
||
|
||
for endpoint in endpoints:
|
||
response = client.get(endpoint)
|
||
# All endpoints should have security headers
|
||
assert "X-Frame-Options" in response.headers
|
||
assert "X-Content-Type-Options" in response.headers
|
||
assert "Content-Security-Policy" in response.headers
|
||
|
||
def test_headers_on_error_responses(self, client):
|
||
"""Test security headers are present even on error responses."""
|
||
# Request non-existent endpoint (404)
|
||
response = client.get("/nonexistent")
|
||
assert response.status_code == 404
|
||
|
||
# Security headers should still be present
|
||
assert "X-Frame-Options" in response.headers
|
||
assert "X-Content-Type-Options" in response.headers
|
||
```
|
||
|
||
**Test Coverage**:
|
||
- Each security header individually
|
||
- Headers present on all endpoints
|
||
- HSTS conditional on production mode
|
||
- Headers present on error responses
|
||
|
||
---
|
||
|
||
## Component 5: HTTPS Enforcement
|
||
|
||
### Purpose
|
||
|
||
Ensure all production traffic uses HTTPS (TLS) to prevent credential theft, token interception, and man-in-the-middle attacks. Allow HTTP only for localhost development.
|
||
|
||
### Design Approach
|
||
|
||
**Strategy**: Configuration-driven enforcement based on `Config.DEBUG` and `Config.BASE_URL`:
|
||
|
||
- **Production Mode** (`DEBUG=False`): Reject HTTP requests or redirect to HTTPS
|
||
- **Development Mode** (`DEBUG=True`): Allow HTTP for localhost only
|
||
- **Validation**: Check `request.url.scheme` and `request.url.hostname`
|
||
|
||
**Implementation**: Middleware that runs before request processing.
|
||
|
||
### Middleware Design
|
||
|
||
**File**: `/src/gondulf/middleware/https_enforcement.py`
|
||
|
||
```python
|
||
"""HTTPS enforcement middleware for Gondulf IndieAuth server."""
|
||
|
||
import logging
|
||
from typing import Callable
|
||
|
||
from fastapi import Request, Response
|
||
from starlette.middleware.base import BaseHTTPMiddleware
|
||
from starlette.responses import RedirectResponse
|
||
|
||
from gondulf.config import Config
|
||
|
||
logger = logging.getLogger("gondulf.middleware.https_enforcement")
|
||
|
||
|
||
def is_https_request(request: Request) -> bool:
|
||
"""
|
||
Check if request is HTTPS, considering reverse proxy headers.
|
||
|
||
Args:
|
||
request: Incoming HTTP request
|
||
|
||
Returns:
|
||
True if HTTPS, False otherwise
|
||
"""
|
||
# Direct HTTPS
|
||
if request.url.scheme == "https":
|
||
return True
|
||
|
||
# Behind proxy - check forwarded header
|
||
# Only trust this header in production with TRUST_PROXY=true
|
||
if Config.TRUST_PROXY:
|
||
forwarded_proto = request.headers.get("X-Forwarded-Proto", "").lower()
|
||
return forwarded_proto == "https"
|
||
|
||
return False
|
||
|
||
|
||
class HTTPSEnforcementMiddleware(BaseHTTPMiddleware):
|
||
"""
|
||
Enforce HTTPS in production mode.
|
||
|
||
In production (DEBUG=False), reject or redirect HTTP requests to HTTPS.
|
||
In development (DEBUG=True), allow HTTP for localhost only.
|
||
|
||
Supports reverse proxy deployments via X-Forwarded-Proto header when
|
||
Config.TRUST_PROXY is enabled.
|
||
|
||
References:
|
||
- OAuth 2.0 Security Best Practices: HTTPS required
|
||
- W3C IndieAuth: TLS required for production
|
||
- Clarifications: See /docs/designs/phase-4b-clarifications.md section 2
|
||
"""
|
||
|
||
def __init__(self, app, debug: bool = False, redirect: bool = True):
|
||
"""
|
||
Initialize HTTPS enforcement middleware.
|
||
|
||
Args:
|
||
app: FastAPI application
|
||
debug: If True, allow HTTP for localhost (development mode)
|
||
redirect: If True, redirect HTTP to HTTPS. If False, return 400.
|
||
"""
|
||
super().__init__(app)
|
||
self.debug = debug
|
||
self.redirect = redirect
|
||
|
||
async def dispatch(self, request: Request, call_next: Callable) -> Response:
|
||
"""
|
||
Process request and enforce HTTPS if in production mode.
|
||
|
||
Args:
|
||
request: Incoming HTTP request
|
||
call_next: Next middleware/handler in chain
|
||
|
||
Returns:
|
||
Response (redirect to HTTPS, error, or normal response)
|
||
"""
|
||
hostname = request.url.hostname or ""
|
||
|
||
# Debug mode: Allow HTTP for localhost only
|
||
if self.debug:
|
||
if not is_https_request(request) and hostname not in ["localhost", "127.0.0.1", "::1"]:
|
||
logger.warning(
|
||
f"HTTP request to non-localhost in debug mode: {hostname}"
|
||
)
|
||
# Allow but log warning (for development on local networks)
|
||
|
||
# Continue processing
|
||
return await call_next(request)
|
||
|
||
# Production mode: Enforce HTTPS
|
||
if not is_https_request(request):
|
||
logger.warning(
|
||
f"HTTP request blocked in production mode: "
|
||
f"{request.method} {request.url}"
|
||
)
|
||
|
||
if self.redirect:
|
||
# Redirect HTTP → HTTPS
|
||
https_url = request.url.replace(scheme="https")
|
||
logger.info(f"Redirecting to HTTPS: {https_url}")
|
||
return RedirectResponse(url=str(https_url), status_code=301)
|
||
else:
|
||
# Return 400 Bad Request (strict mode)
|
||
from fastapi.responses import JSONResponse
|
||
return JSONResponse(
|
||
status_code=400,
|
||
content={
|
||
"error": "invalid_request",
|
||
"error_description": "HTTPS is required"
|
||
}
|
||
)
|
||
|
||
# HTTPS or allowed HTTP: Continue processing
|
||
return await call_next(request)
|
||
```
|
||
|
||
### Configuration Options
|
||
|
||
**Update `/src/gondulf/config.py`** to add security configuration:
|
||
|
||
```python
|
||
class Config:
|
||
"""Application configuration."""
|
||
|
||
# Existing configuration...
|
||
|
||
# Security configuration (Phase 4b)
|
||
HTTPS_REDIRECT: bool = Field(
|
||
default=True,
|
||
description="Redirect HTTP requests to HTTPS in production"
|
||
)
|
||
|
||
TRUST_PROXY: bool = Field(
|
||
default=False,
|
||
description="Trust X-Forwarded-* headers from reverse proxy"
|
||
)
|
||
|
||
SECURE_COOKIES: bool = Field(
|
||
default=True,
|
||
description="Set secure flag on cookies (requires HTTPS)"
|
||
)
|
||
|
||
@validator("HTTPS_REDIRECT")
|
||
def validate_https_redirect(cls, v, values):
|
||
"""Disable HTTPS redirect in development."""
|
||
if values.get("ENV") == "development":
|
||
return False
|
||
return v
|
||
```
|
||
|
||
**Environment Variables**:
|
||
- `GONDULF_HTTPS_REDIRECT=true` - Enable HTTPS redirect (default: true)
|
||
- `GONDULF_TRUST_PROXY=true` - Trust X-Forwarded-Proto header (default: false)
|
||
- `GONDULF_SECURE_COOKIES=true` - Set secure flag on cookies (default: true)
|
||
|
||
**Clarification**: See `/docs/designs/phase-4b-clarifications.md` section 2 for reverse proxy support details.
|
||
|
||
**Integration**: Add to `main.py`:
|
||
|
||
```python
|
||
from gondulf.middleware.https_enforcement import HTTPSEnforcementMiddleware
|
||
|
||
# Add HTTPS enforcement middleware (before security headers)
|
||
app.add_middleware(
|
||
HTTPSEnforcementMiddleware,
|
||
debug=Config.DEBUG,
|
||
redirect=Config.HTTPS_REDIRECT
|
||
)
|
||
logger.info(f"HTTPS enforcement middleware registered (debug={Config.DEBUG})")
|
||
```
|
||
|
||
**Middleware Order**:
|
||
1. HTTPS enforcement (first, redirects before processing)
|
||
2. Security headers (second, adds headers to all responses)
|
||
3. Application routers (last)
|
||
|
||
### Testing Requirements
|
||
|
||
**File**: `/tests/integration/test_https_enforcement.py`
|
||
|
||
```python
|
||
"""Integration tests for HTTPS enforcement middleware."""
|
||
|
||
import pytest
|
||
from fastapi.testclient import TestClient
|
||
|
||
from gondulf.main import app
|
||
|
||
|
||
@pytest.fixture
|
||
def client():
|
||
"""FastAPI test client."""
|
||
return TestClient(app)
|
||
|
||
|
||
class TestHTTPSEnforcement:
|
||
"""Test HTTPS enforcement middleware."""
|
||
|
||
def test_https_allowed_in_production(self, client, monkeypatch):
|
||
"""Test HTTPS requests are allowed in production mode."""
|
||
# Simulate production mode
|
||
from gondulf.config import Config
|
||
monkeypatch.setattr(Config, "DEBUG", False)
|
||
|
||
# HTTPS request should succeed
|
||
# Note: TestClient uses http by default, so this test is illustrative
|
||
# In real production, requests come from a reverse proxy (nginx) with HTTPS
|
||
response = client.get("/health")
|
||
assert response.status_code == 200
|
||
|
||
def test_http_localhost_allowed_in_debug(self, client, monkeypatch):
|
||
"""Test HTTP to localhost is allowed in debug mode."""
|
||
from gondulf.config import Config
|
||
monkeypatch.setattr(Config, "DEBUG", True)
|
||
|
||
# HTTP to localhost should succeed in debug mode
|
||
response = client.get("http://localhost:8000/health")
|
||
assert response.status_code == 200
|
||
|
||
def test_https_always_allowed(self, client):
|
||
"""Test HTTPS requests are always allowed regardless of mode."""
|
||
# HTTPS should work in both debug and production
|
||
response = client.get("/health")
|
||
# TestClient doesn't enforce HTTPS, but middleware should allow it
|
||
assert response.status_code == 200
|
||
```
|
||
|
||
**Note**: Full HTTPS testing requires integration tests with a real server (uvicorn with TLS). The TestClient doesn't enforce scheme validation, so these tests are illustrative. Real-world testing should be done in Phase 5 deployment testing.
|
||
|
||
### Production Deployment Notes
|
||
|
||
**Reverse Proxy Configuration** (nginx example):
|
||
|
||
```nginx
|
||
server {
|
||
listen 80;
|
||
server_name auth.example.com;
|
||
|
||
# Redirect all HTTP to HTTPS
|
||
return 301 https://$server_name$request_uri;
|
||
}
|
||
|
||
server {
|
||
listen 443 ssl http2;
|
||
server_name auth.example.com;
|
||
|
||
ssl_certificate /etc/letsencrypt/live/auth.example.com/fullchain.pem;
|
||
ssl_certificate_key /etc/letsencrypt/live/auth.example.com/privkey.pem;
|
||
|
||
# Proxy to Gondulf
|
||
location / {
|
||
proxy_pass http://localhost:8000;
|
||
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
|
||
proxy_set_header X-Forwarded-Proto $scheme;
|
||
proxy_set_header Host $host;
|
||
}
|
||
}
|
||
```
|
||
|
||
**Important**: The reverse proxy handles TLS termination. Gondulf sees HTTP from nginx but with `X-Forwarded-Proto: https` header. FastAPI's middleware respects this header.
|
||
|
||
---
|
||
|
||
## Component 6: Security Test Suite
|
||
|
||
### Purpose
|
||
|
||
Comprehensive security tests to verify protection against common vulnerabilities: timing attacks, SQL injection, XSS, open redirects, CSRF. Tests demonstrate compliance with OWASP Top 10 and OAuth 2.0 security best practices.
|
||
|
||
### Test Organization
|
||
|
||
**Directory**: `/tests/security/`
|
||
|
||
**Test Files**:
|
||
- `test_timing_attacks.py` - Timing attack resistance (token comparison)
|
||
- `test_sql_injection.py` - SQL injection prevention
|
||
- `test_xss_prevention.py` - Cross-site scripting prevention
|
||
- `test_open_redirect.py` - Open redirect prevention
|
||
- `test_csrf_protection.py` - CSRF protection (state parameter)
|
||
- `test_input_validation.py` - Input validation edge cases
|
||
|
||
**Pytest Marker**: Add `@pytest.mark.security` to all security tests for easy execution:
|
||
|
||
```bash
|
||
# Run only security tests
|
||
pytest -m security
|
||
|
||
# Run security tests with coverage
|
||
pytest -m security --cov=src/gondulf --cov-report=html
|
||
```
|
||
|
||
**Marker Registration**: See `/docs/designs/phase-4b-clarifications.md` section 7. Register the `security` marker in `pytest.ini` or `pyproject.toml` to prevent warnings and enable targeted test runs.
|
||
|
||
### Test 1: Timing Attack Resistance
|
||
|
||
**File**: `/tests/security/test_timing_attacks.py`
|
||
|
||
**Purpose**: Verify that token comparison uses constant-time algorithms to prevent timing side-channel attacks.
|
||
|
||
```python
|
||
"""Security tests for timing attack resistance."""
|
||
|
||
import secrets
|
||
import time
|
||
from statistics import mean, stdev
|
||
|
||
import pytest
|
||
|
||
from gondulf.services.token_service import TokenService
|
||
|
||
|
||
@pytest.mark.security
|
||
class TestTimingAttackResistance:
|
||
"""Test timing attack resistance in token validation."""
|
||
|
||
def test_token_verification_constant_time(self, db_session):
|
||
"""
|
||
Test that token verification takes similar time for valid and invalid tokens.
|
||
|
||
Timing attacks exploit differences in processing time to guess secrets.
|
||
This test verifies that token verification uses constant-time comparison.
|
||
"""
|
||
token_service = TokenService(db_session)
|
||
|
||
# Generate valid token
|
||
me = "https://user.example.com"
|
||
client_id = "https://client.example.com"
|
||
token = token_service.generate_access_token(
|
||
me=me,
|
||
client_id=client_id,
|
||
scope=""
|
||
)
|
||
|
||
# Measure time for valid token (hits database, passes validation)
|
||
valid_times = []
|
||
for _ in range(100):
|
||
start = time.perf_counter()
|
||
result = token_service.verify_access_token(token)
|
||
end = time.perf_counter()
|
||
valid_times.append(end - start)
|
||
assert result is not None # Valid token
|
||
|
||
# Measure time for invalid token (misses database, fails validation)
|
||
invalid_token = secrets.token_urlsafe(32)
|
||
invalid_times = []
|
||
for _ in range(100):
|
||
start = time.perf_counter()
|
||
result = token_service.verify_access_token(invalid_token)
|
||
end = time.perf_counter()
|
||
invalid_times.append(end - start)
|
||
assert result is None # Invalid token
|
||
|
||
# Statistical analysis: times should be similar
|
||
valid_mean = mean(valid_times)
|
||
invalid_mean = mean(invalid_times)
|
||
valid_stdev = stdev(valid_times)
|
||
invalid_stdev = stdev(invalid_times)
|
||
|
||
# Difference in means should be small relative to standard deviations
|
||
# Allow 3x stdev difference (99.7% confidence interval)
|
||
max_allowed_diff = 3 * max(valid_stdev, invalid_stdev)
|
||
actual_diff = abs(valid_mean - invalid_mean)
|
||
|
||
assert actual_diff < max_allowed_diff, (
|
||
f"Timing difference suggests timing attack vulnerability: "
|
||
f"valid={valid_mean:.6f}s, invalid={invalid_mean:.6f}s, "
|
||
f"diff={actual_diff:.6f}s, max_allowed={max_allowed_diff:.6f}s"
|
||
)
|
||
|
||
def test_hash_comparison_uses_constant_time(self):
|
||
"""
|
||
Test that hash comparison uses secrets.compare_digest.
|
||
|
||
This is a code inspection test (verified by reading token_service.py).
|
||
"""
|
||
import inspect
|
||
from gondulf.services.token_service import TokenService
|
||
|
||
source = inspect.getsource(TokenService.verify_access_token)
|
||
|
||
# Verify that secrets.compare_digest is used (or equivalent)
|
||
# OR that comparison happens in SQL (which is also constant-time)
|
||
assert "compare_digest" in source or "SELECT" in source, (
|
||
"Token verification should use constant-time comparison or SQL lookup"
|
||
)
|
||
|
||
def test_authorization_code_verification_constant_time(self, code_store):
|
||
"""
|
||
Test that authorization code verification is constant-time.
|
||
|
||
Similar to token verification test.
|
||
"""
|
||
# Store valid code
|
||
code = secrets.token_urlsafe(32)
|
||
code_data = {
|
||
"client_id": "https://client.example.com",
|
||
"redirect_uri": "https://client.example.com/callback",
|
||
"me": "https://user.example.com",
|
||
"state": "test-state"
|
||
}
|
||
code_store.store_authorization_code(code, code_data)
|
||
|
||
# Measure valid code lookup
|
||
valid_times = []
|
||
for _ in range(100):
|
||
start = time.perf_counter()
|
||
result = code_store.get_authorization_code(code)
|
||
end = time.perf_counter()
|
||
valid_times.append(end - start)
|
||
|
||
# Measure invalid code lookup
|
||
invalid_code = secrets.token_urlsafe(32)
|
||
invalid_times = []
|
||
for _ in range(100):
|
||
start = time.perf_counter()
|
||
result = code_store.get_authorization_code(invalid_code)
|
||
end = time.perf_counter()
|
||
invalid_times.append(end - start)
|
||
|
||
# Times should be similar
|
||
valid_mean = mean(valid_times)
|
||
invalid_mean = mean(invalid_times)
|
||
|
||
# In-memory dict lookup is very fast, so allow tighter bounds
|
||
# But still should be constant-time
|
||
assert abs(valid_mean - invalid_mean) < 0.001, (
|
||
"Authorization code lookup timing difference detected"
|
||
)
|
||
```
|
||
|
||
**Acceptance Criteria**:
|
||
- Token verification timing difference < 3x standard deviation
|
||
- Hash comparison uses `secrets.compare_digest` or SQL lookup
|
||
- Authorization code lookup is constant-time
|
||
|
||
**Implementation Note**: See `/docs/designs/phase-4b-clarifications.md` section 4 for handling timing test flakiness in CI environments (relaxed thresholds, increased samples, skip markers).
|
||
|
||
### Test 2: SQL Injection Prevention
|
||
|
||
**File**: `/tests/security/test_sql_injection.py`
|
||
|
||
**Purpose**: Verify that all database queries use parameterized queries and are not vulnerable to SQL injection.
|
||
|
||
```python
|
||
"""Security tests for SQL injection prevention."""
|
||
|
||
import pytest
|
||
from sqlalchemy.exc import SQLAlchemyError
|
||
|
||
from gondulf.services.token_service import TokenService
|
||
from gondulf.services.domain_verification import DomainVerificationService
|
||
|
||
|
||
@pytest.mark.security
|
||
class TestSQLInjectionPrevention:
|
||
"""Test SQL injection prevention in database queries."""
|
||
|
||
def test_token_service_sql_injection_in_me(self, db_session):
|
||
"""Test token service prevents SQL injection in 'me' parameter."""
|
||
token_service = TokenService(db_session)
|
||
|
||
# Attempt SQL injection via 'me' parameter
|
||
malicious_me = "https://user.example.com'; DROP TABLE tokens; --"
|
||
client_id = "https://client.example.com"
|
||
|
||
# Should not raise exception, should treat as literal string
|
||
token = token_service.generate_access_token(
|
||
me=malicious_me,
|
||
client_id=client_id,
|
||
scope=""
|
||
)
|
||
|
||
assert token is not None
|
||
|
||
# Verify token was stored safely (not executed as SQL)
|
||
result = token_service.verify_access_token(token)
|
||
assert result is not None
|
||
assert result["me"] == malicious_me # Stored as literal string
|
||
|
||
def test_domain_service_sql_injection_in_domain(self, db_session):
|
||
"""Test domain service prevents SQL injection in domain parameter."""
|
||
domain_service = DomainVerificationService(db_session)
|
||
|
||
# Attempt SQL injection via domain parameter
|
||
malicious_domain = "example.com'; DROP TABLE domains; --"
|
||
|
||
# Should not raise exception
|
||
# Note: This will fail validation (not a valid domain), but should not execute SQL
|
||
try:
|
||
domain_service.verify_domain_ownership(malicious_domain)
|
||
except ValueError:
|
||
# Expected: invalid domain format
|
||
pass
|
||
|
||
# Verify tables still exist (not dropped by injection)
|
||
result = db_session.execute("SELECT COUNT(*) FROM tokens")
|
||
assert result.scalar() >= 0 # Table exists
|
||
|
||
def test_token_lookup_sql_injection(self, db_session):
|
||
"""Test token lookup prevents SQL injection in token parameter."""
|
||
token_service = TokenService(db_session)
|
||
|
||
# Attempt SQL injection via token parameter
|
||
malicious_token = "' OR '1'='1"
|
||
|
||
# Should return None (not found), not execute malicious SQL
|
||
result = token_service.verify_access_token(malicious_token)
|
||
assert result is None
|
||
|
||
def test_parameterized_queries_used(self):
|
||
"""
|
||
Test that all database queries use parameterized queries.
|
||
|
||
This is a code inspection test.
|
||
"""
|
||
import ast
|
||
import inspect
|
||
from pathlib import Path
|
||
|
||
# Check all service files
|
||
services_dir = Path("src/gondulf/services")
|
||
violations = []
|
||
|
||
for service_file in services_dir.glob("*.py"):
|
||
source = service_file.read_text()
|
||
|
||
# Check for dangerous patterns (string formatting in SQL)
|
||
if 'f"SELECT' in source or "f'SELECT" in source:
|
||
violations.append(f"{service_file}: f-string in SQL query")
|
||
if '.format(' in source and 'SELECT' in source:
|
||
violations.append(f"{service_file}: .format() in SQL query")
|
||
if 'f"INSERT' in source or "f'INSERT" in source:
|
||
violations.append(f"{service_file}: f-string in INSERT query")
|
||
|
||
assert not violations, (
|
||
"SQL injection vulnerabilities detected:\n" + "\n".join(violations)
|
||
)
|
||
```
|
||
|
||
**Acceptance Criteria**:
|
||
- Malicious SQL strings treated as literal values
|
||
- No SQL execution from injected parameters
|
||
- All queries use parameterized format (behavioral testing, not source inspection)
|
||
|
||
**Implementation Note**: See `/docs/designs/phase-4b-clarifications.md` section 5. Tests should verify actual behavior (injection attempts fail safely) rather than inspecting source code patterns.
|
||
|
||
### Test 3: XSS Prevention
|
||
|
||
**File**: `/tests/security/test_xss_prevention.py`
|
||
|
||
**Purpose**: Verify that user input is properly escaped in HTML templates to prevent cross-site scripting.
|
||
|
||
```python
|
||
"""Security tests for XSS prevention."""
|
||
|
||
import pytest
|
||
from fastapi.testclient import TestClient
|
||
|
||
from gondulf.main import app
|
||
|
||
|
||
@pytest.mark.security
|
||
class TestXSSPrevention:
|
||
"""Test XSS prevention in HTML templates."""
|
||
|
||
def test_client_name_xss_escaped(self, client):
|
||
"""Test that client name is HTML-escaped in consent screen."""
|
||
# Mock h-app parser to return malicious client name
|
||
malicious_name = '<script>alert("XSS")</script>'
|
||
|
||
# Request authorization with malicious client metadata
|
||
# (This requires mocking the h-app parser)
|
||
# For now, test that templates auto-escape by inspecting template
|
||
|
||
from jinja2 import Environment
|
||
env = Environment(autoescape=True)
|
||
|
||
template_source = '{{ client_name }}'
|
||
template = env.from_string(template_source)
|
||
|
||
rendered = template.render(client_name=malicious_name)
|
||
|
||
# Should be escaped
|
||
assert '<script>' not in rendered
|
||
assert '<script>' in rendered
|
||
|
||
def test_me_parameter_xss_escaped(self):
|
||
"""Test that 'me' parameter is HTML-escaped in UI."""
|
||
from jinja2 import Environment
|
||
env = Environment(autoescape=True)
|
||
|
||
malicious_me = '<img src=x onerror="alert(1)">'
|
||
|
||
template_source = '<p>{{ me }}</p>'
|
||
template = env.from_string(template_source)
|
||
|
||
rendered = template.render(me=malicious_me)
|
||
|
||
# Should be escaped
|
||
assert '<img' not in rendered
|
||
assert '<img' in rendered
|
||
|
||
def test_client_url_xss_escaped(self):
|
||
"""Test that client URL is HTML-escaped in consent screen."""
|
||
from jinja2 import Environment
|
||
env = Environment(autoescape=True)
|
||
|
||
malicious_url = 'javascript:alert(1)'
|
||
|
||
template_source = '<a href="{{ client_url }}">{{ client_url }}</a>'
|
||
template = env.from_string(template_source)
|
||
|
||
rendered = template.render(client_url=malicious_url)
|
||
|
||
# Should be escaped (Jinja2 escapes href attributes)
|
||
# Note: This might still be vulnerable if not validated at input
|
||
assert 'javascript:' in rendered # Jinja2 doesn't prevent javascript: in href
|
||
# So we need validation layer too (handled by Pydantic HttpUrl)
|
||
|
||
def test_jinja2_autoescape_enabled(self):
|
||
"""Test that Jinja2 autoescaping is enabled in templates."""
|
||
from gondulf.main import app
|
||
|
||
# FastAPI uses Jinja2Templates which has autoescape=True by default
|
||
# Verify this is the case
|
||
from fastapi.templating import Jinja2Templates
|
||
import inspect
|
||
|
||
# Find Jinja2Templates instances in the codebase
|
||
# Should have autoescape=True
|
||
# This is a configuration check
|
||
|
||
# For FastAPI's Jinja2Templates, autoescape is True by default
|
||
# So this test just confirms the default is used
|
||
templates = Jinja2Templates(directory="templates")
|
||
assert templates.env.autoescape == True
|
||
```
|
||
|
||
**Acceptance Criteria**:
|
||
- All user input HTML-escaped in templates
|
||
- Jinja2 autoescape enabled
|
||
- No raw HTML rendering without explicit marking
|
||
|
||
### Test 4: Open Redirect Prevention
|
||
|
||
**File**: `/tests/security/test_open_redirect.py`
|
||
|
||
**Purpose**: Verify that redirect_uri validation prevents open redirect attacks.
|
||
|
||
```python
|
||
"""Security tests for open redirect prevention."""
|
||
|
||
import pytest
|
||
from fastapi.testclient import TestClient
|
||
|
||
from gondulf.main import app
|
||
from gondulf.services.validation import validate_redirect_uri
|
||
|
||
|
||
@pytest.mark.security
|
||
class TestOpenRedirectPrevention:
|
||
"""Test open redirect prevention in authorization flow."""
|
||
|
||
def test_redirect_uri_must_match_client_id_domain(self):
|
||
"""Test that redirect_uri domain must match client_id domain."""
|
||
client_id = "https://client.example.com"
|
||
|
||
# Valid: same domain
|
||
is_valid, _ = validate_redirect_uri(
|
||
"https://client.example.com/callback",
|
||
client_id
|
||
)
|
||
assert is_valid
|
||
|
||
# Invalid: different domain
|
||
is_valid, _ = validate_redirect_uri(
|
||
"https://attacker.com/steal",
|
||
client_id
|
||
)
|
||
assert not is_valid
|
||
|
||
def test_redirect_uri_subdomain_allowed(self):
|
||
"""Test that redirect_uri subdomain of client_id is allowed."""
|
||
client_id = "https://example.com"
|
||
|
||
# Valid: subdomain
|
||
is_valid, _ = validate_redirect_uri(
|
||
"https://app.example.com/callback",
|
||
client_id
|
||
)
|
||
assert is_valid
|
||
|
||
def test_redirect_uri_rejects_open_redirect(self):
|
||
"""Test that common open redirect patterns are rejected."""
|
||
client_id = "https://client.example.com"
|
||
|
||
# Test various open redirect patterns
|
||
malicious_uris = [
|
||
"https://client.example.com@attacker.com/callback",
|
||
"https://client.example.com.attacker.com/callback",
|
||
"https://client.example.com/../../../attacker.com",
|
||
"https://attacker.com?client.example.com",
|
||
"https://attacker.com#client.example.com",
|
||
]
|
||
|
||
for uri in malicious_uris:
|
||
is_valid, _ = validate_redirect_uri(uri, client_id)
|
||
assert not is_valid, f"Open redirect allowed: {uri}"
|
||
|
||
def test_authorization_endpoint_validates_redirect_uri(self, client):
|
||
"""Test that authorization endpoint validates redirect_uri."""
|
||
# Attempt authorization with malicious redirect_uri
|
||
response = client.get(
|
||
"/authorize",
|
||
params={
|
||
"me": "https://user.example.com",
|
||
"client_id": "https://client.example.com",
|
||
"redirect_uri": "https://attacker.com/steal",
|
||
"state": "test-state",
|
||
"response_type": "code"
|
||
}
|
||
)
|
||
|
||
# Should return error (not redirect)
|
||
assert response.status_code == 400
|
||
assert "redirect_uri" in response.json().get("error_description", "")
|
||
|
||
def test_redirect_uri_must_be_https(self):
|
||
"""Test that redirect_uri must use HTTPS (except localhost)."""
|
||
client_id = "https://client.example.com"
|
||
|
||
# Invalid: HTTP for non-localhost
|
||
is_valid, _ = validate_redirect_uri(
|
||
"http://client.example.com/callback",
|
||
client_id
|
||
)
|
||
assert not is_valid
|
||
|
||
# Valid: HTTPS
|
||
is_valid, _ = validate_redirect_uri(
|
||
"https://client.example.com/callback",
|
||
client_id
|
||
)
|
||
assert is_valid
|
||
|
||
# Valid: HTTP for localhost (development)
|
||
is_valid, _ = validate_redirect_uri(
|
||
"http://localhost:3000/callback",
|
||
"http://localhost:3000"
|
||
)
|
||
assert is_valid
|
||
```
|
||
|
||
**Acceptance Criteria**:
|
||
- redirect_uri domain must match client_id domain or be subdomain
|
||
- Common open redirect patterns rejected
|
||
- HTTPS enforced (except localhost)
|
||
- Authorization endpoint validates redirect_uri
|
||
|
||
### Test 5: CSRF Protection
|
||
|
||
**File**: `/tests/security/test_csrf_protection.py`
|
||
|
||
**Purpose**: Verify that state parameter provides CSRF protection in authorization flow.
|
||
|
||
```python
|
||
"""Security tests for CSRF protection."""
|
||
|
||
import pytest
|
||
from fastapi.testclient import TestClient
|
||
|
||
from gondulf.main import app
|
||
|
||
|
||
@pytest.mark.security
|
||
class TestCSRFProtection:
|
||
"""Test CSRF protection via state parameter."""
|
||
|
||
def test_state_parameter_required(self, client):
|
||
"""Test that state parameter is required in authorization request."""
|
||
response = client.get(
|
||
"/authorize",
|
||
params={
|
||
"me": "https://user.example.com",
|
||
"client_id": "https://client.example.com",
|
||
"redirect_uri": "https://client.example.com/callback",
|
||
"response_type": "code"
|
||
# Missing state parameter
|
||
}
|
||
)
|
||
|
||
# Should return error
|
||
assert response.status_code == 400
|
||
assert "state" in response.json().get("error_description", "").lower()
|
||
|
||
def test_state_parameter_preserved_in_redirect(self, client, db_session):
|
||
"""Test that state parameter is included in redirect callback."""
|
||
# Complete authorization flow
|
||
# 1. Request authorization
|
||
state = "random-csrf-token-12345"
|
||
response = client.get(
|
||
"/authorize",
|
||
params={
|
||
"me": "https://user.example.com",
|
||
"client_id": "https://client.example.com",
|
||
"redirect_uri": "https://client.example.com/callback",
|
||
"state": state,
|
||
"response_type": "code"
|
||
}
|
||
)
|
||
|
||
# 2. Simulate user approval (POST to /authorize)
|
||
# ... (requires full integration test with domain verification)
|
||
|
||
# 3. Check that redirect includes state parameter
|
||
# In callback: https://client.example.com/callback?code=...&state=...
|
||
# This is tested in integration tests
|
||
|
||
def test_state_parameter_minimum_length(self, client):
|
||
"""Test that state parameter has minimum length for security."""
|
||
# Very short state (weak CSRF token)
|
||
response = client.get(
|
||
"/authorize",
|
||
params={
|
||
"me": "https://user.example.com",
|
||
"client_id": "https://client.example.com",
|
||
"redirect_uri": "https://client.example.com/callback",
|
||
"state": "123", # Too short
|
||
"response_type": "code"
|
||
}
|
||
)
|
||
|
||
# Should accept (OAuth 2.0 doesn't mandate minimum length)
|
||
# But recommend 32+ chars in documentation
|
||
# For v1.0.0, accept any non-empty state
|
||
|
||
def test_state_parameter_returned_unchanged(self, db_session):
|
||
"""Test that state parameter is returned unchanged (not modified)."""
|
||
from gondulf.services.authorization_service import AuthorizationService
|
||
|
||
auth_service = AuthorizationService(db_session)
|
||
|
||
original_state = "my-csrf-token-with-special-chars-!@#$%"
|
||
|
||
# Store authorization code
|
||
code = auth_service.generate_authorization_code(
|
||
me="https://user.example.com",
|
||
client_id="https://client.example.com",
|
||
redirect_uri="https://client.example.com/callback",
|
||
state=original_state
|
||
)
|
||
|
||
# Retrieve code data
|
||
code_data = auth_service.get_authorization_code(code)
|
||
|
||
# State should be unchanged
|
||
assert code_data["state"] == original_state
|
||
```
|
||
|
||
**Acceptance Criteria**:
|
||
- State parameter required in authorization requests
|
||
- State parameter preserved and returned in callback
|
||
- State parameter returned unchanged (no modification)
|
||
|
||
### Test 6: Input Validation Edge Cases
|
||
|
||
**File**: `/tests/security/test_input_validation.py`
|
||
|
||
**Purpose**: Verify that input validation handles edge cases and malicious input safely.
|
||
|
||
```python
|
||
"""Security tests for input validation."""
|
||
|
||
import pytest
|
||
|
||
from gondulf.services.validation import (
|
||
validate_url,
|
||
validate_redirect_uri,
|
||
validate_email_format
|
||
)
|
||
|
||
|
||
@pytest.mark.security
|
||
class TestInputValidation:
|
||
"""Test input validation edge cases and security."""
|
||
|
||
def test_url_validation_rejects_javascript_protocol(self):
|
||
"""Test that javascript: URLs are rejected."""
|
||
is_valid, _ = validate_url("javascript:alert(1)")
|
||
assert not is_valid
|
||
|
||
def test_url_validation_rejects_data_protocol(self):
|
||
"""Test that data: URLs are rejected."""
|
||
is_valid, _ = validate_url("data:text/html,<script>alert(1)</script>")
|
||
assert not is_valid
|
||
|
||
def test_url_validation_rejects_file_protocol(self):
|
||
"""Test that file: URLs are rejected."""
|
||
is_valid, _ = validate_url("file:///etc/passwd")
|
||
assert not is_valid
|
||
|
||
def test_url_validation_handles_unicode(self):
|
||
"""Test that URL validation handles Unicode safely."""
|
||
# IDN homograph attack
|
||
is_valid, _ = validate_url("https://аpple.com") # Cyrillic 'а'
|
||
# Should either reject or normalize to punycode
|
||
# For v1.0.0, basic validation (may accept)
|
||
|
||
def test_url_validation_handles_very_long_urls(self):
|
||
"""Test that URL validation handles very long URLs."""
|
||
long_url = "https://example.com/" + "a" * 10000
|
||
|
||
# Should handle without crashing (may reject)
|
||
try:
|
||
is_valid, _ = validate_url(long_url)
|
||
except Exception as e:
|
||
pytest.fail(f"URL validation crashed on long URL: {e}")
|
||
|
||
def test_email_validation_rejects_injection(self):
|
||
"""Test that email validation rejects injection attempts."""
|
||
malicious_emails = [
|
||
"user@example.com\nBcc: attacker@evil.com",
|
||
"user@example.com\r\nSubject: Injected",
|
||
"user@example.com<script>alert(1)</script>",
|
||
]
|
||
|
||
for email in malicious_emails:
|
||
is_valid = validate_email_format(email)
|
||
assert not is_valid, f"Email injection allowed: {email}"
|
||
|
||
def test_null_byte_injection_rejected(self):
|
||
"""Test that null byte injection is rejected."""
|
||
malicious_url = "https://example.com\x00.attacker.com"
|
||
|
||
is_valid, _ = validate_url(malicious_url)
|
||
assert not is_valid
|
||
```
|
||
|
||
**Acceptance Criteria**:
|
||
- Dangerous URL protocols rejected (javascript:, data:, file:)
|
||
- Email injection attempts rejected
|
||
- Null byte injection rejected
|
||
- Very long inputs handled safely
|
||
|
||
---
|
||
|
||
## Component 7: PII Logging Audit
|
||
|
||
### Purpose
|
||
|
||
Review all logging statements throughout the codebase to ensure no personally identifiable information (PII) is logged. PII includes email addresses, tokens, codes, and IP addresses.
|
||
|
||
### Audit Process
|
||
|
||
**Step 1: Grep for Logging Statements**
|
||
|
||
```bash
|
||
# Find all logging statements
|
||
grep -rn "logger\." src/gondulf/ > /tmp/logging_audit.txt
|
||
|
||
# Find potential PII in logs
|
||
grep -rn "logger.*email" src/gondulf/
|
||
grep -rn "logger.*token" src/gondulf/
|
||
grep -rn "logger.*code" src/gondulf/
|
||
grep -rn "logger.*password" src/gondulf/
|
||
```
|
||
|
||
**Step 2: Manual Review**
|
||
|
||
Review each logging statement for:
|
||
- Email addresses (PII)
|
||
- Full tokens or codes (security)
|
||
- Passwords (security)
|
||
- IP addresses (PII in some jurisdictions)
|
||
|
||
**Step 3: Remediation Patterns**
|
||
|
||
**SAFE Logging Patterns**:
|
||
```python
|
||
# GOOD: Domain only (public information)
|
||
logger.info(f"Authorization granted for {domain} to {client_id}")
|
||
|
||
# GOOD: Token prefix for correlation (first 8 chars)
|
||
logger.debug(f"Token generated: {token[:8]}...")
|
||
|
||
# GOOD: Error without sensitive data
|
||
logger.error(f"Email send failed for domain {domain}")
|
||
|
||
# GOOD: Count/aggregate data
|
||
logger.info(f"Cleaned up {count} expired tokens")
|
||
```
|
||
|
||
**UNSAFE Logging Patterns** (to fix):
|
||
```python
|
||
# BAD: Full email address
|
||
logger.info(f"Verification sent to {email}") # ← REMOVE email
|
||
|
||
# BAD: Full token
|
||
logger.debug(f"Token: {token}") # ← Use token[:8] only
|
||
|
||
# BAD: Full authorization code
|
||
logger.info(f"Code generated: {code}") # ← Use code[:8] only
|
||
|
||
# BAD: IP address
|
||
logger.info(f"Request from {request.client.host}") # ← Remove IP
|
||
```
|
||
|
||
**Step 4: Code Changes**
|
||
|
||
Create a checklist of files to update:
|
||
|
||
```markdown
|
||
## PII Logging Audit Checklist
|
||
|
||
- [ ] `/src/gondulf/services/email.py` - Remove email addresses from logs
|
||
- [ ] `/src/gondulf/services/token_service.py` - Use token[:8] prefix only
|
||
- [ ] `/src/gondulf/routers/authorization.py` - No PII in logs
|
||
- [ ] `/src/gondulf/routers/token.py` - No PII in logs
|
||
- [ ] `/src/gondulf/routers/verification.py` - Remove email from logs
|
||
- [ ] `/src/gondulf/services/domain_verification.py` - Remove email from logs
|
||
- [ ] `/src/gondulf/main.py` - Review startup logs
|
||
```
|
||
|
||
### Testing Requirements
|
||
|
||
**File**: `/tests/security/test_pii_logging.py`
|
||
|
||
```python
|
||
"""Security tests for PII in logging."""
|
||
|
||
import logging
|
||
import re
|
||
import pytest
|
||
|
||
|
||
@pytest.mark.security
|
||
class TestPIILogging:
|
||
"""Test that no PII is logged."""
|
||
|
||
def test_no_email_addresses_in_logs(self, caplog):
|
||
"""Test that email addresses are not logged."""
|
||
# Email regex pattern
|
||
email_pattern = r'\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Z|a-z]{2,}\b'
|
||
|
||
caplog.set_level(logging.DEBUG)
|
||
|
||
# Trigger various operations that might log email
|
||
# ... (requires integration test)
|
||
|
||
# Check logs for email addresses
|
||
for record in caplog.records:
|
||
match = re.search(email_pattern, record.message)
|
||
assert match is None, (
|
||
f"Email address found in log: {record.message}"
|
||
)
|
||
|
||
def test_no_full_tokens_in_logs(self, caplog):
|
||
"""Test that full tokens are not logged (only prefixes)."""
|
||
caplog.set_level(logging.DEBUG)
|
||
|
||
# Trigger token generation
|
||
# ... (requires integration test)
|
||
|
||
# Check logs for long alphanumeric strings (potential tokens)
|
||
for record in caplog.records:
|
||
# Tokens are 32+ chars of base64url
|
||
long_tokens = re.findall(r'[A-Za-z0-9_-]{32,}', record.message)
|
||
|
||
# If found, should be prefixed with "..." or truncated
|
||
for token in long_tokens:
|
||
assert "..." in record.message, (
|
||
f"Full token logged: {record.message}"
|
||
)
|
||
|
||
def test_no_passwords_in_logs(self, caplog):
|
||
"""Test that passwords are never logged."""
|
||
caplog.set_level(logging.DEBUG)
|
||
|
||
# Check all logs for "password" keyword
|
||
for record in caplog.records:
|
||
if "password" in record.message.lower():
|
||
# Should only be in config messages like "SMTP password: ***"
|
||
assert "***" in record.message or "password" in record.levelname.lower()
|
||
|
||
def test_logging_guidelines_documented(self):
|
||
"""Test that logging guidelines are documented."""
|
||
# Check for LOGGING.md or similar documentation
|
||
from pathlib import Path
|
||
|
||
docs_dir = Path("docs/standards")
|
||
coding_doc = docs_dir / "coding.md"
|
||
|
||
assert coding_doc.exists(), "Coding standards documentation missing"
|
||
|
||
content = coding_doc.read_text()
|
||
assert "logging" in content.lower()
|
||
assert "PII" in content or "personal" in content.lower()
|
||
```
|
||
|
||
**Acceptance Criteria**:
|
||
- No email addresses in logs (grep verification)
|
||
- No full tokens in logs (only 8-char prefixes with ellipsis suffix)
|
||
- No passwords in logs
|
||
- No IP addresses in logs (except rate limiting, temporary)
|
||
- Logging guidelines documented in `/docs/standards/coding.md`
|
||
|
||
**Implementation Notes**:
|
||
- See `/docs/designs/phase-4b-clarifications.md` section 3 for consistent token prefix format (exactly 8 chars + ellipsis)
|
||
- See section 8 for secure logging guidelines structure and examples to add to coding standards
|
||
|
||
---
|
||
|
||
## Error Handling
|
||
|
||
### Security Header Errors
|
||
|
||
**Scenario**: Middleware fails to add headers
|
||
|
||
**Handling**: Log error but continue processing (fail open, not fail closed). Security headers are defense-in-depth, not primary security.
|
||
|
||
```python
|
||
try:
|
||
response.headers["X-Frame-Options"] = "DENY"
|
||
except Exception as e:
|
||
logger.error(f"Failed to add security header: {e}")
|
||
# Continue anyway
|
||
```
|
||
|
||
### HTTPS Enforcement Errors
|
||
|
||
**Scenario**: Cannot determine request scheme
|
||
|
||
**Handling**: Assume HTTPS in production (conservative). Log warning.
|
||
|
||
```python
|
||
scheme = request.url.scheme or "https" # Default to HTTPS
|
||
if not scheme:
|
||
logger.warning("Cannot determine request scheme, assuming HTTPS")
|
||
```
|
||
|
||
### Security Test Failures
|
||
|
||
**Scenario**: Security test fails
|
||
|
||
**Handling**: Block merge/deployment. Security tests are blocking gates.
|
||
|
||
```yaml
|
||
# CI/CD
|
||
- name: Run Security Tests
|
||
run: pytest -m security
|
||
# If fails, pipeline stops (no merge)
|
||
```
|
||
|
||
---
|
||
|
||
## Security Considerations
|
||
|
||
### Defense-in-Depth
|
||
|
||
This phase implements multiple layers of security:
|
||
1. **Network**: HTTPS enforcement
|
||
2. **Headers**: Browser-level protections
|
||
3. **Input**: Validation (Pydantic, already implemented)
|
||
4. **Processing**: Constant-time operations
|
||
5. **Output**: HTML escaping
|
||
6. **Storage**: Hashed tokens, parameterized SQL
|
||
7. **Logging**: No PII exposure
|
||
|
||
### Threat Coverage
|
||
|
||
| Threat | Mitigation | Component |
|
||
|--------|------------|-----------|
|
||
| Clickjacking | X-Frame-Options: DENY | Security Headers |
|
||
| XSS | CSP + HTML escaping | Security Headers + Templates |
|
||
| MIME Sniffing | X-Content-Type-Options | Security Headers |
|
||
| Man-in-the-Middle | HTTPS + HSTS | HTTPS Enforcement + Headers |
|
||
| SQL Injection | Parameterized queries | Already implemented (verified by tests) |
|
||
| Timing Attacks | Constant-time comparison | Already implemented (verified by tests) |
|
||
| Open Redirects | redirect_uri validation | Already implemented (verified by tests) |
|
||
| CSRF | State parameter | Already implemented (verified by tests) |
|
||
| Privacy Violation | No PII logging | PII Audit |
|
||
|
||
### Residual Risks
|
||
|
||
**Accepted Risks** (v1.0.0):
|
||
- DDoS attacks (handled by infrastructure/CDN)
|
||
- Zero-day vulnerabilities in dependencies (monitoring required)
|
||
- Social engineering (user education)
|
||
- Compromised client applications (trust model limitation)
|
||
|
||
**Future Mitigations** (v1.1.0+):
|
||
- Rate limiting (prevent brute force)
|
||
- PKCE (additional token security)
|
||
- Token revocation (limit blast radius)
|
||
|
||
---
|
||
|
||
## Testing Strategy
|
||
|
||
### Test Organization
|
||
|
||
**Pytest Markers**:
|
||
```python
|
||
# Mark all security tests
|
||
@pytest.mark.security
|
||
|
||
# Run security tests only
|
||
pytest -m security
|
||
```
|
||
|
||
**Test Levels**:
|
||
- **Integration**: Security headers, HTTPS enforcement (middleware)
|
||
- **Security**: Timing attacks, injection, XSS, redirects, CSRF
|
||
- **Unit**: PII logging (caplog inspection)
|
||
|
||
**Coverage Target**: 100% of security-critical code
|
||
- Token validation: 100%
|
||
- Input validation: 100%
|
||
- Middleware: 100%
|
||
|
||
### Test Execution
|
||
|
||
**Local Development**:
|
||
```bash
|
||
# Run all security tests
|
||
pytest -m security -v
|
||
|
||
# Run with coverage
|
||
pytest -m security --cov=src/gondulf --cov-report=html
|
||
|
||
# Run specific security test file
|
||
pytest tests/security/test_timing_attacks.py -v
|
||
```
|
||
|
||
**CI/CD Pipeline**:
|
||
```yaml
|
||
security_tests:
|
||
stage: test
|
||
script:
|
||
- pytest -m security --cov=src/gondulf --cov-report=term --cov-report=xml
|
||
- bandit -r src/gondulf/ # Security linter
|
||
- pip-audit # Dependency vulnerability scan
|
||
artifacts:
|
||
reports:
|
||
coverage: coverage.xml
|
||
```
|
||
|
||
**Blocking Gate**: Security test failures block merge to main branch.
|
||
|
||
---
|
||
|
||
## Acceptance Criteria
|
||
|
||
### Component 4: Security Headers Middleware
|
||
|
||
- [ ] `SecurityHeadersMiddleware` class implemented in `/src/gondulf/middleware/security_headers.py`
|
||
- [ ] All 7 security headers added to responses:
|
||
- [ ] X-Frame-Options: DENY
|
||
- [ ] X-Content-Type-Options: nosniff
|
||
- [ ] X-XSS-Protection: 1; mode=block
|
||
- [ ] Strict-Transport-Security (production only)
|
||
- [ ] Content-Security-Policy (configured for templates + h-app logos)
|
||
- [ ] Referrer-Policy: strict-origin-when-cross-origin
|
||
- [ ] Permissions-Policy: geolocation=(), microphone=(), camera=()
|
||
- [ ] HSTS header conditional on production mode (not in DEBUG mode)
|
||
- [ ] Middleware registered in `main.py`
|
||
- [ ] All integration tests pass (`tests/integration/test_security_headers.py`)
|
||
- [ ] Headers present on all endpoints (tested)
|
||
- [ ] Headers present on error responses (tested)
|
||
|
||
### Component 5: HTTPS Enforcement
|
||
|
||
- [ ] `HTTPSEnforcementMiddleware` class implemented in `/src/gondulf/middleware/https_enforcement.py`
|
||
- [ ] HTTP requests blocked or redirected in production mode
|
||
- [ ] HTTP allowed for localhost in development mode
|
||
- [ ] Configuration option `GONDULF_HTTPS_REDIRECT` (default: true)
|
||
- [ ] Middleware registered in `main.py` (before security headers)
|
||
- [ ] Integration tests pass (`tests/integration/test_https_enforcement.py`)
|
||
- [ ] Production deployment documentation updated (nginx example)
|
||
|
||
### Component 6: Security Test Suite
|
||
|
||
- [ ] Security test directory created: `/tests/security/`
|
||
- [ ] All 6 security test files implemented:
|
||
- [ ] `test_timing_attacks.py` - Timing attack resistance tests pass
|
||
- [ ] `test_sql_injection.py` - SQL injection prevention tests pass
|
||
- [ ] `test_xss_prevention.py` - XSS prevention tests pass
|
||
- [ ] `test_open_redirect.py` - Open redirect prevention tests pass
|
||
- [ ] `test_csrf_protection.py` - CSRF protection tests pass
|
||
- [ ] `test_input_validation.py` - Input validation edge case tests pass
|
||
- [ ] All security tests marked with `@pytest.mark.security`
|
||
- [ ] All security tests pass: `pytest -m security`
|
||
- [ ] Security test coverage ≥95% for security-critical code
|
||
- [ ] CI/CD pipeline runs security tests (blocking gate)
|
||
|
||
### Component 7: PII Logging Audit
|
||
|
||
- [ ] All logging statements reviewed (grep audit completed)
|
||
- [ ] No email addresses in logs (verified by grep + tests)
|
||
- [ ] No full tokens in logs (only 8-char prefixes, verified)
|
||
- [ ] No passwords in logs (verified)
|
||
- [ ] No IP addresses in logs except rate limiting (verified)
|
||
- [ ] PII logging test implemented: `/tests/security/test_pii_logging.py`
|
||
- [ ] Logging guidelines documented in `/docs/standards/coding.md`
|
||
- [ ] Code changes committed with clear commit message
|
||
|
||
### Overall Phase 4b Completion
|
||
|
||
- [ ] All components implemented and tested
|
||
- [ ] All security tests passing
|
||
- [ ] No security warnings from bandit or pip-audit
|
||
- [ ] Code review completed (security focus)
|
||
- [ ] Documentation updated:
|
||
- [ ] `/docs/architecture/security.md` - Implementation confirmed
|
||
- [ ] `/docs/standards/coding.md` - Logging guidelines added
|
||
- [ ] Deployment guide includes HTTPS configuration
|
||
- [ ] Implementation report created: `/docs/reports/YYYY-MM-DD-phase-4b-security-hardening.md`
|
||
|
||
---
|
||
|
||
## Dependencies
|
||
|
||
**Upstream Dependencies** (must be complete before starting):
|
||
- Phase 1 (Foundation): COMPLETE ✅
|
||
- Phase 2 (Domain Verification): COMPLETE ✅
|
||
- Phase 3 (IndieAuth Protocol): COMPLETE ✅
|
||
- Phase 4a (Client Metadata): IN PROGRESS (parallel track)
|
||
|
||
**Downstream Dependencies** (blocked until complete):
|
||
- Phase 5 (Deployment & Testing): Requires Phase 4b complete
|
||
- v1.0.0 Release: Requires Phase 4b complete
|
||
|
||
**Parallel Work**:
|
||
- Phase 4a (Metadata Endpoint + Client Metadata): Can proceed in parallel
|
||
- Phase 4b (Security Hardening): Can proceed immediately
|
||
|
||
---
|
||
|
||
## Implementation Notes
|
||
|
||
### Estimated Effort
|
||
|
||
**Component 4: Security Headers Middleware**
|
||
- Implementation: 2-3 hours
|
||
- Testing: 2-3 hours
|
||
- **Total**: 0.5 days
|
||
|
||
**Component 5: HTTPS Enforcement**
|
||
- Implementation: 2-3 hours
|
||
- Testing: 1-2 hours
|
||
- **Total**: 0.5 days
|
||
|
||
**Component 6: Security Test Suite**
|
||
- Test 1 (Timing Attacks): 3-4 hours
|
||
- Test 2 (SQL Injection): 2-3 hours
|
||
- Test 3 (XSS Prevention): 2-3 hours
|
||
- Test 4 (Open Redirect): 2-3 hours
|
||
- Test 5 (CSRF Protection): 2-3 hours
|
||
- Test 6 (Input Validation): 2-3 hours
|
||
- **Total**: 2-2.5 days
|
||
|
||
**Component 7: PII Logging Audit**
|
||
- Grep audit: 1 hour
|
||
- Manual review: 2-3 hours
|
||
- Code changes: 2-3 hours
|
||
- Testing: 1-2 hours
|
||
- **Total**: 0.5-1 day
|
||
|
||
**Total Phase 4b Effort**: 3.5-4.5 days
|
||
|
||
### Recommended Implementation Order
|
||
|
||
1. **Day 1 Morning**: Security Headers Middleware (Component 4)
|
||
- Quick win, immediate security improvement
|
||
- Tests straightforward
|
||
|
||
2. **Day 1 Afternoon**: HTTPS Enforcement (Component 5)
|
||
- Pairs well with security headers
|
||
- Complete middleware layer
|
||
|
||
3. **Day 2**: PII Logging Audit (Component 7)
|
||
- Requires careful review
|
||
- Must be thorough
|
||
- Best done fresh
|
||
|
||
4. **Days 3-4**: Security Test Suite (Component 6)
|
||
- Most time-consuming
|
||
- Multiple test files
|
||
- Save for when middleware is complete
|
||
|
||
**Rationale**: Implement protections before verifying them. Security headers and HTTPS enforcement provide immediate value. PII audit is independent. Security tests verify everything at the end.
|
||
|
||
### Developer Notes
|
||
|
||
**Testing Considerations**:
|
||
- TestClient doesn't enforce HTTPS (limitation of test framework)
|
||
- Timing attack tests require statistical analysis (may be flaky)
|
||
- PII logging tests require caplog fixture and log level configuration
|
||
- Security headers tests are straightforward integration tests
|
||
|
||
**Code Review Focus**:
|
||
- Verify all security headers are correct
|
||
- Check HTTPS enforcement doesn't break development
|
||
- Ensure PII audit is complete (grep verification)
|
||
- Review timing attack test methodology
|
||
|
||
**Documentation**:
|
||
- Update deployment guide with HTTPS configuration (nginx)
|
||
- Add logging guidelines to coding standards
|
||
- Document security test execution in testing standards
|
||
|
||
---
|
||
|
||
## References
|
||
|
||
**Standards**:
|
||
- RFC 6797: HTTP Strict Transport Security (HSTS)
|
||
- RFC 7034: X-Frame-Options Header
|
||
- RFC 8414: OAuth 2.0 Authorization Server Metadata
|
||
- W3C Content Security Policy Level 3
|
||
- W3C IndieAuth Specification (Security Considerations)
|
||
- OWASP Top 10 (2021)
|
||
- OWASP Secure Headers Project
|
||
|
||
**Tools**:
|
||
- bandit: Python security linter
|
||
- pip-audit: Dependency vulnerability scanner
|
||
- pytest: Testing framework with security marker support
|
||
|
||
**Internal References**:
|
||
- `/docs/architecture/security.md` - Security architecture
|
||
- `/docs/standards/testing.md` - Testing standards
|
||
- `/docs/standards/coding.md` - Coding standards (to be updated)
|
||
- `/docs/reports/2025-11-20-gap-analysis-v1.0.0.md` - Gap analysis
|
||
|
||
---
|
||
|
||
**Design Status**: COMPLETE - CLARIFICATIONS PROVIDED
|
||
|
||
**Implementation Clarifications**: All Developer questions answered in `/docs/designs/phase-4b-clarifications.md` (2025-11-20):
|
||
1. CSP img-src allows any HTTPS source for client logos
|
||
2. HTTPS middleware checks X-Forwarded-Proto when TRUST_PROXY configured
|
||
3. Token logging uses consistent 8-char + ellipsis format
|
||
4. Timing tests use relaxed thresholds and increased samples in CI
|
||
5. SQL injection tests use behavioral testing, not source inspection
|
||
6. Security configuration (HTTPS_REDIRECT, TRUST_PROXY, SECURE_COOKIES) added to Config class
|
||
7. Pytest markers registered in pytest.ini/pyproject.toml
|
||
8. Comprehensive secure logging guidelines added to coding standards
|
||
|
||
**Next Steps**: Developer may proceed with implementation. Upon completion, Developer should create implementation report in `/docs/reports/YYYY-MM-DD-phase-4b-security-hardening.md`.
|
||
|
||
---
|
||
|
||
**Architect Sign-off**: This design is complete, all clarification questions are answered, and implementation may proceed. All security requirements from the v1.0.0 roadmap Phase 4 are addressed. Implementation of this design will satisfy exit criteria for Phase 4 (lines 212-218) and move v1.0.0 toward production readiness.
|