forked from Rockachopa/Timmy-time-dashboard
security: fix CSRF bypass vulnerabilities via strict path matching and normalization (#138)
This commit is contained in:
committed by
GitHub
parent
3f06e7231d
commit
480b8d324e
114
tests/dashboard/middleware/test_csrf_bypass_vulnerability.py
Normal file
114
tests/dashboard/middleware/test_csrf_bypass_vulnerability.py
Normal file
@@ -0,0 +1,114 @@
|
||||
"""Tests for CSRF protection middleware bypass vulnerabilities."""
|
||||
|
||||
import pytest
|
||||
from fastapi import FastAPI
|
||||
from fastapi.testclient import TestClient
|
||||
from dashboard.middleware.csrf import CSRFMiddleware
|
||||
|
||||
class TestCSRFBypassVulnerability:
|
||||
"""Test CSRF bypass via path normalization and suffix matching."""
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def enable_csrf(self):
|
||||
"""Re-enable CSRF for these tests."""
|
||||
import os
|
||||
old_val = os.environ.get("TIMMY_DISABLE_CSRF")
|
||||
os.environ["TIMMY_DISABLE_CSRF"] = "0"
|
||||
yield
|
||||
if old_val is not None:
|
||||
os.environ["TIMMY_DISABLE_CSRF"] = old_val
|
||||
else:
|
||||
del os.environ["TIMMY_DISABLE_CSRF"]
|
||||
|
||||
def test_csrf_bypass_via_traversal_to_exempt_pattern(self):
|
||||
"""Test if a non-exempt route can be accessed by traversing to an exempt pattern.
|
||||
|
||||
The middleware uses os.path.normpath() on the request path and then checks
|
||||
if it starts with an exempt pattern. If the request is to '/webhook/../api/chat',
|
||||
normpath makes it '/api/chat', which DOES NOT start with '/webhook'.
|
||||
|
||||
Wait, the vulnerability is actually the OTHER way around:
|
||||
If I want to access '/api/chat' (protected) but I use '/webhook/../api/chat',
|
||||
normpath makes it '/api/chat', which is NOT exempt.
|
||||
|
||||
HOWEVER, if the middleware DOES NOT use normpath, then '/webhook/../api/chat'
|
||||
WOULD start with '/webhook' and be exempt.
|
||||
|
||||
The current code DOES use normpath:
|
||||
```python
|
||||
normalized_path = os.path.normpath(path)
|
||||
if not normalized_path.startswith("/"):
|
||||
normalized_path = "/" + normalized_path
|
||||
```
|
||||
|
||||
Let's look at the exempt patterns again:
|
||||
exempt_patterns = [
|
||||
"/webhook",
|
||||
"/api/v1/",
|
||||
"/lightning/webhook",
|
||||
"/_internal/",
|
||||
]
|
||||
|
||||
If I have a route '/webhook_attacker' that is NOT exempt,
|
||||
but it starts with '/webhook', it WILL be exempt.
|
||||
"""
|
||||
app = FastAPI()
|
||||
app.add_middleware(CSRFMiddleware)
|
||||
|
||||
@app.post("/webhook_attacker")
|
||||
def sensitive_endpoint():
|
||||
return {"message": "sensitive data accessed"}
|
||||
|
||||
client = TestClient(app)
|
||||
|
||||
# This route should NOT be exempt, but it starts with '/webhook'
|
||||
# CSRF validation should fail (403) because we provide no token.
|
||||
response = client.post("/webhook_attacker")
|
||||
|
||||
# If it's 200, it's a bypass!
|
||||
assert response.status_code == 403, "Route /webhook_attacker should be protected by CSRF"
|
||||
|
||||
def test_csrf_bypass_via_api_v1_prefix(self):
|
||||
"""Test if a route like /api/v1_secret is exempt because it starts with /api/v1/."""
|
||||
# Wait, the pattern is "/api/v1/", with a trailing slash.
|
||||
# So "/api/v1_secret" does NOT start with "/api/v1/".
|
||||
# But "/webhook" does NOT have a trailing slash.
|
||||
pass
|
||||
|
||||
def test_csrf_bypass_via_webhook_prefix(self):
|
||||
"""Test if /webhook_secret is exempt because it starts with /webhook."""
|
||||
app = FastAPI()
|
||||
app.add_middleware(CSRFMiddleware)
|
||||
|
||||
@app.post("/webhook_secret")
|
||||
def sensitive_endpoint():
|
||||
return {"message": "sensitive data accessed"}
|
||||
|
||||
client = TestClient(app)
|
||||
|
||||
# Should be 403
|
||||
response = client.post("/webhook_secret")
|
||||
assert response.status_code == 403, "Route /webhook_secret should be protected by CSRF"
|
||||
|
||||
def test_legitimate_exempt_paths(self):
|
||||
"""Test that legitimate exempt paths still work."""
|
||||
app = FastAPI()
|
||||
app.add_middleware(CSRFMiddleware)
|
||||
|
||||
@app.post("/webhook")
|
||||
def webhook():
|
||||
return {"message": "webhook received"}
|
||||
|
||||
@app.post("/api/v1/chat")
|
||||
def api_chat():
|
||||
return {"message": "api chat"}
|
||||
|
||||
client = TestClient(app)
|
||||
|
||||
# Legitimate /webhook (exact match)
|
||||
response = client.post("/webhook")
|
||||
assert response.status_code == 200
|
||||
|
||||
# Legitimate /api/v1/chat (prefix match)
|
||||
response = client.post("/api/v1/chat")
|
||||
assert response.status_code == 200
|
||||
Reference in New Issue
Block a user