Skip to content

Secure progress and leaderboard endpoints#1023

Open
tanvishdesai wants to merge 1 commit into
vicharanashala:mainfrom
tanvishdesai:fix/progress-endpoint-security
Open

Secure progress and leaderboard endpoints#1023
tanvishdesai wants to merge 1 commit into
vicharanashala:mainfrom
tanvishdesai:fix/progress-endpoint-security

Conversation

@tanvishdesai

Copy link
Copy Markdown

Security: protect progress mutation and leaderboard endpoints

Summary

This PR tightens two progress-related endpoints:

  • Requires authentication and course-level progress permissions for the bulk watch-time backfill endpoint.
  • Converts the deprecated leaderboard/no-auth route into an authenticated compatibility alias and removes email exposure from the no-auth response shape/service path.

Where the problem was

  • backend/src/modules/users/controllers/ProgressController.ts
    • POST /users/progress/watch-time/bulk
    • GET /users/progress/courses/:courseId/versions/:versionId/leaderboard/no-auth
  • backend/src/modules/users/classes/validators/ProgressValidators.ts
  • backend/src/modules/users/services/ProgressService.ts

Root cause

The bulk watch-time endpoint was a mutation route without @Authorized() and without a CASL ability check. It accepted courseId, versionId, and userId from the request body and delegated directly to createBulkWatchiTimeDocs.

The leaderboard route was explicitly labelled as a temporary no-auth endpoint. Its service path loaded user records and returned names and email addresses in leaderboard rows.

Redacted vulnerable shape before this change:

@Post('/progress/watch-time/bulk')
async createBulkWatchiTimeDocs(@Body() body: any): Promise<any> {
  const { courseId, versionId, userId } = body;
  return this.progressService.createBulkWatchiTimeDocs(courseId, versionId, userId);
}

@Get('/progress/courses/:courseId/versions/:versionId/leaderboard/no-auth')
async getNoAuthLeaderboard(@Params() params: GetUserProgressParams) {
  return await this.progressService.getLeaderboardNoAuth(courseId, versionId);
}

Risk

An unauthenticated mutation endpoint can allow progress/watch-time records to be created without a verified actor or permission context. This can affect learner progress integrity and course completion records.

The public leaderboard route can expose private learner metadata and allows course participation data to be enumerated without login.

What changed

  • Added @Authorized() to POST /progress/watch-time/bulk.
  • Added a required courseId/versionId validation guard.
  • Added @Ability(getProgressAbility) and require ability.can('manage', subject('Progress', ...)) before backfilling watch-time records.
  • Added @Authorized() to the deprecated leaderboard/no-auth route.
  • Added course-scoped CASL authorization to both leaderboard routes.
  • Routed the deprecated leaderboard alias through the existing authenticated getLeaderboard service method.
  • Updated leaderboard OpenAPI response metadata to describe the actual paginated response, including myStats.
  • Removed email from LeaderboardNoAuthResponse.
  • Removed email mapping from getLeaderboardNoAuth so the legacy service path is also sanitized if it is accidentally used elsewhere.

Why this fixes it

The bulk endpoint now requires a logged-in actor and applies the same course-scoped permission model already used by the progress controller. Students keep normal self-progress capabilities, but cannot call the maintenance backfill operation because it requires manage on the course progress subject.

The deprecated leaderboard route remains available for compatibility, but it now requires authentication and returns the same privacy-preserving shape as the normal leaderboard endpoint.

Both leaderboard routes now require the requester to have view or manage permission on the requested course/version progress subject, preventing an arbitrary logged-in user from enumerating another course's leaderboard.

Safe validation plan

Run these checks only against a local development instance or an authorized staging environment:

  • Anonymous request to POST /users/progress/watch-time/bulk should return 401 Unauthorized.
  • Authenticated learner request to the bulk backfill endpoint should return 403 Forbidden.
  • Authenticated instructor/manager/admin with course access should be able to run the endpoint for an authorized course fixture.
  • Anonymous request to the deprecated leaderboard alias should return 401 Unauthorized.
  • Authenticated user without access to the requested course/version should receive 403 Forbidden.
  • Authenticated leaderboard response should not contain an email field in any row.

Files changed

  • backend/src/modules/users/controllers/ProgressController.ts
  • backend/src/modules/users/classes/validators/ProgressValidators.ts
  • backend/src/modules/users/services/ProgressService.ts

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant