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 3 commits 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
3 commits
Select commit Hold shift + click to select a range
8008752
fix: harden security — configurable CORS origin, role escalation, una…
zikunz Mar 26, 2026
94b21f3
fix: strip whitespace from ALLOWED_ORIGIN env var
zikunz Apr 12, 2026
a9fd543
Merge branch 'main' into fix/security-hardening
zikunz Apr 17, 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
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 @@ -356,8 +356,8 @@ def verify_token(raw: str, secret: str):
# Response helpers
# ---------------------------------------------------------------------------

_CORS_ORIGIN_INITIALISED = False
Comment thread
zikunz marked this conversation as resolved.
_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 @@ -1061,12 +1061,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"
Comment on lines 1063 to +1068
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 17, 2026

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Privilege-escalation fix looks solid 🛡️ — please refresh the stale join tests.

Hardcoding role = "participant" and dropping any client-supplied value is exactly the right move here; an authenticated user can no longer self-promote to instructor/organizer via the join endpoint.

One downstream cleanup: the existing tests in tests/test_api_join_dashboard.py (test_invalid_role_defaults_to_participant and test_valid_role_instructor) still POST a role field and only assert status == 200. After this change they pass vacuously — the request body's role is now silently ignored, so neither test actually verifies any role-selection behavior, and test_valid_role_instructor is now misleadingly named (an instructor role can no longer be obtained through /api/join). Per the repo's Python guideline to "verify tests cover the key logic paths," it'd be worth tightening these to assert the persisted enrollment row's role is "participant" regardless of what the client sent — that way the test suite actively guards the new invariant instead of rubber-stamping it.

🧪 Sketch of the hardened assertion 
async def test_join_ignores_client_supplied_role(self):
    tok = _token()
    act_row = MockRow(id="act-1")
    insert_stmt = make_stmt()  # capture bound params
    env = make_env(db=MockDB([make_stmt(first=act_row), insert_stmt]))
    r = await worker.api_join(
        self._req({"activity_id": "act-1", "role": "instructor"}, token=tok), env
    )
    assert r.status == 200
    # The 4th bound parameter to the INSERT is the role column.
    assert insert_stmt.bound_args[-1] == "participant"

As per coding guidelines: "Verify tests cover the key logic paths."

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

In `@src/worker.py` around lines 1063 - 1068, Update the tests in
tests/test_api_join_dashboard.py to assert that the persisted enrollment row
uses role "participant" regardless of client input: change the existing
test_invalid_role_defaults_to_participant and test_valid_role_instructor to call
worker.api_join (or the helper _req) with a client-supplied "role" (e.g.,
"instructor"), then capture the INSERT statement (or insert_stmt.bound_args) and
assert its role parameter equals "participant"; keep the request/status checks
but add the DB-bound-parameter assertion so api_join (which hardcodes role =
"participant" in worker.py) is explicitly verified rather than relying on a 200
status alone.


act = await env.DB.prepare(
"SELECT id FROM activities WHERE id=?"
Expand Down Expand Up @@ -1335,6 +1334,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", "") or "").strip()
_CORS["Access-Control-Allow-Origin"] = origin if origin else "*"
Comment thread
zikunz marked this conversation as resolved.

Comment thread
zikunz marked this conversation as resolved.
path = urlparse(request.url).path
method = request.method.upper()
admin_path = _clean_path(getattr(env, "ADMIN_URL", ""))
Expand All @@ -1357,6 +1362,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()
Comment thread
zikunz marked this conversation as resolved.
try:
await init_db(env)
await seed_db(env, env.ENCRYPTION_KEY)
Expand Down