Perfectly organized worker.py into 8 files total#17
Perfectly organized worker.py into 8 files total#17Shrey1006 wants to merge 15 commits intoalphaonelabs:mainfrom
Conversation
Co-authored-by: A1L13N <193832434+A1L13N@users.noreply.github.com> Agent-Logs-Url: https://github.com/alphaonelabs/learn/sessions/7e2e86c2-ca44-43eb-9ece-e502c7a54a9b
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMonolithic worker split into focused modules: security primitives, HTTP helpers, DB schema/seeding, static serving, and three API modules (auth, activities, admin). The main worker now delegates route handling to these modules while retaining centralized request dispatch. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Worker as worker.py
participant AuthAPI as src/api_auth.py
participant Sec as src/security_utils.py
participant DB as env.DB
Client->>Worker: POST /api/register {username,email,password,...}
Worker->>AuthAPI: forward request (req, env)
AuthAPI->>Sec: new_id()
AuthAPI->>Sec: blind_index(username)
AuthAPI->>Sec: hash_password(password, username)
AuthAPI->>Sec: encrypt_aes(username/email/name/role, env.JWT_SECRET)
AuthAPI->>DB: INSERT users (...)
DB-->>AuthAPI: insert result / error
AuthAPI->>Sec: create_token(id, username, role, env.JWT_SECRET)
AuthAPI-->>Worker: response { token, user }
Worker-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/api_handlers.py`:
- Around line 195-199: The api_create_activity handler currently only verifies
authentication via verify_token in api_create_activity; enforce that the
authenticated user has the host role before proceeding by checking the user's
role/roles field (e.g., user.get("role") == "host" or "host" in
user.get("roles", [])) and return an authorization error (403) if not a host;
update the guard in api_create_activity (and ensure the same host-only contract
assumed by api_create_session and api_add_activity_tags is enforced here) so
that non-host members cannot create activities that would bypass later owner
checks.
In `@src/platform_utils.py`:
- Around line 224-240: The current try/except around env.DB.prepare(...)
swallows all exceptions and records uid_map entries before the INSERT succeeds,
causing /api/seed to report success even when inserts fail; change the logic in
the seeding function so you only add to uid_map after a successful run(), and
replace the broad except Exception: pass with targeted handling: either use a
DB-side "INSERT OR IGNORE" (or ON CONFLICT DO NOTHING) in the env.DB.prepare SQL
or catch specific duplicate/key-conflict errors and log them, but for any other
exception log the full error and abort/raise so callers get a failure; update
the blocks that call env.DB.prepare and the uid_map updates (the same pattern at
the other ranges referenced) accordingly.
- Around line 410-430: The current code always falls back to serving
"index.html" when a key is missing, which returns the HTML shell for missing
assets; change the logic so you only attempt the SPA fallback for extensionless
routes. After trying env.__STATIC_CONTENT.get(key, "text"), compute ext as in
the snippet (ext = key.rsplit(".",1)[-1] if "." in key else "html") and if the
original key had an extension (i.e., "." in key) and content is None then
immediately return a 404 Response (use a plain text or appropriate MIME from
_MIME if available) instead of loading index.html; keep the existing behavior of
fetching "index.html" only when there was no extension on the requested key and
include _CORS and proper Content-Type in all Responses.
In `@src/security_utils.py`:
- Around line 71-80: The hard-coded module-level secret _PEPPER must be removed
and loaded from the worker environment at runtime: replace the literal _PEPPER
with a module-level value that is initialized once from an environment variable
(e.g. os.environ["PEPPER"]) during import/startup, validate it (non-empty,
bytes) and raise a clear exception (fail closed) if the env var is missing or
invalid; update _user_salt(username: str) to use that runtime-loaded pepper
instead of a literal so functions like _user_salt and any callers relying on
_PEPPER use the environment-derived secret.
- Around line 99-125: create_token currently signs only id/username/role and
verify_token accepts it indefinitely; update create_token to include iat
(current UNIX epoch int) and exp (iat + TTL, make TTL a configurable constant or
optional arg) in the JSON payload before base64-encoding and signing with the
same _hmac logic, and update verify_token to decode and parse iat/exp then
reject the token (return None) if current time (time.time()) is >= exp (allow a
small leeway if desired); ensure the signature is computed over the payload
containing iat/exp (use the same payload variable names in create_token and
verify_token), keep the existing "payload.sig" format, and add tests exercising
expired, valid, and malformed tokens.
- Around line 25-52: The XOR stream cipher in encrypt()/decrypt() must be
replaced with an authenticated AEAD like AES-GCM: change _derive_key(secret) to
return a 32-byte key suitable for AES-256, then in encrypt() generate a fresh
random 12-byte nonce per invocation, use AESGCM(key).encrypt(nonce,
plaintext_bytes, associated_data=None), and base64-encode the concatenation of
nonce||ciphertext for storage; in decrypt() base64-decode, split out the 12-byte
nonce and ciphertext, call AESGCM(key).decrypt(nonce, ciphertext,
associated_data=None) and decode the resulting plaintext, and keep the current
error handling (return "[decryption error]" on failure). Use a well-maintained
library (e.g., cryptography.hazmat.primitives.ciphers.aead.AESGCM) and do not
reuse nonces or the keystream.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05c47b96-cea2-46ce-9d9d-bd9c0c47eca0
📒 Files selected for processing (4)
src/api_handlers.pysrc/platform_utils.pysrc/security_utils.pysrc/worker.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/api_activities.py (2)
90-94:⚠️ Potential issue | 🟠 MajorEnforce the host role before activity creation.
src/worker.pyLines 10-16 documents this route as[host], but this handler only checks authentication. That lets any member create an activity and then satisfy the owner checks inapi_create_session()andapi_add_activity_tags()for that record.🧰 Proposed fix
async def api_create_activity(req, env): user = verify_token(req.headers.get("Authorization"), env.JWT_SECRET) if not user: return err("Authentication required", 401) + if user.get("role") != "host": + return err("Host role required", 403) body, bad_resp = await parse_json_object(req)As per coding guidelines,
**/*.py: Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api_activities.py` around lines 90 - 94, The api_create_activity handler currently only verifies authentication; enforce that the authenticated user is a host before allowing creation by checking the user's role (from the object returned by verify_token) and returning a 403 error if it is not "host" (or the equivalent host role value used in your app). Update api_create_activity to inspect the role (e.g., user["role"] or user.role) immediately after verify_token and reject non-hosts so downstream functions like api_create_session and api_add_activity_tags cannot be exploited by members.
130-154:⚠️ Potential issue | 🟠 MajorValidate
tagsbefore mutating the database, and make the upsert race-safe.Line 131 and Line 405 call
.strip()on unvalidated JSON values. A string becomes one-character tags, and any non-string element raises after earlier writes have already succeeded. The SELECT-then-INSERT flow also loses the link under concurrent requests: the losing insert hits the exception path and never reuses the tag that was just created.🧰 Suggested direction
- for tag_name in (body.get("tags") or []): - tag_name = tag_name.strip() + tags = body.get("tags") or [] + if not isinstance(tags, list) or any(not isinstance(tag, str) for tag in tags): + return err("tags must be an array of strings") + for raw_tag_name in tags: + tag_name = raw_tag_name.strip() if not tag_name: continue - t_row = await env.DB.prepare( - "SELECT id FROM tags WHERE name=?" - ).bind(tag_name).first() - if t_row: - tag_id = t_row.id - else: - tag_id = new_id() - try: - await env.DB.prepare( - "INSERT INTO tags (id,name) VALUES (?,?)" - ).bind(tag_id, tag_name).run() - except Exception as e: - capture_exception(e, req, env, f"...") - continue + candidate_tag_id = new_id() + await env.DB.prepare( + "INSERT OR IGNORE INTO tags (id,name) VALUES (?,?)" + ).bind(candidate_tag_id, tag_name).run() + t_row = await env.DB.prepare( + "SELECT id FROM tags WHERE name=?" + ).bind(tag_name).first() + if not t_row: + return err("Failed to resolve tag", 500) + tag_id = t_row.idApply the same pattern in both
api_create_activity()andapi_add_activity_tags().As per coding guidelines,
**/*.py: Ensure proper error handling and logging.Also applies to: 392-428
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api_activities.py` around lines 130 - 154, The tag loop in api_create_activity (and mirror in api_add_activity_tags) currently calls .strip() on unvalidated JSON values and uses a SELECT-then-INSERT that races under concurrency; fix by first validating each tag is a string (if not, capture_exception/log and continue), then trim and skip empty strings, and make the upsert race-safe by using an INSERT OR IGNORE INTO tags (id,name) with a generated id and immediately SELECT id FROM tags WHERE name=? to retrieve the canonical tag id (so the code reuses a concurrently inserted tag instead of failing), keeping capture_exception around DB errors and continuing on failure; update the loops around env.DB.prepare, new_id(), and activity_tags INSERT to follow this pattern in both api_create_activity and api_add_activity_tags.src/db_utils.py (1)
113-132:⚠️ Potential issue | 🟠 MajorDon't swallow non-duplicate seed failures.
seed_db()still treats every D1 error as “already seeded”. Line 115 also recordsuid_map[uname]before the user insert succeeds, so/api/seedcan report success with partial or inconsistent data.🧰 Suggested direction
- uid_map[uname] = uid - try: + try: await env.DB.prepare( - "INSERT INTO users " + "INSERT OR IGNORE INTO users " "(id,username_hash,email_hash,name,username,email,password_hash,role)" " VALUES (?,?,?,?,?,?,?,?)" ).bind( uid, blind_index(uname, enc_key), @@ hash_password(pw, uname), encrypt(role, enc_key), ).run() - except Exception: - pass # already seeded + uid_map[uname] = uid + except Exception: + raiseApply the same rule to the tag/activity/session/enrollment loops: use
INSERT OR IGNOREonly where duplicates are expected, and let unexpected write failures bubble so/api/seedfails instead of returning a false success.As per coding guidelines,
**/*.py: Ensure proper error handling and logging.Also applies to: 149-155, 202-221, 245-257, 269-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db_utils.py` around lines 113 - 132, The seed_db loop currently records uid_map[uname] before attempting the insert and catches all Exceptions (silencing unexpected failures); change this to only mark uid_map after a successful insert and stop swallowing non-duplicate errors — either use a vendor-safe "INSERT OR IGNORE" (or equivalent upsert) if duplicates are expected for the users insert prepared via env.DB.prepare, or catch only the specific duplicate/constraint error and re-raise or log+raise any other Exception so /api/seed fails on real write errors; apply the same pattern to the tag/activity/session/enrollment loops referenced and ensure functions/values like uid_map, env.DB.prepare, blind_index, encrypt, and hash_password are only used after a successful DB write.
🤖 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/api_activities.py`:
- Around line 240-246: The join handler currently trusts the incoming "role"
(variables act_id and role) and permits "participant", "instructor", or
"organizer", allowing users to self-assign privileged roles; change the logic so
the /api/join path always creates an enrollment with role = "participant"
regardless of body.get("role") input, remove or ignore the whitelist branch that
promotes instructor/organizer, and ensure only host/admin-only mutation paths
(separate functions) can assign elevated roles; keep the existing act_id
validation and use err("activity_id is required") as before.
In `@src/worker.py`:
- Around line 68-75: The admin page at admin_path is protected but a request to
the raw asset "/admin.html" can bypass auth because serve_static handles it;
update the request handling in the request dispatch (use the same variables
path, method and existing is_basic_auth_valid / unauthorized_basic checks) so
that both admin_path and the asset path for admin (e.g., "/admin.html" or
clean_path(getattr(env, "ADMIN_ASSET", "admin.html"))) are checked together
before falling through to serve_static(), or explicitly block/redirect requests
to "/admin.html" by returning unauthorized_basic() when path matches that asset;
ensure you modify the same branch that checks path == admin_path and reuse
is_basic_auth_valid/unauthorized_basic to enforce auth for both identifiers.
---
Duplicate comments:
In `@src/api_activities.py`:
- Around line 90-94: The api_create_activity handler currently only verifies
authentication; enforce that the authenticated user is a host before allowing
creation by checking the user's role (from the object returned by verify_token)
and returning a 403 error if it is not "host" (or the equivalent host role value
used in your app). Update api_create_activity to inspect the role (e.g.,
user["role"] or user.role) immediately after verify_token and reject non-hosts
so downstream functions like api_create_session and api_add_activity_tags cannot
be exploited by members.
- Around line 130-154: The tag loop in api_create_activity (and mirror in
api_add_activity_tags) currently calls .strip() on unvalidated JSON values and
uses a SELECT-then-INSERT that races under concurrency; fix by first validating
each tag is a string (if not, capture_exception/log and continue), then trim and
skip empty strings, and make the upsert race-safe by using an INSERT OR IGNORE
INTO tags (id,name) with a generated id and immediately SELECT id FROM tags
WHERE name=? to retrieve the canonical tag id (so the code reuses a concurrently
inserted tag instead of failing), keeping capture_exception around DB errors and
continuing on failure; update the loops around env.DB.prepare, new_id(), and
activity_tags INSERT to follow this pattern in both api_create_activity and
api_add_activity_tags.
In `@src/db_utils.py`:
- Around line 113-132: The seed_db loop currently records uid_map[uname] before
attempting the insert and catches all Exceptions (silencing unexpected
failures); change this to only mark uid_map after a successful insert and stop
swallowing non-duplicate errors — either use a vendor-safe "INSERT OR IGNORE"
(or equivalent upsert) if duplicates are expected for the users insert prepared
via env.DB.prepare, or catch only the specific duplicate/constraint error and
re-raise or log+raise any other Exception so /api/seed fails on real write
errors; apply the same pattern to the tag/activity/session/enrollment loops
referenced and ensure functions/values like uid_map, env.DB.prepare,
blind_index, encrypt, and hash_password are only used after a successful DB
write.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a166d0d8-5dc9-49ae-9334-8421868b3806
📒 Files selected for processing (7)
src/api_activities.pysrc/api_admin.pysrc/api_auth.pysrc/db_utils.pysrc/http_utils.pysrc/static_utils.pysrc/worker.py
…a-one-labs-logo Add Alpha One Labs logo to header and favicon across all pages
There was a problem hiding this comment.
Pull request overview
Refactors the Cloudflare Worker implementation by splitting the previously monolithic src/worker.py into focused modules for HTTP helpers, security utilities, DB/schema setup, and API route handlers—while keeping the existing request dispatch structure.
Changes:
- Extracted security, HTTP/response, DB init/seed, static serving, and API handler logic into new modules under
src/. - Simplified
worker.pyto mostly imports + routing/dispatch (_dispatch,on_fetch). - Centralized shared helpers (CORS headers, JSON responses, basic auth, exception logging) in
http_utils.py.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/worker.py | Reduced to dispatcher + imports wiring handlers/utilities from new modules. |
| src/http_utils.py | Introduces shared HTTP helpers (CORS, JSON responses, request parsing, basic auth, exception logging). |
| src/security_utils.py | Houses ID generation, encryption/decryption, blind indexing, password hashing, and token helpers. |
| src/db_utils.py | Moves schema DDL and seed logic out of the worker entrypoint. |
| src/api_auth.py | Contains register/login handlers using shared HTTP + security utilities. |
| src/api_activities.py | Contains activities/enrollments/sessions/tags/dashboard handlers. |
| src/api_admin.py | Adds admin-only table-count endpoint protected by basic auth. |
| src/static_utils.py | Moves static asset serving out of worker.py. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/worker.py (1)
68-76:⚠️ Potential issue | 🟠 Major
/admin.htmlcan still bypass the Basic Auth gate.The condition at Line 73 only checks
admin_path, but a direct request to/admin.htmlwill skip this block entirely and fall through toserve_static()at Line 132. This allows unauthenticated access to the admin page by simply requesting the raw asset path.Consider protecting both paths together:
🔒 Proposed fix to close the bypass
- if path == admin_path and method == "GET": + if path in {admin_path, "/admin.html"} and method == "GET": if not is_basic_auth_valid(request, env): return unauthorized_basic() return await serve_static("/admin.html", env)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 68 - 76, The admin page can be accessed via the raw asset path "/admin.html" because the auth check only matches admin_path; update the guard to protect both paths by checking if path == admin_path or path == "/admin.html" (use the existing variables and helpers is_basic_auth_valid, unauthorized_basic, and serve_static) so that any request to either admin_path or the literal "/admin.html" enforces basic auth and then calls await serve_static("/admin.html", env) on success.src/static_utils.py (1)
25-45:⚠️ Potential issue | 🟠 MajorSPA fallback for missing assets serves HTML with wrong Content-Type.
When a request for
/app.jsmisses in KV storage, the code falls back toindex.html(Lines 30-34) but then determines the MIME type from the originalkey("app.js"), resulting in HTML content served asapplication/javascript. This breaks the browser and masks real storage failures.A better approach: only use the SPA fallback for extensionless routes (likely client-side navigation), and return 404 or 500 for missing assets with extensions.
🛠️ Proposed fix to differentiate route fallback from asset errors
try: content = await env.__STATIC_CONTENT.get(key, "text") except Exception: - content = None + return Response( + "Static content unavailable", + status=500, + headers={"Content-Type": "text/plain; charset=utf-8", **CORS}, + ) - if content is None: + # Only fall back to index.html for extensionless routes (SPA navigation) + is_route = "." not in key.rsplit("/", 1)[-1] + if content is None and is_route: try: content = await env.__STATIC_CONTENT.get("index.html", "text") except Exception: - content = None + return Response( + "Static content unavailable", + status=500, + headers={"Content-Type": "text/plain; charset=utf-8", **CORS}, + ) + ext = "html" # Override ext for the fallback case + else: + ext = key.rsplit(".", 1)[-1] if "." in key else "html" if content is None: return Response( "<h1>404 - Not Found</h1>", status=404, headers={"Content-Type": "text/html"}, ) - ext = key.rsplit(".", 1)[-1] if "." in key else "html" mime = _MIME.get(ext, "text/plain") return Response(content, headers={"Content-Type": mime, **CORS})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/static_utils.py` around lines 25 - 45, The SPA fallback currently loads index.html via env.__STATIC_CONTENT.get when the original key (variable key) is missing but then infers MIME from the original key (using _MIME and ext), causing HTML to be served with the wrong content-type for asset requests; change the logic in the block around env.__STATIC_CONTENT.get + Response so that you only perform the index.html fallback when key has no file extension (i.e., "." not in key), and for keys that contain an extension return a 404 (or 500) Response immediately; when serving the fallback explicitly set mime to "text/html" (or treat ext="html") and return Response(content, headers={"Content-Type": "text/html", **CORS}), otherwise on missing asset return Response("<h1>404 - Not Found</h1>", status=404, headers={"Content-Type": _MIME.get(ext, "text/plain"), **CORS}) so MIME for errors matches the asset type.
🤖 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/http_utils.py`:
- Around line 10-28: The capture_exception function currently swallows any
errors raised while building or printing the payload (the bare except/pass),
making logger failures invisible; update capture_exception to catch exceptions
from the logging path and emit a minimal fallback message to stderr (including
the original exception type/name and the traceback from traceback.format_exc())
so logging failures are visible, and ensure the fallback runs inside the
existing except block rather than re-raising; reference the capture_exception
function and the payload/print logic when making the change.
- Around line 57-68: Add a precise return type hint to parse_json_object to
improve IDE/type checking: annotate the function signature (parse_json_object)
to return Tuple[Optional[Dict[str, Any]], Optional[Any]] (or use Python 3.10+
union/list syntax) and add the required typing imports (Tuple, Optional, Dict,
Any) at the top of the module; keep the existing behavior and return values
(body or None, error response or None) unchanged.
In `@src/static_utils.py`:
- Around line 5-14: The _MIME mapping is missing common font and modern image
types; update the _MIME dict to include webfont and new image MIME entries
(e.g., "woff" -> "font/woff", "woff2" -> "font/woff2", "ttf" -> "font/ttf",
"otf" -> "font/otf", "eot" -> "application/vnd.ms-fontobject", plus "webp" ->
"image/webp", "avif" -> "image/avif") so static file responses return correct
Content-Type headers; modify the _MIME variable in src/static_utils.py (the
_MIME dict) to add these keys and their MIME strings.
---
Duplicate comments:
In `@src/static_utils.py`:
- Around line 25-45: The SPA fallback currently loads index.html via
env.__STATIC_CONTENT.get when the original key (variable key) is missing but
then infers MIME from the original key (using _MIME and ext), causing HTML to be
served with the wrong content-type for asset requests; change the logic in the
block around env.__STATIC_CONTENT.get + Response so that you only perform the
index.html fallback when key has no file extension (i.e., "." not in key), and
for keys that contain an extension return a 404 (or 500) Response immediately;
when serving the fallback explicitly set mime to "text/html" (or treat
ext="html") and return Response(content, headers={"Content-Type": "text/html",
**CORS}), otherwise on missing asset return Response("<h1>404 - Not Found</h1>",
status=404, headers={"Content-Type": _MIME.get(ext, "text/plain"), **CORS}) so
MIME for errors matches the asset type.
In `@src/worker.py`:
- Around line 68-76: The admin page can be accessed via the raw asset path
"/admin.html" because the auth check only matches admin_path; update the guard
to protect both paths by checking if path == admin_path or path == "/admin.html"
(use the existing variables and helpers is_basic_auth_valid, unauthorized_basic,
and serve_static) so that any request to either admin_path or the literal
"/admin.html" enforces basic auth and then calls await
serve_static("/admin.html", env) on success.
🪄 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: 568a9ce6-c387-4c66-a370-9a238f8d6919
📒 Files selected for processing (3)
src/http_utils.pysrc/static_utils.pysrc/worker.py
|
Hey @A1L13N , This PR’s aim is to split the code into 8 subfiles. The Copilot, codderbhai comments are mostly follow-up improvement and hardening suggestions. I feel this can be ignored for now... |
…ncryption feat: replace XOR cipher with AES-256-GCM encryption via Web Crypto API
|
please resolve conflicts |
…eb-crypto-aes-object-conversion fix: Resolve Web Crypto AES-GCM Map conversion bug during encryption
|
Sure sir! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
src/api_activities.py (2)
220-225:⚠️ Potential issue | 🟠 MajorIgnore the caller’s requested enrollment role on
/api/join.Any authenticated user can currently self-assign
instructorororganizer. Public joins should always createparticipantenrollments; elevated roles need a separate privileged mutation path.🔒 Suggested fix
act_id = body.get("activity_id") - role = (body.get("role") or "participant").strip() + role = "participant" if not act_id: return err("activity_id is required") - if role not in ("participant", "instructor", "organizer"): - role = "participant"As per coding guidelines,
**/*.py: Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api_activities.py` around lines 220 - 225, The endpoint currently honors the caller-supplied role from request body (role = (body.get("role") or "participant").strip()), allowing users to self-assign elevated roles; change the logic in the join handler that sets act_id and role so that the role returned/used is always "participant" for public /api/join calls (ignore body.get("role") entirely), i.e., remove/replace the role parsing and any condition that promotes "instructor" or "organizer" so only "participant" is created; optionally log or validate if the client attempted an elevated role and ensure any elevated-role assignment is handled only via a separate privileged mutation path.
118-137:⚠️ Potential issue | 🟠 MajorValidate
tagsbefore mutating the database, and re-read tag ids after upserts.
parse_json_object()only guarantees a dict, sotags="python"becomes one-character tags and any non-string item can raise after the activity/tag update has already started. The current SELECT-then-INSERT flow also loses the association under concurrent requests: the losing insert logs andcontinues instead of reusing the existing tag id. Normalizingtagstolist[str]up front, thenINSERT OR IGNORE+ re-selecting by name before inserting intoactivity_tags, would make both handlers safe.As per coding guidelines,
**/*.py: Ensure proper error handling and logging.Also applies to: 377-396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api_activities.py` around lines 118 - 137, The tags loop must first validate and normalize the incoming tags payload (use the same parsing path as parse_json_object to ensure tags is a list and convert/strip only string items, skipping or logging non-strings) before mutating the DB; replace the current per-item SELECT-then-INSERT pattern in the api_create_activity tag handling with an INSERT OR IGNORE into tags (using tag_name) and then re-query the tags table for the tag id (so concurrent upserts reuse the existing row), and only after obtaining a confirmed tag_id insert into activity_tags (INSERT OR IGNORE), preserving the existing capture_exception calls but ensuring they occur after DB state is stable and after validation of tag_name and act_id.src/worker.py (1)
38-46:⚠️ Potential issue | 🟠 MajorProtect the raw admin asset path too.
This branch only challenges
admin_path. A direct request to/admin.htmlstill falls through toserve_static()and bypasses Basic Auth. Please gate both the friendly admin URL and the underlying asset path before the static fallback runs.🔐 Suggested fix
admin_path = clean_path(getattr(env, "ADMIN_URL", "")) + admin_asset_path = "/admin.html" - if path == admin_path and method == "GET": + if path in {admin_path, admin_asset_path} and method == "GET": if not is_basic_auth_valid(request, env): return unauthorized_basic() return await serve_static("/admin.html", env)As per coding guidelines,
**/*.py: Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 38 - 46, The admin asset route currently only enforces Basic Auth for the friendly admin URL (admin_path) but not the raw asset path ("/admin.html"), so update the request branch that checks path/method to include both cases: when path equals admin_path OR equals "/admin.html" (the asset served by serve_static), call is_basic_auth_valid(request, env) and return unauthorized_basic() if it fails before calling serve_static; ensure the same logic (method == "GET") and reuse the existing unauthorized_basic(), is_basic_auth_valid(), admin_path variable, and serve_static("/admin.html", env) symbols so the raw asset cannot bypass authentication.src/static_utils.py (1)
25-45:⚠️ Potential issue | 🟠 MajorReturn 404 for missing asset files; reserve
index.htmlfallback for routes.A miss on
/app.jsor/logo.pngcurrently falls back toindex.html, and the response still uses the original extension-derived MIME type. That serves HTML as JS/image data and hides storage failures behind a fake success. Only extensionless routes should use the SPA fallback; missing asset keys should 404, and__STATIC_CONTENT.get()errors should surface as 500s.🛠️ Suggested fix
try: content = await env.__STATIC_CONTENT.get(key, "text") except Exception: - content = None + return Response( + "Static content unavailable", + status=500, + headers={"Content-Type": "text/plain; charset=utf-8", **CORS}, + ) - if content is None: + wants_route_fallback = "." not in key.rsplit("/", 1)[-1] + if content is None and wants_route_fallback: try: content = await env.__STATIC_CONTENT.get("index.html", "text") except Exception: - content = None + return Response( + "Static content unavailable", + status=500, + headers={"Content-Type": "text/plain; charset=utf-8", **CORS}, + )As per coding guidelines,
**/*.py: Ensure proper error handling and logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/static_utils.py` around lines 25 - 45, The current handler uses env.__STATIC_CONTENT.get errors and missing assets to fall back to index.html and picks MIME from the requested key, so binary/static assets like /app.js or /logo.png incorrectly return HTML and storage errors are hidden; change the logic in the function that calls env.__STATIC_CONTENT.get (refer to env.__STATIC_CONTENT.get, key, Response, _MIME, CORS) to: 1) if env.__STATIC_CONTENT.get raises an exception, log/propagate it and return a 500 Response (do not fall back to index.html on exceptions), 2) if get returns None for a key that has an extension (key contains "."), return a 404 with proper Content-Type "text/html" for the 404 body, and 3) only when key has no extension (no ".") attempt to fetch "index.html" as the SPA fallback; finally ensure the Content-Type header uses _MIME[ext] (or default) and include CORS in all responses.src/security_utils.py (2)
130-156:⚠️ Potential issue | 🟠 MajorExpire signed tokens.
These tokens never become invalid on their own, so a leaked token stays usable until
secretrotates. Addiat/expto the signed payload, reject expired tokens inverify_token(), and cover valid, expired, and malformed cases with tests.As per coding guidelines,
**/*.py: Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security_utils.py` around lines 130 - 156, create_token currently emits unsigned-expiring tokens and verify_token never rejects expired tokens; update create_token to include iat (issued-at) and exp (expiry) fields in the JSON payload (use integer epoch seconds, allow configurable TTL or default duration) and sign that payload, and update verify_token to parse exp and return None if the current time >= exp (also handle missing/invalid exp by rejecting), keeping all other validation (signature, base64 decode) intact; add unit tests for valid, expired, and malformed tokens exercising create_token and verify_token to ensure correct acceptance/rejection.
111-116:⚠️ Potential issue | 🟠 MajorMove
_PEPPERout of the repository.Hard-coding the password pepper means every clone and deployment shares the same long-lived secret. If the repo leaks, offline cracking of password hashes gets much easier. Load this from the worker environment at startup and fail closed when it is missing or empty.
As per coding guidelines,
**/*.py: Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security_utils.py` around lines 111 - 116, The module currently hard-codes _PEPPER and uses it in _user_salt, which exposes a long-lived secret; change initialization so the pepper is read from the environment at import/startup (e.g., os.getenv or similar) and fail closed if it's missing or empty: replace the literal _PEPPER with a module-level value loaded from env (still kept as bytes for use in _user_salt) and raise a clear exception during startup if the env var is not set or is blank; ensure _user_salt continues to use the loaded value (preserve sha256(_PEPPER + username.encode(...))).src/db_utils.py (2)
95-115:⚠️ Potential issue | 🟠 MajorDon't advance
uid_mapbefore the user insert outcome is known.
uid_map[uname] = uidhappens before the write, and the broadexcepthides every failure. A non-duplicate insert error will still let later activity/session rows reference users that were never persisted. For seed data, handle duplicates explicitly (INSERT OR IGNOREor conflict-specific handling), and log or abort on anything else.As per coding guidelines,
**/*.py: Ensure proper error handling and logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db_utils.py` around lines 95 - 115, Currently uid_map[uname] = uid is set before the DB write and the except swallows all errors; change the logic in the loop over seed_users so you only add uid_map[uname] after a confirmed successful insert (i.e., after run() returns without error), use a conflict-aware insert strategy (e.g., "INSERT OR IGNORE" or ON CONFLICT DO NOTHING) or explicitly catch the DB/IntegrityError for duplicates, and replace the broad except Exception: pass with targeted exception handling that logs non-duplicate failures (using your logger) and re-raises or aborts on unexpected errors; refer to uid_map, seed_users, env.DB.prepare(...).bind(...).run(), blind_index, encrypt_aes, and hash_password to locate and update the code.
132-135:⚠️ Potential issue | 🟠 MajorStop suppressing arbitrary seed failures.
These
except Exception: passblocks make seeding look successful even when tags, activities, sessions, or enrollments were not written. Restrict suppression to expected duplicate conflicts, or switch the idempotent statements to explicitINSERT OR IGNORE, and log/raise unexpected DB errors.As per coding guidelines,
**/*.py: Ensure proper error handling and logging.Also applies to: 146-155, 157-162, 172-187, 198-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db_utils.py` around lines 132 - 135, The try/except blocks around the DB seed writes (e.g., env.DB.prepare("INSERT INTO tags (id,name) VALUES (?,?)").bind(tid, tname).run()) currently swallow all exceptions; change them to either use idempotent SQL (e.g., INSERT OR IGNORE / ON CONFLICT DO NOTHING) for the seed statements inserting into tags, activities, sessions, enrollments, or catch and handle only the expected duplicate-key exception and log/raise any other errors; ensure you use the same unique identifiers (env.DB.prepare, the INSERT statements for tags/activities/sessions/enrollments) so unexpected DB errors are logged with context (or re-raised) instead of silently passing.
🤖 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/api_auth.py`:
- Around line 89-96: The code checks decrypt_aes result for real_role but not
for real_name, allowing "[decryption error]" to be returned to clients; modify
the login flow in the function that calls decrypt_aes (variables real_role and
real_name) to validate real_name the same way as real_role, returning
err("Account data corrupted - please contact support", 500) (and optionally log
the failure) before creating the token (create_token) or returning ok, so
neither create_token nor ok({"token":..., "user": {"id": ..., "username": ...,
"name": real_name, "role": real_role}}) runs when real_name indicates a
decryption error.
In `@src/security_utils.py`:
- Around line 126-127: The password verification in verify_password currently
uses a plain equality (hash_password(password, username) == stored); replace
this with a constant-time comparison by computing hash_password(password,
username) into a local variable and returning hmac.compare_digest(computed,
stored) (consistent with verify_token's pattern) so timing attacks are
prevented.
---
Duplicate comments:
In `@src/api_activities.py`:
- Around line 220-225: The endpoint currently honors the caller-supplied role
from request body (role = (body.get("role") or "participant").strip()), allowing
users to self-assign elevated roles; change the logic in the join handler that
sets act_id and role so that the role returned/used is always "participant" for
public /api/join calls (ignore body.get("role") entirely), i.e., remove/replace
the role parsing and any condition that promotes "instructor" or "organizer" so
only "participant" is created; optionally log or validate if the client
attempted an elevated role and ensure any elevated-role assignment is handled
only via a separate privileged mutation path.
- Around line 118-137: The tags loop must first validate and normalize the
incoming tags payload (use the same parsing path as parse_json_object to ensure
tags is a list and convert/strip only string items, skipping or logging
non-strings) before mutating the DB; replace the current per-item
SELECT-then-INSERT pattern in the api_create_activity tag handling with an
INSERT OR IGNORE into tags (using tag_name) and then re-query the tags table for
the tag id (so concurrent upserts reuse the existing row), and only after
obtaining a confirmed tag_id insert into activity_tags (INSERT OR IGNORE),
preserving the existing capture_exception calls but ensuring they occur after DB
state is stable and after validation of tag_name and act_id.
In `@src/db_utils.py`:
- Around line 95-115: Currently uid_map[uname] = uid is set before the DB write
and the except swallows all errors; change the logic in the loop over seed_users
so you only add uid_map[uname] after a confirmed successful insert (i.e., after
run() returns without error), use a conflict-aware insert strategy (e.g.,
"INSERT OR IGNORE" or ON CONFLICT DO NOTHING) or explicitly catch the
DB/IntegrityError for duplicates, and replace the broad except Exception: pass
with targeted exception handling that logs non-duplicate failures (using your
logger) and re-raises or aborts on unexpected errors; refer to uid_map,
seed_users, env.DB.prepare(...).bind(...).run(), blind_index, encrypt_aes, and
hash_password to locate and update the code.
- Around line 132-135: The try/except blocks around the DB seed writes (e.g.,
env.DB.prepare("INSERT INTO tags (id,name) VALUES (?,?)").bind(tid,
tname).run()) currently swallow all exceptions; change them to either use
idempotent SQL (e.g., INSERT OR IGNORE / ON CONFLICT DO NOTHING) for the seed
statements inserting into tags, activities, sessions, enrollments, or catch and
handle only the expected duplicate-key exception and log/raise any other errors;
ensure you use the same unique identifiers (env.DB.prepare, the INSERT
statements for tags/activities/sessions/enrollments) so unexpected DB errors are
logged with context (or re-raised) instead of silently passing.
In `@src/security_utils.py`:
- Around line 130-156: create_token currently emits unsigned-expiring tokens and
verify_token never rejects expired tokens; update create_token to include iat
(issued-at) and exp (expiry) fields in the JSON payload (use integer epoch
seconds, allow configurable TTL or default duration) and sign that payload, and
update verify_token to parse exp and return None if the current time >= exp
(also handle missing/invalid exp by rejecting), keeping all other validation
(signature, base64 decode) intact; add unit tests for valid, expired, and
malformed tokens exercising create_token and verify_token to ensure correct
acceptance/rejection.
- Around line 111-116: The module currently hard-codes _PEPPER and uses it in
_user_salt, which exposes a long-lived secret; change initialization so the
pepper is read from the environment at import/startup (e.g., os.getenv or
similar) and fail closed if it's missing or empty: replace the literal _PEPPER
with a module-level value loaded from env (still kept as bytes for use in
_user_salt) and raise a clear exception during startup if the env var is not set
or is blank; ensure _user_salt continues to use the loaded value (preserve
sha256(_PEPPER + username.encode(...))).
In `@src/static_utils.py`:
- Around line 25-45: The current handler uses env.__STATIC_CONTENT.get errors
and missing assets to fall back to index.html and picks MIME from the requested
key, so binary/static assets like /app.js or /logo.png incorrectly return HTML
and storage errors are hidden; change the logic in the function that calls
env.__STATIC_CONTENT.get (refer to env.__STATIC_CONTENT.get, key, Response,
_MIME, CORS) to: 1) if env.__STATIC_CONTENT.get raises an exception,
log/propagate it and return a 500 Response (do not fall back to index.html on
exceptions), 2) if get returns None for a key that has an extension (key
contains "."), return a 404 with proper Content-Type "text/html" for the 404
body, and 3) only when key has no extension (no ".") attempt to fetch
"index.html" as the SPA fallback; finally ensure the Content-Type header uses
_MIME[ext] (or default) and include CORS in all responses.
In `@src/worker.py`:
- Around line 38-46: The admin asset route currently only enforces Basic Auth
for the friendly admin URL (admin_path) but not the raw asset path
("/admin.html"), so update the request branch that checks path/method to include
both cases: when path equals admin_path OR equals "/admin.html" (the asset
served by serve_static), call is_basic_auth_valid(request, env) and return
unauthorized_basic() if it fails before calling serve_static; ensure the same
logic (method == "GET") and reuse the existing unauthorized_basic(),
is_basic_auth_valid(), admin_path variable, and serve_static("/admin.html", env)
symbols so the raw asset cannot bypass authentication.
🪄 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: 754a39c4-88e9-4d1c-b7f2-2e5282408d47
⛔ Files ignored due to path filters (1)
public/images/logo.pngis excluded by!**/*.png
📒 Files selected for processing (14)
public/admin.htmlpublic/course.htmlpublic/dashboard.htmlpublic/index.htmlpublic/login.htmlpublic/teach.htmlsrc/api_activities.pysrc/api_admin.pysrc/api_auth.pysrc/db_utils.pysrc/http_utils.pysrc/security_utils.pysrc/static_utils.pysrc/worker.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/security_utils.py (2)
111-116:⚠️ Potential issue | 🟠 MajorMove the password pepper to runtime configuration.
_PEPPERis still a long-lived secret committed to source, so every clone and deployment shares the same password-salt material. Load it from the environment during startup and fail closed if it is missing or empty.🔐 Suggested fix
-_PEPPER = b"edu-platform-cf-pepper-2024" +_PEPPER_ENV = "PASSWORD_PEPPER" +_pepper = os.environ.get(_PEPPER_ENV) +if not _pepper: + raise RuntimeError(f"Missing required environment variable: {_PEPPER_ENV}") +_PEPPER = _pepper.encode("utf-8")As per coding guidelines,
**/*.py: Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security_utils.py` around lines 111 - 116, The module currently hardcodes _PEPPER causing a shared secret; change it to load from an environment variable at startup (e.g., os.environ["PASSWORD_PEPPER"]) and fail closed if missing/empty by raising an exception during import/initialization; update references in _user_salt(username: str) to use the runtime-loaded pepper variable (keep _PBKDF2_IT unchanged) and ensure any tests or startup code sets the env var accordingly.
130-156:⚠️ Potential issue | 🟠 MajorAdd
iat/expto signed auth tokens.These bearer tokens never expire today, so a leaked token stays valid until
JWT_SECRETrotates. Add a TTL increate_token(), enforce it inverify_token(), and cover valid, expired, and malformed tokens with tests.⏳ Suggested fix
+import time + +_TOKEN_TTL_SECONDS = 60 * 60 + def create_token(uid: str, username: str, role: str, secret: str) -> str: + now = int(time.time()) payload = base64.b64encode( - json.dumps({"id": uid, "username": username, "role": role}).encode() + json.dumps( + { + "id": uid, + "username": username, + "role": role, + "iat": now, + "exp": now + _TOKEN_TTL_SECONDS, + } + ).encode() ).decode("ascii") sig = _hmac.new( secret.encode("utf-8"), payload.encode("utf-8"), hashlib.sha256 @@ def verify_token(raw: str, secret: str): @@ - return json.loads(base64.b64decode(payload + "=" * padding).decode("utf-8")) + data = json.loads(base64.b64decode(payload + "=" * padding).decode("utf-8")) + if int(time.time()) >= int(data.get("exp", 0)): + return None + return dataAs per coding guidelines,
**/*.py: Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval(). 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/security_utils.py` around lines 130 - 156, create_token currently emits unsigned-timestamp-less base64 payloads and verify_token doesn't enforce expiry; update create_token(uid, username, role, secret) to include "iat" (current UNIX epoch) and "exp" (iat + TTL, e.g., configurable default seconds) inside the JSON payload before base64-encoding/signing, and update verify_token(raw, secret) to parse the payload and return None if the token is malformed or the current time is >= exp; keep HMAC signing of the full base64 payload (payload and sig variables) the same but re-sign the new payload format, and add tests for a valid token, an expired token (exp in the past), and malformed tokens to cover these paths.
🤖 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/api_auth.py`:
- Around line 74-100: Wrap the DB lookup, all decryptions (decrypt_aes),
password verification (verify_password) and token creation (create_token) inside
a local try/except in api_login so unexpected DB/read/token exceptions are
caught; in the except block call capture_exception(e) and return the same error
responses the handler uses for normal failures (e.g., err("Invalid username or
password", 401) for auth failures or err("Account data corrupted - please
contact support", 500) for decryption/account corruption), ensuring
capture_exception is imported if not already.
In `@src/security_utils.py`:
- Around line 105-108: blind_index currently lacks a return type and
verify_token returns an untyped payload; add explicit type hints by annotating
blind_index(...) -> str and introduce a concrete return type for verify_token
(e.g., a TypedDict or dataclass named AuthPayload with the expected fields) and
change verify_token(...) -> AuthPayload | None (or Optional[AuthPayload])
accordingly; update imports (from typing import Optional, TypedDict or
dataclasses) and replace any uses that expect an untyped dict to rely on the new
AuthPayload type to keep the auth boundary explicit and type-safe.
---
Duplicate comments:
In `@src/security_utils.py`:
- Around line 111-116: The module currently hardcodes _PEPPER causing a shared
secret; change it to load from an environment variable at startup (e.g.,
os.environ["PASSWORD_PEPPER"]) and fail closed if missing/empty by raising an
exception during import/initialization; update references in
_user_salt(username: str) to use the runtime-loaded pepper variable (keep
_PBKDF2_IT unchanged) and ensure any tests or startup code sets the env var
accordingly.
- Around line 130-156: create_token currently emits unsigned-timestamp-less
base64 payloads and verify_token doesn't enforce expiry; update
create_token(uid, username, role, secret) to include "iat" (current UNIX epoch)
and "exp" (iat + TTL, e.g., configurable default seconds) inside the JSON
payload before base64-encoding/signing, and update verify_token(raw, secret) to
parse the payload and return None if the token is malformed or the current time
is >= exp; keep HMAC signing of the full base64 payload (payload and sig
variables) the same but re-sign the new payload format, and add tests for a
valid token, an expired token (exp in the past), and malformed tokens to cover
these paths.
🪄 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: ccbd26b4-6011-4133-9ed5-d83c13241549
📒 Files selected for processing (2)
src/api_auth.pysrc/security_utils.py
Organised.workers.py.mp4
Refactor: split monolithic worker.py into eight focused modules; preserve routing while centralizing security, HTTP helpers, DB init/seeding, static serving, and API handlers.
Purpose
Key modifications
Potential impact / review notes