Repository navigation
fix(backend): wrap lastfm routes with asynchandler - #1726
Merged
Merged
Conversation
📝 WalkthroughWalkthroughLast.fm routes now use shared async handling, rate limiting, authenticated identity preference, and strict query/cookie state matching. Integration tests cover the revised callback paths. Spotify formatting and frontend bundle documentation/configuration changes preserve behavior. ChangesLast.fm OAuth route hardening
Frontend bundle documentation formatting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant LastfmRoutes
participant StateCookie
participant LastfmAPI
Browser->>LastfmRoutes: Send callback state and authorization code
LastfmRoutes->>StateCookie: Clear and read lastfm_state
LastfmRoutes->>LastfmRoutes: Validate and compare both states
LastfmRoutes->>LastfmAPI: Exchange authorization code
LastfmAPI-->>LastfmRoutes: Return access token
LastfmRoutes-->>Browser: Redirect with callback result
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Failed to generate code suggestions for PR |
|
github-actions
Bot
added
dependencies
|
Size Change: 0 B Total Size: 493 kB ℹ️ View Unchanged
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
Auto-approved: Wraps three lastfm route handlers with asyncHandler for consistent error handling; lockfile updated automatically. No logic changes.
Re-trigger cubic
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
wraps async route handlers with asyncHandler middleware for consistent error handling and prevention of unhandled promise rejections (/api/lastfm/status, /api/lastfm/unlink, callback). Closes #1624
LucasSantana-Dev
force-pushed
the
fix-1624-lastfm-asynchandler
branch
from
July 9, 2026 04:10
bec939c to
311a411
Compare
github-actions
Bot
removed
the
dependencies
LucasSantana-Dev
enabled auto-merge (squash)
July 9, 2026 04:11
This was referenced Jul 9, 2026
- Add apiLimiter middleware to /api/lastfm/status and /api/lastfm/unlink to prevent denial-of-service attacks (js/missing-rate-limiting) - Fix user-controlled-bypass in /api/lastfm/connect by prioritizing authenticated user's ID over user-supplied state parameter. When a user is authenticated, their own discordId is always used for linking, not a potentially attacker-controlled state value (js/user-controlled-bypass)
…urity fixes Combines: - Async handler wrapping from fix-1624-lastfm-asynchandler - Cookie validation from origin/main commit 4b0d267 - Rate limiting middleware fixes (js/missing-rate-limiting) - User-controlled-bypass fix (js/user-controlled-bypass)
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The callback handler accepted query state without matching it against the cookie, allowing replay attacks. Now both must exist and match exactly to prevent an attacker from linking victims' Last.fm accounts to their own Discord ID.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: OAuth callback state validation and error handling changes in lastfm.ts are logic modifications in a critical auth path that require human review.
Re-trigger cubic
The CSRF fix correctly requires both query state and cookie state to exist and match for replay attack protection. Tests were written for pre-fix code that had no cookie requirement. In a real browser flow: /connect sets state cookie → Last.fm redirects back with state in query → browser sends both cookie and query. Tests now simulate this by setting the state cookie to match the query state in all callback tests. All tests pass while preserving CSRF protection.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Changes touch business logic in Last.fm OAuth flow (state validation, cookie handling), route wrapping, and rate limiting. The number of files is small but the blast radius includes authentication and data linking. Human review is needed to confirm security and correctness.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/backend/tests/integration/routes/lastfm.test.ts (2)
369-383: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the callback consumes the state cookie.
The route now clears
lastfm_state, but this success-path test only verifies linking. Assert that the response expires the state cookie to protect the one-time-state contract from regressions.🤖 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 `@packages/backend/tests/integration/routes/lastfm.test.ts` around lines 369 - 383, Extend the successful callback test to verify that the response expires or clears the lastfm_state cookie, in addition to confirming the redirect. Update the assertions in the test named “should link account on valid callback” to inspect Set-Cookie headers and confirm the state cookie is consumed.
392-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the now-duplicate callback test.
This test supplies identical state in both query and cookie, so it exercises the same matching path as the preceding success test and no longer tests “query-only” state. Convert it into a rejection test with a valid cookie but missing query state, or rename/remove it.
🤖 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 `@packages/backend/tests/integration/routes/lastfm.test.ts` around lines 392 - 403, Replace the duplicate success case around the callback test with a rejection test: retain a valid lastfm_state cookie but omit the state query parameter, then assert the callback rejects the request and does not invoke account linking or token exchange. Update the test name to reflect missing query state, using the existing buildState, mockExchangeToken, and mockSetLink symbols.
🤖 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 `@packages/backend/src/routes/lastfm.ts`:
- Line 168: Bind the OAuth callback state to the authenticated identity in the
Last.fm route: when req.user?.id is present, validate that providedState belongs
to that Discord ID or generate a fresh state for it, and reject mismatches
before continuing. Update the later callback/link-owner logic to use only the
validated state and authenticated ID, preventing discordIdFromState from
overriding req.user.id.
---
Nitpick comments:
In `@packages/backend/tests/integration/routes/lastfm.test.ts`:
- Around line 369-383: Extend the successful callback test to verify that the
response expires or clears the lastfm_state cookie, in addition to confirming
the redirect. Update the assertions in the test named “should link account on
valid callback” to inspect Set-Cookie headers and confirm the state cookie is
consumed.
- Around line 392-403: Replace the duplicate success case around the callback
test with a rejection test: retain a valid lastfm_state cookie but omit the
state query parameter, then assert the callback rejects the request and does not
invoke account linking or token exchange. Update the test name to reflect
missing query state, using the existing buildState, mockExchangeToken, and
mockSetLink symbols.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d2360c7-882d-4243-8d20-9286a2ac2b12
📒 Files selected for processing (5)
packages/backend/src/routes/lastfm.tspackages/backend/src/routes/spotify.tspackages/backend/tests/integration/routes/lastfm.test.tspackages/frontend/.size-limit.jsonpackages/frontend/BUNDLE_ANALYSIS.md
A client-supplied `state` query param on /connect could still encode a different discordId than the authenticated request's own req.user.id, since it was reused as-is whenever present. That let an authenticated session's Last.fm link get written under a different Discord account than the one it's actually signed in as. Authenticated requests now always get a freshly minted state for their own id; providedState is only used to resolve identity in the (unauthenticated) fallback path.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Contains OAuth state handling and security logic changes in lastfm routes that require human review; not a trivial refactor.
Re-trigger cubic
|
Merged
LucasSantana-Dev
added a commit
that referenced
this pull request
Jul 10, 2026
🤖 I have created a release *beep* *boop* --- <details><summary>2.34.0</summary> ## [2.34.0](v2.33.1...v2.34.0) (2026-07-10) ### Features * **twitch:** use Promise.allSettled for per-event subscription error logging ([#1749](#1749)) ([6691305](6691305)) ### Bug Fixes * [#1699](#1699) ([eef5aee](eef5aee)) * **backend:** migrate webhooks to use canonical timingsafekey comparison ([#1747](#1747)) ([eef5aee](eef5aee)) * **backend:** wrap lastfm routes with asynchandler ([#1726](#1726)) ([ce51d86](ce51d86)) * **batch-move:** graceful attachment-fetch degradation + mid-loop client re-check ([#1750](#1750)) ([f21a0ce](f21a0ce)) * **bot:** approve @discordjs/opus install script — P0 music playback outage ([#1757](#1757)) ([9d894e4](9d894e4)) * **ci:** add missing packages field to pnpm-workspace.yaml ([#1760](#1760)) ([a4c585d](a4c585d)) * **ci:** remove pnpm shim from bundle-size workflow ([#1759](#1759)) ([eaf676f](eaf676f)) * **deploy:** increase validation timeout to 10min ([#1743](#1743)) ([07891ec](07891ec)) * **docker:** copy+chown [@prisma](https://github.com/prisma) engines in production-backend — P0 deploy pipeline blocker ([#1758](#1758)) ([a70d0e8](a70d0e8)) * eliminate mock state pollution in bot tests and remove resetMocks config ([#1741](#1741)) ([2e5fd94](2e5fd94)) * **frontend:** prevent state updates after unmount ([#1748](#1748)) ([f4e7c45](f4e7c45)) * pin file-type to resolve CI flake [#1740](#1740) ([#1753](#1753)) ([6b8e527](6b8e527)) * reduce Jest maxWorkers and add DB pool config for test stability ([#1751](#1751)) ([cfead33](cfead33)) * use fake timers in ReminderService.spec to prevent race condition ([#1745](#1745)) ([ba2908c](ba2908c)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Merged
Merged
Merged
This was referenced Oct 1, 2026
Merged
Merged
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
2 participants
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.
Summary
Wraps all three async route handlers with the
asyncHandlermiddleware wrapper to ensure consistent error handling and prevent unhandled promise rejections:/api/lastfm/status— wrapped with asyncHandler/api/lastfm/unlink— wrapped with asyncHandler/api/lastfm/callback— wrapped with asyncHandlerThis aligns the lastfm routes with the existing pattern used in spotify.ts.
Test plan
npm run type:check --workspace=packages/backend)Closes #1624
Summary by cubic
Wrap Last.fm routes with
asyncHandler, add rate limiting, and harden OAuth: require both query and cookiestateto exist and match, and bindstateto the authenticated user. Aligns behavior withspotify.tsand closes #1624./api/lastfm/status,/api/lastfm/unlink, and/api/lastfm/callbackwithasyncHandler; appliedapiLimiterto all three.stateforreq.user.idand ignore any providedstate; only use decodedstatewhen unauthenticated.statematch; clear the state cookie; consistent error redirects; tests updated to send matching query and cookie.Written for commit 61712be. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation