Course Materials Upload #69
Open
Ananya44444 wants to merge 3 commits into
Open
Conversation
WalkthroughAdds a complete course materials feature to the platform. A new ChangesCourse Materials Feature
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser<br/>(activity.js)
participant Worker as CF Worker<br/>(worker.py)
participant MaterialsAPI as materials.py
participant R2 as Cloudflare R2
participant D1 as Cloudflare D1
rect rgba(99, 102, 241, 0.5)
note over Browser,D1: Upload flow with compensating rollback
Browser->>Worker: POST /api/activities/:id/materials (FormData)
Worker->>MaterialsAPI: upload_material(activity_id, req, env)
MaterialsAPI->>D1: SELECT activity EXISTS
MaterialsAPI->>R2: put(key, bytes, metadata)
MaterialsAPI->>D1: INSERT course_materials row
alt D1 insert succeeds
MaterialsAPI-->>Browser: 201 {id, title, file_key, ...}
else D1 insert fails
MaterialsAPI->>R2: delete(key) [compensating]
MaterialsAPI-->>Browser: 500 error
end
end
rect rgba(16, 185, 129, 0.5)
note over Browser,D1: Download flow with presigned URL
Browser->>Worker: GET /api/activities/:id/materials/:mid/download
Worker->>MaterialsAPI: download_material(activity_id, mid, req, env)
MaterialsAPI->>D1: SELECT material by (id, activity_id)
MaterialsAPI->>R2: createPresignedUrl(key) or /api/r2/ fallback
MaterialsAPI-->>Browser: {download_url, expires_in, filename}
Browser->>R2: GET signed URL (new tab)
end
rect rgba(239, 68, 68, 0.5)
note over Browser,D1: Delete flow with auth and best-effort R2 cleanup
Browser->>Worker: DELETE /api/activities/:id/materials/:mid
Worker->>MaterialsAPI: delete_material(activity_id, mid, req, env)
MaterialsAPI->>D1: DELETE course_materials row
MaterialsAPI->>R2: delete(key) [best-effort, errors swallowed]
MaterialsAPI-->>Browser: 200 ok
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Pull request overview
Adds a new “Course Materials” feature backed by Cloudflare R2 (binary storage) + D1 (metadata), with new Worker routes and a refreshed Activities UI to list/upload/edit/delete/download materials.
Changes:
- Adds D1 schema + migration for
course_materials, plus Wrangler R2 bucket binding. - Introduces backend API handlers for materials (list/upload/delete/update/download) and an
/api/r2/:keyproxy fallback for downloads. - Updates the Activities frontend (new
public/js/activity.js, updatedpublic/activity.html) and adds a standalonepublic/course-materials.htmlpage.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
wrangler.toml |
Binds an R2 bucket for materials storage. |
migrations/0006_add_course_materials.sql |
Adds course_materials table + indexes in D1. |
src/api/materials.py |
Implements the Materials API (R2 upload, D1 metadata, download URLs). |
src/worker.py |
Routes materials endpoints and adds an authenticated /api/r2/:key proxy. |
public/activity.html |
Reworks activity detail view into a tabbed UI including Materials. |
public/js/activity.js |
Implements tab logic + materials UI actions (upload/list/download/edit/delete). |
public/course-materials.html |
Adds a standalone materials page with upload/list/download/delete. |
tests/test_api_materials.py |
Adds unit tests for list/upload/delete/download + helper utilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/activity.html`:
- Around line 330-333: The refresh button element with the
onclick="loadMaterials()" handler is missing an explicit type attribute. Add
type="button" to the button tag to explicitly specify that this is a button
element rather than relying on default form submission behavior. This ensures
the button functions as intended without triggering any form submission.
- Around line 236-239: The button element with id="btn-join" and
onclick="joinActivity()" is missing an explicit type attribute. Add
type="button" to this button element to ensure it does not trigger form
submission behavior if it is placed inside a form element in the future. This is
a best practice for buttons that perform custom JavaScript actions rather than
form submission.
- Around line 396-414: The delete confirmation modal with id="mat-confirm-modal"
needs proper accessibility attributes and the buttons need explicit types. Add
aria-modal="true", aria-labelledby="mat-confirm-title", and
aria-describedby="mat-confirm-msg" to the modal div. Add id="mat-confirm-title"
to the h3 element containing "Delete Material". Add explicit type="button"
attributes to both the Cancel button (onclick="closeDeleteModal()") and the
Delete button (id="mat-confirm-ok") to ensure they are properly recognized as
buttons by assistive technologies.
- Around line 416-424: The modal close button in the edit material modal does
not have an explicit type attribute and lacks accessibility attributes. Locate
the button element with onclick="closeEditModal()" in the mat-edit-modal div and
add type="button" to explicitly specify it is a button element rather than
defaulting to submit behavior. Additionally, add an aria-label attribute with a
descriptive label like "Close modal" to improve accessibility for screen readers
and assistive technologies.
- Around line 180-193: Add the `type="button"` attribute to all four tab buttons
(the buttons with classes "tab-btn" and data-tab attributes for "overview",
"sessions", "materials", and "host") to explicitly prevent form submission
behavior. Additionally, add `aria-selected="true"` to the initially active
button (overview) and `aria-selected="false"` to the others. Then update the
switchTab() function to toggle the aria-selected attribute on all buttons,
setting it to true for the clicked tab button and false for all others, to
maintain proper accessibility state management.
In `@public/course-materials.html`:
- Around line 179-198: The confirm-modal div is missing critical accessibility
features required for keyboard and screen reader users. Add role="dialog" and
aria-labelledby attributes to the confirm-modal container, with aria-labelledby
pointing to the heading that says "Delete Material". Then implement a JavaScript
event listener that detects when the Escape key is pressed while the modal is
visible (not has the hidden class) and closes it by adding the hidden class
back. Additionally, implement a focus trap mechanism that prevents keyboard
focus from leaving the modal when users tab through the buttons (confirm-cancel
and confirm-ok) so that focus cycles only within the modal dialog.
- Around line 365-367: The auth token is being appended to the URL as a query
parameter in the condition checking for '/api/r2/', which exposes credentials in
browser history and server logs. Replace this pattern by either implementing a
short-lived, single-use download token to append to the URL instead of the
actual auth token, or refactor the code to use the fetch API with an
Authorization header and handle the blob response for download rather than
relying on anchor element navigation. If this pattern must remain as a fallback
for the R2 proxy endpoint, ensure the backend validates token scope and logs all
download attempts for security monitoring.
- Line 48: Add explicit `type="button"` attribute to all button elements that
are not form submission buttons to prevent them from defaulting to
`type="submit"`. Locate the button element with id="dark-toggle" on line 48 and
add `type="button"` to it. Also apply the same fix to any other non-submit
buttons found at lines 138 and 187-192 to ensure consistent behavior and prevent
unexpected form submissions.
- Around line 403-404: In the deleteBtn variable assignment, the mat.id value is
used unescaped in the data-id attribute while mat.title is properly escaped with
escHtml for the data-title attribute. Apply the same escHtml function to mat.id
when constructing the data-id attribute to maintain consistent security
practices. This same fix should also be applied to any other occurrences where
mat.id is used unescaped in HTML attributes.
- Line 105: Add client-side file size and type validation to the mat-file input
element to improve user experience by preventing unnecessary uploads of invalid
files. First, define a maximum file size constant that matches your backend
limit, then in the upload handler, check the selected file's size against this
limit before upload begins and display an error message if it exceeds the limit.
Additionally, add an accept attribute to the mat-file input element to restrict
which file types users can select, ensuring only supported file formats are
available for upload.
- Around line 554-561: The Safari fallback comment indicates the file input
remains empty when DataTransfer is not supported, but the UI shows the selected
filename, causing confusing errors on submit. Create a variable to track the
dropped file separately (outside the drop handler scope), store the file in this
variable when the DataTransfer assignment fails in the catch block, and then
modify the submit handler to use the dropped file as a fallback using
droppedFile or matFile.files[0] when building the FormData. This ensures the
file is available for upload even when the native file input assignment fails in
Safari.
In `@public/js/activity.js`:
- Around line 684-690: The setInterval ticker that simulates progress with
random increments can mislead users on large file uploads since the bar reaches
85% while actual upload progress is minimal. Replace the simulated progress
mechanism in the ticker setInterval block with actual upload progress tracking
by switching from fetch to XMLHttpRequest, implementing the
xhr.upload.onprogress event handler to update progBar.style.width and
progPct.textContent with real progress values calculated from e.loaded and
e.total, and remove the simulated ticker entirely.
- Around line 182-186: The tag cloud button generation uses inline onclick
attributes with unsanitized tag names, creating an XSS vulnerability where
malicious tag names could break out of the string and execute arbitrary
JavaScript. Replace the inline onclick handler approach in the sorted.map
function that generates the tag-pill buttons with an event delegation pattern
similar to the one used for the materials list buttons (around lines 783-799).
Attach a single event listener to the cloud container that identifies which tag
button was clicked and calls toggleTag with the appropriate tag name, rather
than embedding the tag name directly in the onclick attribute.
- Around line 141-161: The renderActivityCard function (and similar functions
like buildTagCloud, renderSimilarActivities, loadActivityDetail, and
renderMaterialItem) have a critical XSS vulnerability where user-provided data
such as act.title, act.description, and tag names are inserted directly into
HTML without escaping. Create an escapeHtml helper function at the top of the
file that safely escapes HTML special characters using a temporary DOM element's
textContent and innerHTML properties. Then update all instances in
renderActivityCard where user data is interpolated (act.title, act.description,
and tag values) to pass through this escapeHtml function. Apply the same
escaping pattern consistently across all other identified vulnerable render
functions throughout the file.
In `@src/api/materials.py`:
- Around line 187-193: The materials handler functions list_materials,
upload_material, and download_material do not enforce that the requesting user
is a host or participant of the target activity, allowing any authenticated user
to read and write materials across all activities. Add authorization checks to
each of these functions that verify the user has appropriate access (host or
participant status) to the activity before allowing the operation to proceed.
The authorization check should retrieve the user's relationship to the activity
and deny access if they are neither a host nor a participant.
- Around line 278-285: In the isinstance(file_field, dict) branch where
file_bytes is assigned from file_field.get("bytes"), the value may come as a
list from JSON and is passed to _upload_to_r2 unconverted. After assigning
file_bytes at line 280, add logic to detect if file_bytes is a list or bytearray
and convert it to bytes, then validate the byte range before the upload occurs.
In `@src/worker.py`:
- Around line 2737-2752: The code in the path handling for "/api/r2/" only
validates that the JWT token is valid using verify_token but does not enforce
material-level authorization to ensure the user has actual permission to access
the specific r2_key being requested. After the check for user validity (if not
user), add authorization logic to verify that the authenticated user has access
to the requested resource by querying course_materials or similar authorization
data to confirm the requester is the host, uploader, or an allowed participant
before proceeding to call env.R2_BUCKET.get(r2_key). This ensures that token
validity alone does not grant access to arbitrary object keys.
In `@tests/test_api_materials.py`:
- Around line 4-8: The test file covers list_materials, upload_material,
delete_material, and download_material operations but lacks a dedicated test
suite for the update_material endpoint. Add a new test suite that covers the
PATCH /api/activities/:activity_id/materials/:mid operation with test cases for:
verifying authentication is required before allowing updates, validating that
only the uploader or host can update a material, testing rejection of invalid
request bodies or invalid title values, and asserting successful updates with
proper response data. Follow the existing test patterns in the file to maintain
consistency.
In `@wrangler.toml`:
- Around line 22-25: The R2 bucket configuration for the R2_BUCKET binding is
missing a preview_bucket_name parameter, which causes preview deployments to use
the production bucket alphaonelabs-production. Add a preview_bucket_name field
to the r2_buckets section with a value of alphaonelabs-preview (or another
appropriate preview bucket) to ensure preview and development deployments use a
separate bucket instead of the production environment, preventing accidental
data corruption or deletion of course materials.
🪄 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: 681a3143-f9dc-4b8e-96d3-a52245747bca
⛔ Files ignored due to path filters (1)
migrations/0006_add_course_materials.sqlis excluded by!**/migrations/**
📒 Files selected for processing (7)
public/activity.htmlpublic/course-materials.htmlpublic/js/activity.jssrc/api/materials.pysrc/worker.pytests/test_api_materials.pywrangler.toml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/worker.py (1)
2763-2768:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential issue with filename derivation when R2 key format is unexpected.
The filename extraction at line 2768 assumes the R2 key always contains a 36-character UUID followed by an underscore. If
raw_nameis exactly 37 characters or shorter,raw_name[37:]could produce an empty string, which would result in aContent-Dispositionheader with an empty filename.Consider adding a fallback:
raw_name = r2_key.split("/")[-1] -filename = raw_name[37:] if len(raw_name) > 37 else raw_name +filename = raw_name[37:] if len(raw_name) > 37 else raw_name +if not filename: + filename = "download"This matches the fallback pattern used in
download_materialinsrc/api/materials.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/worker.py` around lines 2763 - 2768, The filename extraction logic in the code block currently assumes the R2 key always contains a 36-character UUID followed by an underscore, but if raw_name is exactly 37 characters or shorter, the slice operation raw_name[37:] produces an empty string. Add a fallback condition after extracting the filename to check if the result is empty, and if so, use raw_name as the fallback filename instead. This ensures the Content-Disposition header always has a valid filename value, matching the fallback pattern used in the download_material function in src/api/materials.py.public/course-materials.html (1)
520-525:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSafari drag-and-drop fix is incomplete —
droppedFileis stored but never used.Great work adding the
droppedFilevariable at line 577 to handle Safari's lack ofDataTransfersupport! However, the submit handler still only checksmatFile.files[0]and doesn't fall back todroppedFile. This means Safari users will still see the "Please select a file to upload" error despite the UI showing a file is selected.🔧 Suggested fix to use droppedFile as fallback
var title = matTitle.value.trim(); var desc = matDesc.value.trim(); - var file = matFile.files[0]; + var file = matFile.files[0] || droppedFile; if (!title) { setUploadError('Title is required.'); return; } if (!file) { setUploadError('Please select a file to upload.'); return; }You'll also want to clear
droppedFileafter a successful upload to prevent stale state:// Inside the .then() callback after successful upload (around line 546) droppedFile = null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/course-materials.html` around lines 520 - 525, The submit handler is not using the droppedFile fallback variable that was added for Safari support. In the file upload validation section, the code checking `if (!file) { setUploadError(...) }` only evaluates matFile.files[0] and ignores droppedFile. Update the file variable assignment to check matFile.files[0] first, and fall back to droppedFile if the file input is empty. Additionally, after a successful upload completes in the .then() callback (around line 546), reset droppedFile to null to clear stale state and prevent unexpected behavior on subsequent uploads.public/js/activity.js (2)
313-348:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing HTML escaping in
loadActivityDetailcreates XSS vulnerabilities.While
escHtmlis properly used inrenderActivityCard,buildTagCloud,renderSimilarActivities, andrenderMaterialItem, theloadActivityDetailfunction has severalinnerHTMLassignments that don't escape user-controlled data:
Line(s) Unescaped values 313-331 act.type,act.host_name337-339 Tag names in act-badges346-348 Tag names in act-tagsA malicious host could craft an activity with a
host_namelike<img src=x onerror=alert(1)>or create a tag with embedded JavaScript.🔒 Suggested fix for lines 313-331
if (detailsEl) { detailsEl.innerHTML = ` <div class="flex items-center gap-3 text-sm"> <i class="fas fa-tag w-5 text-teal-300"></i> - <span class="capitalize">${act.type || '—'}</span> + <span class="capitalize">${escHtml(act.type || '—')}</span> </div> <div class="flex items-center gap-3 text-sm"> <i class="fas fa-globe w-5 text-teal-300"></i> - <span class="capitalize">${(act.format || '—').replace('_', ' ')}</span> + <span class="capitalize">${escHtml((act.format || '—').replace('_', ' '))}</span> </div> <div class="flex items-center gap-3 text-sm"> <i class="fas fa-users w-5 text-teal-300"></i> <span>${memberCount} enrolled</span> </div> <div class="flex items-center gap-3 text-sm"> <i class="fas fa-user-tie w-5 text-teal-300"></i> - <span>${act.host_name || 'Unknown Host'}</span> + <span>${escHtml(act.host_name || 'Unknown Host')}</span> </div> - ${act.created_at ? `<div class="flex items-center gap-3 text-sm"><i class="fas fa-calendar w-5 text-teal-300"></i><span>Created ${formatDate(act.created_at)}</span></div>` : ''} + ${act.created_at ? `<div class="flex items-center gap-3 text-sm"><i class="fas fa-calendar w-5 text-teal-300"></i><span>Created ${escHtml(formatDate(act.created_at))}</span></div>` : ''} `; }🔒 Suggested fix for tag badges (lines 337-348)
// ── Hero badges ── const badgesEl = document.getElementById('act-badges'); if (badgesEl) { badgesEl.innerHTML = (act.tags || []).map(t => - `<span class="badge bg-white/20 text-white text-xs">${t}</span>` + `<span class="badge bg-white/20 text-white text-xs">${escHtml(t)}</span>` ).join(''); } // ── Overview sidebar: description + tags ── setText('act-description', act.description || 'No description provided.'); const actTagsEl = document.getElementById('act-tags'); if (actTagsEl) { actTagsEl.innerHTML = (act.tags || []).map(t => - `<span class="badge bg-teal-100 text-teal-800 dark:bg-teal-900/40 dark:text-teal-300 text-xs">${t}</span>` + `<span class="badge bg-teal-100 text-teal-800 dark:bg-teal-900/40 dark:text-teal-300 text-xs">${escHtml(t)}</span>` ).join('') || '<span class="text-sm text-gray-400">No tags</span>'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/js/activity.js` around lines 313 - 348, The loadActivityDetail function has multiple XSS vulnerabilities where user-controlled data is directly inserted into innerHTML without escaping. In the detailsEl section (around lines 313-331), escape act.type and act.host_name using the escHtml function before inserting them into the HTML template strings. Similarly, when rendering badges in the badgesEl element and tags in the actTagsEl element (lines 337-348), escape each tag value using escHtml before creating the span HTML. Apply escHtml consistently to all user-controlled values being inserted via innerHTML, matching the pattern already used in renderActivityCard, buildTagCloud, renderSimilarActivities, and renderMaterialItem functions.
362-377:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnrollment role also needs escaping.
Following the same pattern,
data.enrollment?.roleat line 373 should be escaped:${data.enrollment?.role ? `<span class="badge bg-green-100 text-green-800 dark:bg-green-900/40 dark:text-green-300 text-xs">${data.enrollment.role}</span>` : ''}`; + ${data.enrollment?.role ? `<span class="badge bg-green-100 text-green-800 dark:bg-green-900/40 dark:text-green-300 text-xs">${escHtml(data.enrollment.role)}</span>` : ''}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/js/activity.js` around lines 362 - 377, The data.enrollment?.role value is being inserted directly into the innerHTML template string without proper escaping, which creates a potential XSS vulnerability. Apply the same escaping pattern that is used elsewhere in the codebase (likely a utility function for HTML escaping) to the data.enrollment?.role expression within the conditional template string on the line where enrDetails.innerHTML is set for enrolled users. This ensures user-provided role data cannot be exploited to inject malicious HTML or JavaScript.
♻️ Duplicate comments (3)
src/api/materials.py (2)
241-341:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUpload endpoint allows any authenticated user to upload to any activity.
The
upload_materialhandler validates that the activity exists and the user is authenticated, but doesn't verify that the user is the activity host or an enrolled participant. This means any authenticated user can upload materials to any activity.If this is intentional (e.g., allowing community contributions), consider documenting this in the docstring. If uploads should be restricted to hosts only:
if not act: return w.err("Activity not found", 404) + +# Verify user is the activity host +if act.host_id != user["id"]: + return w.err("Only the activity host can upload materials", 403)You would need to modify the SELECT query to include
host_idin the result.This was flagged in a previous review as a known authorization gap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/materials.py` around lines 241 - 341, The upload_material function currently only verifies that the activity exists and the user is authenticated, but does not check if the user is authorized to upload materials to that specific activity. Modify the SELECT query in the activity lookup section (where it queries "SELECT id FROM activities WHERE id = ?") to also retrieve the host_id field, then add an authorization check after retrieving the activity record that compares the current user's id (from the user object) with the activity's host_id. If they do not match, return an appropriate 403 Forbidden error before proceeding with file upload and database insertion.
466-515:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDownload endpoint allows any authenticated user to download any material.
Similar to the upload endpoint,
download_materialrequires authentication but doesn't verify the user has access to the activity (host, uploader, or enrolled participant). Any authenticated user can generate download URLs for any material.Since
list_materialsis public, an attacker could enumerate all material IDs and then download them with any valid auth token.If materials should be restricted to enrolled participants or hosts, consider adding an access check:
# Check if user is host, uploader, or enrolled participant is_host = (act.host_id == user["id"]) is_enrolled = await env.DB.prepare( "SELECT 1 FROM enrollments WHERE activity_id=? AND user_id=?" ).bind(activity_id, user["id"]).first() if not is_host and not is_enrolled: return w.err("Access denied", 403)This was flagged in a previous review as a known authorization gap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/materials.py` around lines 466 - 515, The download_material function performs authentication verification but lacks authorization checks to ensure the user has access to the activity before allowing material downloads. After fetching the material row successfully, add a check to verify that the authenticated user is either the activity host or an enrolled participant in that activity. If the user lacks access, return a 403 error. Consider fetching the activity record (to get host_id) alongside the material lookup, then verify the user matches the host_id or has an enrollment record for that activity_id in the enrollments table before proceeding with URL generation.src/worker.py (1)
2737-2752:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAuthorize
/api/r2/<key>by resource access, not just token validity.The R2 proxy at lines 2737-2752 validates that the user has a valid JWT and restricts keys to the
materials/prefix, but any authenticated user can download any material file. This bypasses any activity-level access controls you might want to enforce (e.g., restricting materials to enrolled participants or activity hosts).Consider adding a database lookup to verify the requester has access to the specific material before streaming the R2 object. For example:
# Extract activity_id from key: materials/{activity_id}/{uuid}_{filename} parts = r2_key.split("/") if len(parts) < 2: return err("Invalid R2 key format", 400) activity_id = parts[1] # Verify user has access to this activity (enrolled, host, or public) # ... authorization check here ...This is flagged as a known issue from a previous review. If public download access for all authenticated users is intentional for this educational platform, consider documenting that design decision.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/worker.py` around lines 2737 - 2752, The R2 proxy endpoint at the `/api/r2/` GET path currently only validates that a user has a valid JWT token but does not verify they have access to the specific material being requested. After validating the token with verify_token(), add authorization logic to extract the activity_id from the r2_key (which follows the format materials/{activity_id}/{uuid}_{filename}), query the database to confirm the user has access to that activity (enrolled, is host, or activity is public), and return an authorization error if the user lacks access to the specific material resource.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/js/activity.js`:
- Around line 261-269: Refactor the map callback for the similar activities list
to improve readability. Convert the arrow function from an implicit template
literal return to an explicit function body with a return statement. Move the
escaped value computations (sid, stitle, stype, sfmt variables created by
escHtml calls) to the beginning of the function before the template literal,
eliminating the immediately-invoked function expression wrapper. This makes the
escaping logic clearer and easier to follow while maintaining the same
functionality in the onclick handler and template output.
---
Outside diff comments:
In `@public/course-materials.html`:
- Around line 520-525: The submit handler is not using the droppedFile fallback
variable that was added for Safari support. In the file upload validation
section, the code checking `if (!file) { setUploadError(...) }` only evaluates
matFile.files[0] and ignores droppedFile. Update the file variable assignment to
check matFile.files[0] first, and fall back to droppedFile if the file input is
empty. Additionally, after a successful upload completes in the .then() callback
(around line 546), reset droppedFile to null to clear stale state and prevent
unexpected behavior on subsequent uploads.
In `@public/js/activity.js`:
- Around line 313-348: The loadActivityDetail function has multiple XSS
vulnerabilities where user-controlled data is directly inserted into innerHTML
without escaping. In the detailsEl section (around lines 313-331), escape
act.type and act.host_name using the escHtml function before inserting them into
the HTML template strings. Similarly, when rendering badges in the badgesEl
element and tags in the actTagsEl element (lines 337-348), escape each tag value
using escHtml before creating the span HTML. Apply escHtml consistently to all
user-controlled values being inserted via innerHTML, matching the pattern
already used in renderActivityCard, buildTagCloud, renderSimilarActivities, and
renderMaterialItem functions.
- Around line 362-377: The data.enrollment?.role value is being inserted
directly into the innerHTML template string without proper escaping, which
creates a potential XSS vulnerability. Apply the same escaping pattern that is
used elsewhere in the codebase (likely a utility function for HTML escaping) to
the data.enrollment?.role expression within the conditional template string on
the line where enrDetails.innerHTML is set for enrolled users. This ensures
user-provided role data cannot be exploited to inject malicious HTML or
JavaScript.
In `@src/worker.py`:
- Around line 2763-2768: The filename extraction logic in the code block
currently assumes the R2 key always contains a 36-character UUID followed by an
underscore, but if raw_name is exactly 37 characters or shorter, the slice
operation raw_name[37:] produces an empty string. Add a fallback condition after
extracting the filename to check if the result is empty, and if so, use raw_name
as the fallback filename instead. This ensures the Content-Disposition header
always has a valid filename value, matching the fallback pattern used in the
download_material function in src/api/materials.py.
---
Duplicate comments:
In `@src/api/materials.py`:
- Around line 241-341: The upload_material function currently only verifies that
the activity exists and the user is authenticated, but does not check if the
user is authorized to upload materials to that specific activity. Modify the
SELECT query in the activity lookup section (where it queries "SELECT id FROM
activities WHERE id = ?") to also retrieve the host_id field, then add an
authorization check after retrieving the activity record that compares the
current user's id (from the user object) with the activity's host_id. If they do
not match, return an appropriate 403 Forbidden error before proceeding with file
upload and database insertion.
- Around line 466-515: The download_material function performs authentication
verification but lacks authorization checks to ensure the user has access to the
activity before allowing material downloads. After fetching the material row
successfully, add a check to verify that the authenticated user is either the
activity host or an enrolled participant in that activity. If the user lacks
access, return a 403 error. Consider fetching the activity record (to get
host_id) alongside the material lookup, then verify the user matches the host_id
or has an enrollment record for that activity_id in the enrollments table before
proceeding with URL generation.
In `@src/worker.py`:
- Around line 2737-2752: The R2 proxy endpoint at the `/api/r2/` GET path
currently only validates that a user has a valid JWT token but does not verify
they have access to the specific material being requested. After validating the
token with verify_token(), add authorization logic to extract the activity_id
from the r2_key (which follows the format
materials/{activity_id}/{uuid}_{filename}), query the database to confirm the
user has access to that activity (enrolled, is host, or activity is public), and
return an authorization error if the user lacks access to the specific material
resource.
🪄 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: 25cbd003-9c86-4606-a15b-4a651f6ed9d9
📒 Files selected for processing (7)
public/activity.htmlpublic/course-materials.htmlpublic/js/activity.jssrc/api/materials.pysrc/worker.pytests/test_api_materials.pywrangler.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Adds course materials upload/download functionality, allowing activity hosts to share files with participants via Cloudflare R2 object storage. Materials metadata is stored in D1; actual files are kept in R2 with presigned download URLs.
Backend API
Database
Worker Routing
Frontend
Tests
Prerequisites for Deployment
Before merging, verify: R2 bucket exists
Storage Architecture
course_materialstable stores title, description, uploader, timestampsalphaonelabs-productionbucketPurpose
This PR adds activity-scoped course materials upload, management, and download so hosts can share files with participants. Files are stored in Cloudflare R2, while material metadata is stored in Cloudflare D1.
Backend Changes
src/api/materials.py) supporting:multipart/form-data(with JSON fallback for tests)materials/{activity_id}/{uuid}_{sanitised_filename}, with R2 metadata set for content type / content disposition when available.Database Changes
course_materialstable created via migration (src/worker.py) storing:activity_id,title,description,file_key,uploaded_by, and timestampsactivities(cascade delete) andusersactivity_idand uploader/timeR2 Proxy Endpoint
GET /api/r2/{key}endpoint to fetch files from R2 for constrained proxying.materials/) and streams the object back with a derived filename.Route Registration
GET/POST /api/activities/{activity_id}/materials(list/upload)PATCH /api/activities/{activity_id}/materials/{material_id}(update)DELETE /api/activities/{activity_id}/materials/{material_id}(delete)GET /api/activities/{activity_id}/materials/{material_id}/download(download)Frontend Changes
public/activity.html: refactors the activity detail UI to introduce a sticky tab system (including a Materials tab), removes large inline activity logic, and loads external controllers (/js/layout.js,/js/activity.js).public/course-materials.html(new): standalone materials management page with:/api/r2/URLs)public/js/activity.js: expands the activity controller to manage:download_urland using blob download when proxying)Testing
tests/test_api_materials.py, ~754 LOC) for:Configuration / Deployment Note
wrangler.tomlupdated with an R2 bucket binding toalphaonelabs-production.