fix(lms): Stop echoing the user object with troop token in responses - #1248
fix(lms): Stop echoing the user object with troop token in responses#1248jeromehardaway wants to merge 1 commit into
Conversation
Three LMS endpoints (courses, test, admin-only) returned req.user in the response body, exposing the caller's troopToken (their J0dI3 access credential) to the browser. The courses endpoint drops the echo entirely; the two diagnostics return a sanitized id/email/role subset. Regression tests assert no troopToken in any LMS response body.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a security issue where several LMS API endpoints were including req.user in response bodies, unintentionally leaking the caller’s troopToken to the browser.
Changes:
- Removed
req.userentirely fromGET /api/lms/coursesresponses. - Updated
GET /api/lms/testandGET /api/lms/admin-onlyto return only a sanitized subset of identity fields (no token). - Added regression tests ensuring LMS responses do not include
troopToken(key) or the token value.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/pages/api/lms/test.ts | Replaces full req.user echo with a sanitized identity subset in the response payload. |
| src/pages/api/lms/courses/index.ts | Removes user from the response body to prevent token leakage. |
| src/pages/api/lms/admin-only.ts | Returns only a minimal, sanitized identity subset instead of echoing req.user. |
| tests/pages/api/lms/no-user-echo.test.ts | Adds regression coverage to prevent reintroducing token/user echoing across LMS endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Do not echo req.user — it carries the caller's troop access token. | ||
| res.status(200).json({ | ||
| user: req.user, | ||
| courses: coursesWithStats, | ||
| message: "Courses retrieved successfully", | ||
| }); |
There was a problem hiding this comment.
Since the user field is being removed from the /api/lms/courses response to prevent exposing the troop token, should the documented response at the top of index.ts also be updated to remove user: { id, email, role } so it matches the new response shape?
msgem0523
left a comment
msgem0523
left a comment
There was a problem hiding this comment.
Since the user field is being removed from the /api/lms/courses response to prevent exposing the troop token, should the documented response at the top of index.ts also be updated to remove user: { id, email, role } so it matches the new response shape?
This one will have to be nuked since we are moving LMS completely to Jodie in a separate repo. |
Problem
Three LMS endpoints — including the production
GET /api/lms/courses— returnedreq.userin the response body. That object includestroopToken, the caller's J0dI3 access credential, so every course listing shipped a bearer credential to the browser.Fix
GET /api/lms/courses: response echo removed entirely — the endpoint returns courses, not identity.GET /api/lms/testandGET /api/lms/admin-only(diagnostics whose point is proving auth/RBAC): return a sanitized{ id, email, role }/{ id, role }subset — never the token.Verification
__tests__/pages/api/lms/no-user-echo.test.ts, 3 passing) drive each handler with an authenticated user carrying a sentinel token and assert the serialized response contains neither the token value nor atroopTokenkey.npm run typecheck— pass.Closes #1196