Skip to content

ECHO-697 add CORS headers to public stats api #459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ussaama merged 2 commits into from
Mar 3, 2026
Merged

ECHO-697 add CORS headers to public stats api #459

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0faa1c8
- add CORS headers to public stats api
ussaama Feb 23, 2026
bcb0c45
Merge branch 'main' into cors-headers-public-stats
ussaama Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions echo/server/dembrane/api/stats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from fastapi import Request, APIRouter, HTTPException
from pydantic import BaseModel
from fastapi.responses import JSONResponse

from dembrane.directus import directus
from dembrane.redis_async import get_redis_client
Expand Down Expand Up @@ -187,8 +188,22 @@ async def _release_lock() -> None:
logger.warning("Lock release error: %s", e)


_PUBLIC_CORS_HEADERS = {
"Access-Control-Allow-Origin": "*",
"Access-Control-Allow-Methods": "GET, OPTIONS",
"Access-Control-Allow-Headers": "*",
"Access-Control-Max-Age": "86400",
}
Comment on lines +191 to +196
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider using FastAPI's CORSMiddleware instead of hand-rolling headers.

Manual CORS is fragile — every response path (including error handlers) needs these headers or browsers will block the response. FastAPI's built-in CORSMiddleware handles this automatically, including error responses and preflight. If you only need CORS on this single router, you can apply the middleware selectively or use a sub-application.

That said, if there's a deliberate reason to keep it manual (e.g., no middleware access at this layer), then the immediate fix is ensuring all response paths include these headers — see the 503 comment below.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@echo/server/dembrane/api/stats.py` around lines 191 - 196, The code defines a
hand-rolled _PUBLIC_CORS_HEADERS constant which is fragile—replace this by
enabling FastAPI's CORSMiddleware for the app or for the specific
sub-application/router that serves the endpoints in this module so CORS and
preflight are applied to every response (including errors) automatically; if you
cannot add middleware at the app level, create a sub-app with FastAPI(), mount
the router there, and add fastapi.middleware.cors.CORSMiddleware with
allow_origins=["*"], allow_methods=["GET","OPTIONS"], allow_headers=["*"],
max_age=86400 instead of using _PUBLIC_CORS_HEADERS, or as a fallback ensure
every response path and error handler (e.g., the 503 path mentioned) explicitly
merges _PUBLIC_CORS_HEADERS into the Response headers.



@StatsRouter.options("/")
async def stats_preflight() -> JSONResponse:
"""Handle CORS preflight for the public stats endpoint."""
return JSONResponse(content=None, headers=_PUBLIC_CORS_HEADERS)
Comment on lines +199 to +202
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Nit: preflight responses conventionally use 204 No Content.

content=None serializes to a JSON null body with a 200 status. The canonical preflight response is 204 with no body. Not a correctness issue — browsers don't care — but it's cleaner.

✨ Proposed tweak 
 `@StatsRouter.options`("/")
 async def stats_preflight() -> JSONResponse:
     """Handle CORS preflight for the public stats endpoint."""
-    return JSONResponse(content=None, headers=_PUBLIC_CORS_HEADERS)
+    return JSONResponse(content=None, status_code=204, headers=_PUBLIC_CORS_HEADERS)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@StatsRouter.options("/")
async def stats_preflight() -> JSONResponse:
"""Handle CORS preflight for the public stats endpoint."""
return JSONResponse(content=None, headers=_PUBLIC_CORS_HEADERS)
`@StatsRouter.options`("/")
async def stats_preflight() -> JSONResponse:
"""Handle CORS preflight for the public stats endpoint."""
return JSONResponse(content=None, status_code=204, headers=_PUBLIC_CORS_HEADERS)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@echo/server/dembrane/api/stats.py` around lines 199 - 202, The preflight
handler stats_preflight currently returns a JSONResponse with content=None which
yields a 200/JSON null body; change it to return an empty 204 No Content
response instead (use starlette.responses.Response or FastAPI Response) with
status_code=204 and the same _PUBLIC_CORS_HEADERS so the endpoint sends no body;
update the return in the StatsRouter.options("/") handler (stats_preflight)
accordingly.



@StatsRouter.get("/", response_model=StatsResponse)
async def get_public_stats(request: Request) -> StatsResponse:
async def get_public_stats(request: Request) -> JSONResponse:
"""
Public endpoint returning aggregate platform statistics.
Rate-limited to 10 requests per IP per minute.
Expand All @@ -201,20 +216,20 @@ async def get_public_stats(request: Request) -> StatsResponse:
# Check cache first
cached = await _get_cached_stats()
if cached is not None:
return cached
return JSONResponse(content=cached.model_dump(), headers=_PUBLIC_CORS_HEADERS)

# Cache miss — try to acquire lock to prevent stampede
if await _acquire_lock():
try:
# Double-check cache (another request may have populated it)
cached = await _get_cached_stats()
if cached is not None:
return cached
return JSONResponse(content=cached.model_dump(), headers=_PUBLIC_CORS_HEADERS)

# Compute and cache fresh stats
stats = await _compute_stats()
await _set_cached_stats(stats)
return stats
return JSONResponse(content=stats.model_dump(), headers=_PUBLIC_CORS_HEADERS)
finally:
await _release_lock()
else:
Expand All @@ -223,7 +238,7 @@ async def get_public_stats(request: Request) -> StatsResponse:
await asyncio.sleep(0.5)
cached = await _get_cached_stats()
if cached is not None:
return cached
return JSONResponse(content=cached.model_dump(), headers=_PUBLIC_CORS_HEADERS)

# Lock holder likely failed — return 503 instead of stampeding Directus
logger.warning("Stats computation timed out waiting for lock holder")
Expand Down
Loading