Skip to content

Perfectly organized worker.py into 8 files total#17

Open
Shrey1006 wants to merge 15 commits intoalphaonelabs:mainfrom
Shrey1006:workersplit
Open

Perfectly organized worker.py into 8 files total#17
Shrey1006 wants to merge 15 commits intoalphaonelabs:mainfrom
Shrey1006:workersplit

Conversation

@Shrey1006
Copy link
Copy Markdown
Contributor

@Shrey1006 Shrey1006 commented Mar 24, 2026

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

  • Break the single-file server into smaller modules to improve organization, readability, testability, and maintainability while keeping request-dispatch behavior intact.

Key modifications

  • Added src/security_utils.py: UUID generation, AES-GCM encrypt/decrypt (PBKDF2-derived key, v1: Base64 payloads), legacy XOR fallback on decrypt, blind-index (HMAC-SHA256), PBKDF2 password hashing (100k iterations + module pepper), and simple token create/verify (Base64 payload + HMAC-SHA256 signature). Decrypt returns error markers on failures; encrypt logs and raises on errors.
  • Added src/http_utils.py: centralized CORS headers, json_resp/ok/err helpers, parse_json_object(req), capture_exception logging, clean_path path normalization, Basic Auth helpers (is_basic_auth_valid / unauthorized_basic).
  • Added src/db_utils.py: SQLite DDL for users/activities/sessions/enrollments/tags/activity_tags and indexes; init_db(env) to apply schema; seed_db(env, enc_key) to insert sample users, tags, activities, sessions, and enrollments (seeding swallows insert errors for idempotence).
  • Added src/static_utils.py: serve_static(path, env) using env.__STATIC_CONTENT with MIME mapping, path normalization ("/" → index.html, append .html when missing), CORS headers, and 404/index fallback.
  • Added src/api_auth.py: api_register and api_login (registration/login flows with blind-index lookups, password hashing/verification, AES-encrypted fields, and token issuance).
  • Added src/api_activities.py: handlers for listing/creating activities, getting activity details (selective decryption for enrolled/host), joining, dashboard, create_session, list_tags, and add_activity_tags; uses verify_token and INSERT OR IGNORE for tag linking.
  • Added src/api_admin.py: api_admin_table_counts(req, env) protected by Basic Auth; returns non-internal table row counts.
  • Updated src/worker.py: removed in-file implementations for security, HTTP helpers, DB init/seeding, static serving, and API handlers; dispatcher and routes remain but now import and call the new modules; CORS and auth helpers switched to imports.
  • Minor UI changes: favicon link and logo replacing text-avatar in multiple public/*.html files.

Potential impact / review notes

  • Behavior preserved at the HTTP-dispatch level; endpoints were moved mostly verbatim into modules.
  • Security-sensitive areas need careful review: AES key derivation parameters, legacy XOR fallback, token format/signing, blind-index determinism, and pepper management (env expectations).
  • seed_db swallows errors—good for idempotency but can hide initialization problems.
  • worker.py now depends on env variables and env.__STATIC_CONTENT; confirm deployment provides these.
  • Overall: modularization reduces cognitive load and improves testability but warrants focused review on cryptography, token handling, and seed behavior.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 24, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Monolithic 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

Cohort / File(s) Summary
Security utilities
src/security_utils.py
New module: UUID generation, AES‑GCM encrypt/decrypt (PBKDF2 key derivation + v1: Base64 payload), legacy XOR fallback, HMAC blind index, PBKDF2 password hash/verify, stateless token create/verify, and exception capture hooks.
HTTP helpers & authn
src/http_utils.py
New module: CORS headers, JSON response helpers (json_resp, ok, err), request JSON parsing, capture_exception, path normalization clean_path, Basic Auth helpers is_basic_auth_valid and unauthorized_basic.
DB schema & seeding
src/db_utils.py
New module: SQLite DDL for users/activities/sessions/enrollments/tags/indexes, init_db(env) to apply schema, and seed_db(env, enc_key) to insert sample users/activities/sessions/enrollments/tags using blind-index/encryption/password hashing.
API — Authentication
src/api_auth.py
New async handlers api_register and api_login: request validation, blind-index lookups, AES encrypt/decrypt of PII, password hashing/verification, token issuance, and error handling for uniqueness/format issues.
API — Activities
src/api_activities.py
New async handlers for listing/creating activities, retrieving activity details, joining activities, dashboard, session creation, tag listing/adding; enforces token auth for mutations and selectively decrypts fields based on enrollment/host status.
API — Admin
src/api_admin.py
New admin handler api_admin_table_counts(req, env) gated by Basic Auth; returns row counts for non-internal tables.
Static serving
src/static_utils.py
New serve_static(path, env) resolving keys in env.__STATIC_CONTENT, MIME mapping, index fallback, and CORS headers.
Worker wiring & public assets
src/worker.py, public/*.html
worker.py refactored to import and delegate to new modules (uses clean_path, is_basic_auth_valid, CORS, etc.); multiple public/*.html files updated to add favicon and swap inline letter marks for /images/logo.png.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.95% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: refactoring monolithic worker.py into 8 modular files with clear responsibility separation (security, auth, activities, admin, HTTP utilities, database, static serving).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Shrey1006 Shrey1006 changed the title Perfectly organized worker.py and other files Perfectly organized worker.py into other 3 files Mar 24, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 97eec85 and 87bfd48.

📒 Files selected for processing (4)
  • src/api_handlers.py
  • src/platform_utils.py
  • src/security_utils.py
  • src/worker.py

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (3)
src/api_activities.py (2)

90-94: ⚠️ Potential issue | 🟠 Major

Enforce the host role before activity creation.

src/worker.py Lines 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 in api_create_session() and api_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 | 🟠 Major

Validate tags before 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.id

Apply the same pattern in both api_create_activity() and api_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 | 🟠 Major

Don't swallow non-duplicate seed failures.

seed_db() still treats every D1 error as “already seeded”. Line 115 also records uid_map[uname] before the user insert succeeds, so /api/seed can 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:
+            raise

Apply the same rule to the tag/activity/session/enrollment loops: use INSERT OR IGNORE only where duplicates are expected, and let unexpected write failures bubble so /api/seed fails 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

📥 Commits

Reviewing files that changed from the base of the PR and between 87bfd48 and 58ac310.

📒 Files selected for processing (7)
  • src/api_activities.py
  • src/api_admin.py
  • src/api_auth.py
  • src/db_utils.py
  • src/http_utils.py
  • src/static_utils.py
  • src/worker.py

…a-one-labs-logo

Add Alpha One Labs logo to header and favicon across all pages
@Shrey1006 Shrey1006 changed the title Perfectly organized worker.py into other 3 files Perfectly organized worker.py into 8 files total Mar 25, 2026
@A1L13N A1L13N requested a review from Copilot March 26, 2026 00:43
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.py to 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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (2)
src/worker.py (1)

68-76: ⚠️ Potential issue | 🟠 Major

/admin.html can still bypass the Basic Auth gate.

The condition at Line 73 only checks admin_path, but a direct request to /admin.html will skip this block entirely and fall through to serve_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 | 🟠 Major

SPA fallback for missing assets serves HTML with wrong Content-Type.

When a request for /app.js misses in KV storage, the code falls back to index.html (Lines 30-34) but then determines the MIME type from the original key ("app.js"), resulting in HTML content served as application/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

📥 Commits

Reviewing files that changed from the base of the PR and between 58ac310 and 5bcb55c.

📒 Files selected for processing (3)
  • src/http_utils.py
  • src/static_utils.py
  • src/worker.py

@Shrey1006
Copy link
Copy Markdown
Contributor Author

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...

@A1L13N
Copy link
Copy Markdown
Contributor

A1L13N commented Apr 11, 2026

please resolve conflicts

…eb-crypto-aes-object-conversion

fix: Resolve Web Crypto AES-GCM Map conversion bug during encryption
@Shrey1006
Copy link
Copy Markdown
Contributor Author

Sure sir!

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (8)
src/api_activities.py (2)

220-225: ⚠️ Potential issue | 🟠 Major

Ignore the caller’s requested enrollment role on /api/join.

Any authenticated user can currently self-assign instructor or organizer. Public joins should always create participant enrollments; 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 | 🟠 Major

Validate tags before mutating the database, and re-read tag ids after upserts.

parse_json_object() only guarantees a dict, so tags="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 and continues instead of reusing the existing tag id. Normalizing tags to list[str] up front, then INSERT OR IGNORE + re-selecting by name before inserting into activity_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 | 🟠 Major

Protect the raw admin asset path too.

This branch only challenges admin_path. A direct request to /admin.html still falls through to serve_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 | 🟠 Major

Return 404 for missing asset files; reserve index.html fallback for routes.

A miss on /app.js or /logo.png currently falls back to index.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 | 🟠 Major

Expire signed tokens.

These tokens never become invalid on their own, so a leaked token stays usable until secret rotates. Add iat/exp to the signed payload, reject expired tokens in verify_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 | 🟠 Major

Move _PEPPER out 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 | 🟠 Major

Don't advance uid_map before the user insert outcome is known.

uid_map[uname] = uid happens before the write, and the broad except hides 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 IGNORE or 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 | 🟠 Major

Stop suppressing arbitrary seed failures.

These except Exception: pass blocks 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 explicit INSERT 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5bcb55c and f5c9003.

⛔ Files ignored due to path filters (1)
  • public/images/logo.png is excluded by !**/*.png
📒 Files selected for processing (14)
  • public/admin.html
  • public/course.html
  • public/dashboard.html
  • public/index.html
  • public/login.html
  • public/teach.html
  • src/api_activities.py
  • src/api_admin.py
  • src/api_auth.py
  • src/db_utils.py
  • src/http_utils.py
  • src/security_utils.py
  • src/static_utils.py
  • src/worker.py

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (2)
src/security_utils.py (2)

111-116: ⚠️ Potential issue | 🟠 Major

Move the password pepper to runtime configuration.

_PEPPER is 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 | 🟠 Major

Add iat/exp to signed auth tokens.

These bearer tokens never expire today, so a leaked token stays valid until JWT_SECRET rotates. Add a TTL in create_token(), enforce it in verify_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 data

As 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

📥 Commits

Reviewing files that changed from the base of the PR and between f5c9003 and 80a3033.

📒 Files selected for processing (2)
  • src/api_auth.py
  • src/security_utils.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants