Skip to content

fix: harden security — configurable CORS origin, role escalation, unauthenticated seed #26

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

Open
zikunz wants to merge 1 commit into
base: main
Choose a base branch
from
+11 −4
Open

fix: harden security — configurable CORS origin, role escalation, unauthenticated seed #26

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 11 additions & 4 deletions src/worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,8 @@ def verify_token(raw: str, secret: str):
# Response helpers
# ---------------------------------------------------------------------------

_CORS_ORIGIN_INITIALISED = False
_CORS = {
"Access-Control-Allow-Origin": "*",
"Access-Control-Allow-Methods": "GET, POST, PUT, DELETE, OPTIONS",
"Access-Control-Allow-Headers": "Content-Type, Authorization",
}
Expand Down Expand Up @@ -870,12 +870,11 @@ async def api_join(req, env):
return bad_resp

act_id = body.get("activity_id")
role = (body.get("role") or "participant").strip()

if not act_id:
return err("activity_id is required")
if role not in ("participant", "instructor", "organizer"):
role = "participant"

role = "participant"

act = await env.DB.prepare(
"SELECT id FROM activities WHERE id=?"
Expand Down Expand Up @@ -1134,6 +1133,12 @@ async def serve_static(path: str, env):
# ---------------------------------------------------------------------------

async def _dispatch(request, env):
global _CORS_ORIGIN_INITIALISED
if not _CORS_ORIGIN_INITIALISED:
_CORS_ORIGIN_INITIALISED = True
origin = getattr(env, "ALLOWED_ORIGIN", "")
_CORS["Access-Control-Allow-Origin"] = origin if origin else "*"

Comment on lines +1136 to +1141
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider avoiding global mutable state for CORS initialization.

The current approach works but has a few considerations worth noting:

  1. Static analysis flag (PLW0603): The global statement modifying _CORS_ORIGIN_INITIALISED is discouraged in Python best practices.
  2. One-shot initialization: The origin is read once per worker isolate lifetime. If env.ALLOWED_ORIGIN could vary (e.g., during hot config updates), this pattern won't pick up changes.
  3. Dict mutation timing: Concurrent requests could theoretically both enter the initialization block before the flag is set, though in practice this is benign since both would write the same value.

A cleaner approach would be to compute the headers at response time or use a helper that returns the complete headers dict:

♻️ Suggested refactor: Compute CORS headers per-request 
-_CORS_ORIGIN_INITIALISED = False
 _CORS = {
+    "Access-Control-Allow-Origin": "*",  # default, overridden by _cors_headers()
     "Access-Control-Allow-Methods": "GET, POST, PUT, DELETE, OPTIONS",
     "Access-Control-Allow-Headers": "Content-Type, Authorization",
 }
+
+def _cors_headers(env) -> dict:
+    """Return CORS headers with origin from env (or fallback to *)."""
+    origin = getattr(env, "ALLOWED_ORIGIN", "") or "*"
+    return {**_CORS, "Access-Control-Allow-Origin": origin}

Then update response helpers (json_resp, _unauthorized_basic, serve_static) to accept env and use _cors_headers(env).

That said, if simplicity and minimal code churn are priorities, the current implementation is functional and achieves the security goal. The fallback to "*" is documented and intentional per the PR objectives.

🧰 Tools
🪛 Ruff (0.15.7)

[warning] 1136-1136: Using the global statement to update _CORS_ORIGIN_INITIALISED is discouraged

(PLW0603)

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

In `@src/worker.py` around lines 1136 - 1141, The module currently uses a global
mutable flag _CORS_ORIGIN_INITIALISED and mutates the shared _CORS dict during
import-time which triggers the PLW0603 warning and prevents picking up runtime
changes to env.ALLOWED_ORIGIN; replace this by adding a small helper function
_cors_headers(env) that returns a new headers dict (copying existing _CORS base
entries and setting "Access-Control-Allow-Origin" to env.ALLOWED_ORIGIN or "*"
if empty) and update all response helpers (json_resp, _unauthorized_basic,
serve_static) to call _cors_headers(env) per-request instead of relying on
_CORS/_CORS_ORIGIN_INITIALISED so there is no global state mutation and hot
config changes are honored.

Copy link
Copy Markdown
Author

@zikunz zikunz Mar 26, 2026

Choose a reason for hiding this comment

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

Thanks for the suggestion. I considered the per-request approach but it would require threading env through json_resp, ok, err, _unauthorized_basic, and serve_static, touching many call sites. For a focused security fix, I preferred minimal churn. I am happy to refactor in a follow-up if the maintainers prefer that pattern.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 26, 2026

Choose a reason for hiding this comment

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

@zikunz — That's a completely reasonable trade-off. Keeping the diff focused on the security fix rather than introducing a broad refactor across multiple helpers is a sound engineering decision. The current implementation is functional and achieves the intended goal.

If you'd like, I can open a follow-up issue to track the per-request CORS header refactor so it doesn't get lost. Just let me know!

path = urlparse(request.url).path
method = request.method.upper()
admin_path = _clean_path(getattr(env, "ADMIN_URL", ""))
Expand All @@ -1156,6 +1161,8 @@ async def _dispatch(request, env):
return err("Database init failed — check D1 binding", 500)

if path == "/api/seed" and method == "POST":
if not _is_basic_auth_valid(request, env):
return _unauthorized_basic()
try:
await init_db(env)
await seed_db(env, env.ENCRYPTION_KEY)
Expand Down