fix(security): stop serving course videos to anyone who asks - #81
Conversation
/uploads was mounted with express.static and no auth, and the public catalogue endpoint returned whole course documents — including every section's S_content.path. One anonymous request listed the storage path of every video in the system and a second downloaded it, so enrolment and payment could both be skipped entirely. Fixing only the listing would not have been enough: the directory would still have been readable by anyone who had ever seen a path. - The catalogue drops `sections` and reports a sectionCount instead. A card renders nothing from inside a section, so the paths were never needed there. - /uploads is no longer served. Section videos come from GET /api/user/coursevideo/:courseid/:sectionIndex. - A <video> element cannot send an Authorization header, so that route takes its credential from the query string — but not the session token, which is good for a day against every endpoint and would end up in history, Referer headers and access logs. playbackTokens.js mints a 30-minute token scoped to one course and to reading video, issued by /coursecontent, which is the only place that has already checked enrolment. The scope check is what stops a session token from being accepted here. - The stored path is resolved through resolveSafeUploadPath, so a traversal that ever reached the database cannot become an arbitrary file read. - Range requests are handled, since express.static was doing that for free and seeking is unusable without it. 206 with Content-Range, 416 past the end, and the suffix form a player uses to read the MP4 index. Responses are private, no-store, because the URL that produced them carries a credential. Closes udaycodespace#76
046277a to
710212b
Compare
|
Heads up on merge order: this branch and #78 both add to the import block in I rebased this branch on top of #78 (which itself sits on #77) and resolved it. Both conflicts were pure import additions, so the resolution keeps both sides; no logic was dropped from either PR. So this PR currently shows #77 and #78's commits in its diff. Merge those two first and this collapses to just its own changes. Verified: merging #77 → #78 → #79 → #80 → #81 in sequence is conflict-free, and the combined result passes 190 backend tests and 22 frontend tests with a clean #79 and #80 touch nothing these three touch and can merge in any order. |
|
@MOHITKOURAV01 The PR looks good overall.
|
|
@MOHITKOURAV01 I followed the lower-risk PR review and merge order first, which led to these conflicts. Please fix the conflicts and rebase the active PRs accordingly across #68, #77, #78, and #81 |
The same three conflicts as fix/73-otp-resend, resolved the same way: - userControllers.js require block: the paymentDetails require this branch carried has moved into enrollmentController (udaycodespace#62) and nothing here uses it, so only accountIdentity is kept. - Register.jsx imports: main's ROLES/roleLabel (udaycodespace#84) alongside this branch's Toast and VerifyEmailPanel. - Register.jsx handleSelect: main's implementation, with this branch's toast helpers and pending-email effect in front of it.
Overlap with #98, worth knowing aboutBoth merge cleanly into
They pull in the same direction. If #98 lands first the resolution gets easier, not harder: it adds Whichever you take second, tell me and I will push the resolution. |
The same four conflicts as the resend branch this one is stacked on, from udaycodespace#63's credential throttling and udaycodespace#72's email normalisation landing on main. registerController: this branch's version supersedes main's on both hunks — the verified/unverified split replaces the unconditional "User already exists", and issueVerificationOtp saves and mails in one helper, already carrying main's isDuplicateOn catch in the right place. userRoutes: main's rate limiter and failure throttle are kept on verify-otp, forgot-password and reset-password, and /resend-otp is given a rate limiter for the same reason /forgot-password has one and no failure throttle for the same reason it has none. The video routes this branch adds are untouched.
Hey @MOHITKOURAV01 , no worries. The conflicts are happening because I’m reviewing PRs based on LOC, starting with the smaller ones instead of following PR order. For now, please pause raising new PRs and just resolve the conflicts in the existing ones. Once those are cleared, we can move to the next PRs. This will make the review flow easier. |
GitHub reported this branch as conflicting; the merge resolves textually
with no conflicted hunks. Everything main landed while this was open —
the payments and bookmarks aggregations, the navbar panel links, the
upload validation — sits beside this change rather than on top of it.
Backend 435 pass, frontend 169, build clean.
One note: the first full backend run reported a single failure in
payment-listing ("the endpoint returns one page and the true total").
That suite passes in isolation, and three consecutive full runs are
clean, so it is a timing artefact of the shared mongodb-memory-server
under load rather than anything this merge did. Flagging it rather than
quietly re-running until green.
|
@MOHITKOURAV01 Conflicts resolved! Ready to merge! |
Summary
Course videos were downloadable with no account at all. Two things combined:
app.jsserved the upload directory to the world —app.use("/uploads", express.static(uploadsDir)), no auth, no enrolment check.And the public catalogue published the filenames.
GET /api/user/getallcourseshas noauthMiddlewareon it (a visitor is meant tobrowse) and
courseListingControllerreturned whole course documents with.lean().sectionsis part of that document and every section carriesS_content.path.So one anonymous request listed the storage path of every video in the system,
and a second downloaded it:
Fixing only the listing would not have been enough — the directory would still
be readable by anyone who had ever legitimately seen a path, including a
student who enrolled once, and anyone they forwarded the URL to.
Related Issue
Closes #76
What changed
sectionsand reports asectionCountinstead. A cardrenders nothing from inside a section, so the paths were never needed there.
/uploadsis no longer served. Section videos come fromGET /api/user/coursevideo/:courseid/:sectionIndex.<video>element cannot send anAuthorizationheader, so that routetakes its credential from the query string — but not the session token,
which is good for a day against every endpoint and would end up in browser
history,
Refererheaders and access logs.utils/playbackTokens.jsmints a30-minute token scoped to one course and to reading video, issued by
/api/user/coursecontent/:courseid, which is the only place that has alreadychecked enrolment.
resolveSafeUploadPath(already in the treefrom [Security]: Restrict teacher course deletion to owned courses #40), so a traversal that ever reached the database cannot turn this
route into an arbitrary file read.
express.staticwas doing that for free, andseeking is unusable without it — a player also reads the MP4 index with a
suffix range (
bytes=-200) before it can start. 206 withContent-Range,416 past the end of the file.
Cache-Control: private, no-store, since the URL that producedthem carries a credential.
Type
Areas touched
Testing
npm testinbackend/: 150 passing (128 before, 22 added).npm run buildin
frontend/passes.Test steps
Then open a course you are enrolled in and scrub the timeline — seeking still
works.
Screenshots
Edge cases checked
Other edge case details
session token all get the same 401 — distinguishing them would tell a caller
which part of the guess was right.
sectionsis declared as{}on the schema, so a course where it is not anarray reports
sectionCount: 0rather than throwing.filenameonly havepath;both shapes are read.
seed.jswrites paths for files that do not exist. Those sections now 404from the stream route instead of from the static handler — same outcome,
different place.
Checklist
CONTRIBUTING.mdNotes
The load-bearing line is the scope check in
verifyPlaybackToken. Bothtokens are signed with
JWT_SECRET, so without it any session token would openthis route for every course — which is most of what the fix is for. There is a
test that fails if it is removed.
The admin course listing is untouched: it is behind
requireAdmin, and theadmin dashboard is the one place a full document is legitimate.
One consequence worth flagging:
/coursecontentnow returns each section witha
streamUrlinstead ofS_content.path/filename.CourseContent.jsxisthe only consumer and is updated in this PR, but any external client reading
that field will need to follow.
docs/issue-76-video-access-control.mdhas the reasoning, including whyfixing the listing alone was not enough.