fix: harden monitoring dashboard CORS and add optional auth - #109
Conversation
- Replace wildcard '*' CORS origin with host-specific defaults - Warn when binding to 0.0.0.0 without auth - Add optional auth_token parameter for bearer-token auth on API/WS - Wildcard '*' origin disables allow_credentials per CORS spec - 14 new unit tests for auth accept/reject, CORS, and constructor Closes the monitoring dashboard auth+CORS TODO item (P1).
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesMonitoringDashboard now supports optional bearer-token authentication for API and WebSocket routes, host-derived or custom CORS origins, wildcard credential restrictions, and bind-address warnings. The dashboard client forwards tokens for WebSocket and metrics requests. Unit tests and TODO tracking cover the security changes. Monitoring Dashboard Security
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
evoseal/services/monitoring_dashboard.py (2)
48-77: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocstring warns against combining wildcard origins with
auth_token, but nothing enforces it.Lines 53-54 state to never combine
allowed_origins=["*"]withauth_token, but a caller can still do exactly that with no warning or guard. Consider logging a warning (similar to the0.0.0.0and wildcard-CORS warnings already added) when both are set together, so the documented risk is actually surfaced at runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@evoseal/services/monitoring_dashboard.py` around lines 48 - 77, Add a runtime warning in the initializer after assigning allowed_origins when both auth_token is set and allowed_origins explicitly contains "*". Use the existing logger warning pattern, clearly identify the insecure wildcard-CORS/auth combination, and leave other origin configurations unchanged.
99-113: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse constant-time comparison for token checks.
auth_header == f"Bearer {self.auth_token}"andtoken != self.auth_tokencompare request-controlled values against a secret with standard Python equality/inequality, which can leak matching-prefix information through timing; usesecrets.compare_digesthere.🔒 Proposed fix
+import secrets + `@web.middleware` async def _auth_middleware(self, request: web.Request, handler): """Reject unauthenticated API/WebSocket requests when auth_token is set.""" if self.auth_token and request.path != "/": auth_header = request.headers.get("Authorization", "") - if auth_header == f"Bearer {self.auth_token}": + if secrets.compare_digest(auth_header, f"Bearer {self.auth_token}"): pass elif request.path == "/ws": # WebSocket clients can pass ?token= as query param token = request.query.get("token", "") - if token != self.auth_token: + if not secrets.compare_digest(token, self.auth_token): return web.json_response({"error": "Unauthorized"}, status=401) else: return web.json_response({"error": "Unauthorized"}, status=401) return await handler(request)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@evoseal/services/monitoring_dashboard.py` around lines 99 - 113, Update _auth_middleware to use secrets.compare_digest for both the Authorization bearer token and WebSocket query token comparisons against self.auth_token, preserving the existing unauthorized responses and request-path behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evoseal/services/monitoring_dashboard.py`:
- Around line 62-73: Update the security warning condition in the dashboard
setup to trigger for both wildcard bind addresses, "0.0.0.0" and "::". Keep the
existing warning message and default CORS origin handling unchanged.
---
Nitpick comments:
In `@evoseal/services/monitoring_dashboard.py`:
- Around line 48-77: Add a runtime warning in the initializer after assigning
allowed_origins when both auth_token is set and allowed_origins explicitly
contains "*". Use the existing logger warning pattern, clearly identify the
insecure wildcard-CORS/auth combination, and leave other origin configurations
unchanged.
- Around line 99-113: Update _auth_middleware to use secrets.compare_digest for
both the Authorization bearer token and WebSocket query token comparisons
against self.auth_token, preserving the existing unauthorized responses and
request-path behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73db3fa7-8807-4991-abfe-40060a43528f
📒 Files selected for processing (3)
TODO.mdevoseal/services/monitoring_dashboard.pytests/unit/services/test_monitoring_dashboard.py
- Use hmac.compare_digest for constant-time token comparison (prevents timing attacks on bearer-token and WS query-token checks) - Exempt OPTIONS requests from auth so CORS preflight works when auth_token is set - Warn at runtime when allowed_origins=['*'] is combined with auth_token (documented guard rail now enforced) - Extend bind-address warning to include '::' wildcard alongside 0.0.0.0
|
Addressed the review feedback. Here is the point-by-point verdict and changes: 1. Constant-time token comparison ✅ Fixed
2. OPTIONS preflight not exempted ✅ Fixed
3.
4.
Verification: |
…p:// default Address review feedback on PR #109: - Change the check from a warning to a ValueError, so operators cannot silently end up with credentialed cross-origin access wide open. - Document that default allowed_origins use http:// and that explicit origins are needed when behind TLS termination or alternate hostnames. - Update test to expect ValueError instead of warning. Points assessed but not changed: - WebSocket ?token= query param: standard pattern since browsers cannot set custom headers on WebSocket connections; no change needed. - Dashboard page auth_token leakage: _generate_dashboard_html() never interpolates the token into HTML/JS; not a real issue.
|
Addressed review feedback (commit 0c20042): Changed:
Assessed but not changed:
|
- WebSocket auth now prefers Sec-WebSocket-Protocol header over query string to avoid leaking the bearer token in access/proxy logs. The HTML JS client sends the token via the protocol subprotocol and falls back to Authorization header for the HTTP API polling. The ?token= query param is still accepted as a fallback but documented as less secure. - hmac.compare_digest replaced with _constant_compare that encodes both operands to UTF-8 bytes, preventing TypeError on non-ASCII tokens or malformed Authorization headers. - Removed bare http://localhost (no port) from default CORS origins for 0.0.0.0/:: binds — it only matches port 80 and is dead config for typical non-80 dashboard ports. - Added tests: WS auth via Sec-WebSocket-Protocol (accept + reject), non-ASCII token round-trip, _constant_compare unit test, and assertion that bare http://localhost is excluded from defaults. Point 2 (wildcard bind warning vs error): left as a warning. Raising would break legitimate behind-firewall deployments; the warning is clear and actionable. Point 5 (CORS per-origin defaults): verified aiohttp_cors behaves identically with per-origin keys vs '*' for cors.add(route). No change needed.
|
Addressed the review feedback. Here's my assessment of each point and what changed: 1. WebSocket token in query string — ✅ FixedThe The embedded JavaScript client now reads the token from the page URL query param ( 2. Wildcard bind only logs warning — ⏭️ Intentionally unchangedRaising an error on 3.
|
…y token - Replace 'Bearer <token>' subprotocol with 'bearer.<token>' in JS client (spaces are invalid in Sec-WebSocket-Protocol per RFC 6455 token grammar; browsers threw SyntaxError before onerror/onclose attached, silently killing all live WebSocket updates when auth_token was set) - Wrap WebSocket constructor in try/catch so malformed subprotocol doesn't hard-fail silently - Update _auth_middleware to parse 'bearer.<token>' instead of 'Bearer <token>' - Validate auth_token is not empty string in constructor (previously falsy check silently disabled auth for auth_token='') - Update tests to use valid 'bearer.<token>' format - Add test for legacy 'Bearer ' format rejection - Add test for empty auth_token ValueError
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evoseal/services/monitoring_dashboard.py`:
- Around line 635-644: Update the token-loading logic near the WebSocket setup
to read browser bearer tokens from location.hash rather than the query string,
parse the fragment’s token value, and remove it with history.replaceState after
reading. Preserve ?token= only for the documented non-browser WebSocket
fallback, and add a regression check asserting the generated client uses
location.hash instead of location.search.
- Around line 644-648: Update the browser WebSocket authentication flow around
the WebSocket constructor to encode auth_token as an ASCII-safe base64url
representation of its UTF-8 bytes before placing it in the bearer subprotocol.
Update the server-side WebSocket authentication handling to base64url-decode and
UTF-8-decode that value before constant-time comparison with auth_token,
preserving existing behavior for valid ASCII tokens. Add a regression test
covering a non-ASCII credential and successful WebSocket
connection/reconnection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a1ae841-2a10-48c5-be7c-5b64e3a17ba4
📒 Files selected for processing (2)
evoseal/services/monitoring_dashboard.pytests/unit/services/test_monitoring_dashboard.py
| // Read token from page URL query param (set by the operator). | ||
| // The token is sent via Sec-WebSocket-Protocol header so it does | ||
| // not appear in server/proxy access logs. | ||
| if (!authToken) { | ||
| const params = new URLSearchParams(window.location.search); | ||
| authToken = params.get('token'); | ||
| } | ||
|
|
||
| try { | ||
| ws = new WebSocket(wsUrl, authToken ? ['bearer.' + authToken] : undefined); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not load the bearer token from the page query string.
Opening /?token=... sends the secret in the initial dashboard request and access logs, undermining the intended URL-safe WebSocket flow. Read it from #token=... instead, then remove the fragment with history.replaceState; retain ?token= only as the documented non-browser WebSocket fallback. Add a regression check that the generated client reads location.hash, not location.search. HTTP fragments are not sent in requests, and are stripped from Referer values. (w3.org)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@evoseal/services/monitoring_dashboard.py` around lines 635 - 644, Update the
token-loading logic near the WebSocket setup to read browser bearer tokens from
location.hash rather than the query string, parse the fragment’s token value,
and remove it with history.replaceState after reading. Preserve ?token= only for
the documented non-browser WebSocket fallback, and add a regression check
asserting the generated client uses location.hash instead of location.search.
Source: Coding guidelines
| ws = new WebSocket(wsUrl, authToken ? ['bearer.' + authToken] : undefined); | ||
| } catch (e) { | ||
| console.error('WebSocket constructor failed (bad subprotocol?):', e); | ||
| addLogEntry('WebSocket setup error: ' + e.message, 'error'); | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'new WebSocket|location\.(search|hash)|bearer\.' \
evoseal/services/monitoring_dashboard.py
rg -n -C2 'non_ascii.*ws|ws_auth.*ascii|bearer\.' \
tests/unit/services/test_monitoring_dashboard.pyRepository: SHA888/EVOSEAL
Length of output: 1720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== monitoring dashboard outline/size =="
wc -l evoseal/services/monitoring_dashboard.py
ast-grep outline evoseal/services/monitoring_dashboard.py --depth 3 --view expanded | sed -n '1,220p' || true
echo "== relevant monitoring dashboard sections =="
sed -n '1,220p' evoseal/services/monitoring_dashboard.py
sed -n '580,670p' evoseal/services/monitoring_dashboard.py
echo "== test monitoring dashboard relevant sections =="
sed -n '130,210p' tests/unit/services/test_monitoring_dashboard.py
echo "== occurrences of auth_token/authToken/url token config =="
rg -n -C2 "auth_token|authToken|SEAL_AUTH_TOKEN|SECRETCODE|bearer\\.\\+|new WebSocket|Sec-WebSocket-Protocol|subprotocol|sec_websocket_token" evoseal testsRepository: SHA888/EVOSEAL
Length of output: 35871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import base64
import urllib.parse
import hmac
non_ascii = "über-sécret"
plain = "bearer." + non_ascii
b64 = base64.urlsafe_b64encode(non_ascii.encode("utf-8")).decode("ascii").rstrip("=")
proto_b64encoded_token = "bearer." + b64
decode_b64_token = base64.urlsafe_b64decode(proto_b64encoded_token[len("bearer."):] + b"==").decode("utf-8")
assert hmac.compare_digest(decode_b64_token.encode("utf-8"), non_ascii.encode("utf-8"))
print("plain_proto", plain)
print("base64url_proto", proto_b64encoded_token)
print("decoded_correct", decode_b64_token == non_ascii)
PY
sed -n '210,240p' tests/unit/services/test_monitoring_dashboard.pyRepository: SHA888/EVOSEAL
Length of output: 282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import base64
import hmac
non_ascii = "über-sécret"
plain = "bearer." + non_ascii
b64 = base64.urlsafe_b64encode(non_ascii.encode("utf-8")).decode("ascii").rstrip("=")
proto_b64encoded_token = "bearer." + b64
decode_b64_token = base64.urlsafe_b64decode(proto_b64encoded_token[len("bearer."):] + b"==").decode("utf-8")
assert hmac.compare_digest(decode_b64_token.encode("utf-8"), non_ascii.encode("utf-8"))
print("plain_proto", plain)
print("base64url_proto", proto_b64encoded_token)
print("decoded_correct", decode_b64_token == non_ascii)
PY
sed -n '210,242p' tests/unit/services/test_monitoring_dashboard.pyRepository: SHA888/EVOSEAL
Length of output: 282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 <<'PY'
from __future__ import annotations
import base64
import hmac
non_ascii = "über-sécret"
plain = "bearer." + non_ascii
b64 = base64.urlsafe_b64encode(non_ascii.encode("utf-8")).decode("ascii").rstrip("=")
proto_b64encoded_token = "bearer." + b64
encoded = proto_b64encoded_token[len("bearer."):]
padding_needed = (4 - len(encoded) % 4) % 4
decode_b64_token = base64.urlsafe_b64decode(encoded + "=" * padding_needed).decode("utf-8")
assert hmac.compare_digest(decode_b64_token.encode("utf-8"), non_ascii.encode("utf-8"))
print("plain_proto", repr(plain))
print("plain_proto_ascii", plain.isascii())
print("base64url_proto", repr(proto_b64encoded_token))
print("base64url_proto_ascii", proto_b64encoded_token.isascii())
print("decoded_correct", decode_b64_token == non_ascii)
PY
sed -n '210,242p' tests/unit/services/test_monitoring_dashboard.pyRepository: SHA888/EVOSEAL
Length of output: 1436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 <<'PY'
import io
import http.client
from urllib.parse import urlencode
from aiohttp import web
import asyncio
loop = asyncio.new_event_loop()
asyncio.set_event_loop(loop)
app = web.Application()
routes = web.RouteTableDef()
`@routes.get`("/")
async def dashboard_page(request):
return web.Response(text='<html></html>', content_type="text/html")
`@routes.get`("/ws")
async def websocket_handler(request):
ws = web.WebSocketResponse()
try:
await ws.prepare(request)
await ws.close()
except Exception as exc:
return web.Response(text=f"WebSocket prepare raised {type(exc).__name__}: {exc}")
return ws
app.router.add_routes(routes)
runner = web.WebAppRunner(app, port=18999)
`@asyncio.coroutine`
def run():
yield from runner.setup()
yield from startup_site()
yield from asyncio.sleep(0.1)
asyncio.set_event_loop(loop)
loop.run_until_complete(run())Repository: SHA888/EVOSEAL
Length of output: 359
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 <<'PY'
from pathlib import Path
from urllib.parse import urlparse
dashboard = Path("evoseal/services/monitoring_dashboard.py").read_text()
search = Path("tests/").glob("*/services/test_monitoring_dashboard.py")
files = next(iter(Path("tests").rglob("*test_monitoring_dashboard.py")), None)
print("dashboard_path_exists", Path("evoseal/services/monitoring_dashboard.py").exists())
print("dashboard_path", Path("evoseal/services/monitoring_dashboard.py").absolute())
print("dashboard_contains_new_websocket", "new WebSocket(wsUrl, authToken ? ['bearer.' + authToken] : undefined)" in dashboard)
print("dashboard_contains_restricted_ws_auth", "--restricted-ws-auth" in dashboard)
print("dashboard_route_ws", "self.app.router.add_get(\"/ws\", self.websocket_handler)" in dashboard)
print("server_authorization_header", "Bearer {self.auth_token}" in dashboard)
print("server_auth_from_protocol", "token_from_protocol" in dashboard)
print("test_file", files)
if files:
txt = files.read_text()
print("test_contains_ws_non_ascii", "test_ws_auth_via_sec_protocol_header" in txt and "auth_token='über-sécret'" in txt)
print("test_contains_non_ascii_ws_subprotocol", "test_non_ascii_token_does_not_500" in txt and "Sec-WebSocket-Protocol" in txt and "über-sécret" in txt)
PY
sed -n '305,365p' tests/unit/services/test_monitoring_dashboard.py 2>/dev/null || trueRepository: SHA888/EVOSEAL
Length of output: 2235
Encode browser WebSocket credentials into a valid subprotocol token.
auth_token supports non-ASCII values, but bearer. + über-sécret is not a valid RFC 6455 WebSocket subprotocol value; the browser constructor throws, this catch returns without reconnecting, and real-time updates fail on non-ASCII credentials. Encode auth tokens as an ASCII-only transport such as base64url-encoded UTF-8, decode server-side before constant-time comparison, and add a non-ASCII browser WebSocket regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@evoseal/services/monitoring_dashboard.py` around lines 644 - 648, Update the
browser WebSocket authentication flow around the WebSocket constructor to encode
auth_token as an ASCII-safe base64url representation of its UTF-8 bytes before
placing it in the bearer subprotocol. Update the server-side WebSocket
authentication handling to base64url-decode and UTF-8-decode that value before
constant-time comparison with auth_token, preserving existing behavior for valid
ASCII tokens. Add a regression test covering a non-ASCII credential and
successful WebSocket connection/reconnection.
Source: Coding guidelines
- Bracket IPv6 literals in default CORS origin (http://[::1]:8081) - Compute allow_credentials per-origin (wildcard=False, explicit=True) - Echo accepted bearer subprotocol in WebSocket handler (RFC 6455 §4.2.2) - Validate auth_token charset against HTTP token grammar (RFC 7230 §3.2.6) - Update non-ASCII token test to expect ValueError (also invalid tchar) - Add tests for IPv6 origins, per-origin CORS, WS subprotocol echo, token charset
|
Addressed all four review points: 1. IPv6 default-origin bracketing — Added helper using . Now produces instead of the invalid . Tests: , , . **2. Per-origin ** — Replaced the single shared with per-origin computation in . Wildcard gets ; explicit origins keep . Test: . 3. WebSocket subprotocol echo — now reads the header, extracts the subprotocol, and passes it to so browsers complete the handshake per RFC 6455 §4.2.2. Tests: , . 4. Token charset validation — Added regex check (RFC 7230 §3.2.6 tchar) in . Tokens containing , All 32 tests pass. Lint clean. |
|
Addressed all four review points: 1. IPv6 default-origin bracketing — Added 2. Per-origin 3. WebSocket subprotocol echo — 4. Token charset validation — Added All 32 tests pass. Lint clean. |
# Conflicts: # TODO.md
21ebe0a to
f2b6534
Compare
Rewrite docs/api/index.md to document the real public API surface: - CLI commands (init, config, pipeline, start, doctor, estimate-cost, etc.) - Core Python API (EvolutionPipeline, EvolutionConfig) - Safety layer (SafetyIntegration, CheckpointManager, EditScopeValidator, etc.) - Phase 3 services (ContinuousEvolutionService, TrainingManager) - Monitoring dashboard quick reference with auth/CORS info The previous version documented fabricated classes (EVOSEAL.evolve(), OpenAIModel, AnthropicModel) that don't exist in the codebase. Also fix stale CORS and auth info in docs/API_REFERENCE.md: - Auth: document optional bearer token (added in PR #109) - CORS: origins now default to host:port, not wildcard * Addresses TODO.md item: Improve API reference
1 participant
What
Hardens
MonitoringDashboardCORS and adds optional bearer-token authentication.Why
The monitoring dashboard had two security issues flagged in the whole-repo review (2026-07-22):
setup_cors()used wildcard*origin withallow_credentials=True, meaning any website could make credentialed requests to internal operational endpoints.Changes
host:portinstead of*. Binding to0.0.0.0logs a security warning. When*is explicitly passed,allow_credentialsis forcedFalseper the CORS spec.auth_tokenparameter. When set, all/api/*and/wsrequests must presentAuthorization: Bearer <token>. WebSocket clients can pass?token=as a query param. The HTML dashboard page (/) is not gated.0.0.0.0warning, and constructor parameter storage.Addresses the P1 TODO item: "Monitoring dashboard has no authentication and permissive CORS-with-credentials".
Summary by CodeRabbit
Authorization: Bearer <token>andSec-WebSocket-Protocol/?token=).OPTIONS) is supported while keeping the dashboard landing page ungated.auth_tokenvalues.