fix: harden security — configurable CORS origin, role escalation, unauthenticated seed#26
fix: harden security — configurable CORS origin, role escalation, unauthenticated seed#26zikunz wants to merge 3 commits intoalphaonelabs:mainfrom
Conversation
…uthenticated seed Read the Access-Control-Allow-Origin value from the ALLOWED_ORIGIN environment variable at runtime instead of hardcoding the wildcard (*). When the variable is not configured the wildcard is preserved as a fallback so existing deployments are not disrupted. Force the enrollment role to "participant" in api_join. Previously a user could self-assign "instructor" or "organizer" by including a role field in the request body. Require HTTP Basic Auth on the /api/seed endpoint to prevent unauthenticated callers from overwriting the database with sample data. /api/init is left open because it is idempotent (CREATE TABLE IF NOT EXISTS) and is needed for first-time bootstrap before admin credentials are configured.
WalkthroughRemoved request-provided role handling in join requests (always set to "participant"), moved CORS origin initialization to per-worker lazy setup using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker.py`:
- Around line 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.
🪄 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: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4fa9601-e34d-4905-8f69-3a174425d8d7
📒 Files selected for processing (1)
src/worker.py
There was a problem hiding this comment.
Pull request overview
This PR hardens security in the Cloudflare Worker API (src/worker.py) by tightening CORS behavior, preventing user-driven role escalation during enrollment, and protecting the database seeding endpoint with admin Basic Auth.
Changes:
- Make
Access-Control-Allow-Originconfigurable viaALLOWED_ORIGIN(with a fallback behavior). - Force
/api/joinenrollments to always use theparticipantrole. - Require admin HTTP Basic Auth for
POST /api/seed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Match the (getattr(...) or "").strip() pattern already used for ADMIN_BASIC_USER and ADMIN_BASIC_PASS in _is_basic_auth_valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker.py`:
- Line 196: Add regression tests covering the three security-sensitive branches
once test infrastructure (e.g., pytest) is available: test the ALLOWED_ORIGIN
fallback and whitespace-stripping logic (use
CORS_ORIGIN_INITIALISED/ALLOWED_ORIGIN behavior), test that the /api/join
endpoint forces the participant role (assert the returned user/role is
participant even if request attempts to set another role), and test that
/api/seed requires valid basic auth (assert 401 when missing/invalid credentials
and success when valid). Implement focused unit/integration tests calling the
route handlers or test client to exercise these specific branches to prevent
regressions.
🪄 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: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53dfb936-f84c-4cb9-8146-b5ee24362621
📒 Files selected for processing (1)
src/worker.py
|
@A1L13N All review feedback has been addressed and resolved. Would appreciate your review when you get a chance. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/worker.py (1)
1364-1373: 🧹 Nitpick | 🔵 TrivialNice gating of
/api/seed— and good call leaving/api/initopen. 🔐Requiring Basic Auth before
init_db+seed_dbcloses the unauthenticated-overwrite hole cleanly, and_is_basic_auth_validalready does the right things (rejects whenADMIN_BASIC_USER/ADMIN_BASIC_PASSare unset or blank, useshmac.compare_digestfor timing-safe comparison, decodes carefully inside a try/except). The reasoning in the PR description for keeping/api/initunauthenticated — idempotentCREATE TABLE IF NOT EXISTSonly, needed to bootstrap before admin creds exist — is sound.One small operational thought for the future (not blocking): once you have structured logging in place, it'd be worth emitting a warn-level record on failed
/api/seedauth attempts so abuse is visible in whatever log sink you're using. Totally fine as a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1364 - 1373, Add a warn-level log when a client fails Basic Auth on the /api/seed path: inside the handler where you call _is_basic_auth_valid(request, env) and you currently return _unauthorized_basic(), emit a warning (e.g., via the app/logger available as env.logger or similar) recording the endpoint "/api/seed", the fact that auth failed, and minimal request context (e.g., remote IP or headers) before returning; keep the existing flow and do not log sensitive secrets, and leave init_db, seed_db, capture_exception, and _unauthorized_basic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker.py`:
- Around line 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.
---
Outside diff comments:
In `@src/worker.py`:
- Around line 1364-1373: Add a warn-level log when a client fails Basic Auth on
the /api/seed path: inside the handler where you call
_is_basic_auth_valid(request, env) and you currently return
_unauthorized_basic(), emit a warning (e.g., via the app/logger available as
env.logger or similar) recording the endpoint "/api/seed", the fact that auth
failed, and minimal request context (e.g., remote IP or headers) before
returning; keep the existing flow and do not log sensitive secrets, and leave
init_db, seed_db, capture_exception, and _unauthorized_basic unchanged.
🪄 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: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7e97f70-94fd-4bb3-9742-e0c9796670c4
📒 Files selected for processing (1)
src/worker.py
| 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" |
There was a problem hiding this comment.
🧹 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.
What
Three security fixes in
src/worker.py:Configurable CORS origin —
Access-Control-Allow-Originwas hardcoded to*, allowing any website to read authenticated API responses (e.g. user dashboard data). The origin is now loaded from theALLOWED_ORIGINenv var at runtime. When the variable is not set, the wildcard is preserved as a fallback so existing deployments are not disrupted.Self-assigned privileged roles —
POST /api/joinaccepted a user-suppliedrolefield, allowing any authenticated user to enroll asinstructorororganizer. The role is now hardcoded toparticipant.Unauthenticated
/api/seed— Anyone couldPOST /api/seedwithout credentials and overwrite the database with sample data. The endpoint now requires the same HTTP Basic Auth used by the admin panel (ADMIN_BASIC_USER/ADMIN_BASIC_PASS)./api/initremains open because it only runs idempotentCREATE TABLE IF NOT EXISTSstatements and is needed to bootstrap a fresh deployment before admin credentials exist.Reproduction
All three issues were reproduced locally against
wrangler devonlocalhost:8787.CORS origin:
Role escalation:
Unauthenticated seed:
Configuration
After this change, set
ALLOWED_ORIGINfor cross-origin access:ALLOWED_ORIGIN=http://localhost:8787to.dev.varswrangler secret put ALLOWED_ORIGINSame-origin requests (frontend served by the same worker) do not need CORS headers and work without this variable. When
ALLOWED_ORIGINis not configured, the wildcard*is used as a fallback to avoid breaking existing deployments.Security Hardening: Configurable CORS, Prevent Role Escalation, and Authenticated Seeding
This PR applies three targeted security fixes in src/worker.py to reduce attack surface and tighten access control.
Configurable CORS origin
Prevent role escalation
Require auth for seeding
Other notes
Impact: strengthens authorization and access controls with small behavioral changes that prevent CORS misconfiguration, role escalation, and unauthenticated data seeding.