Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .github/workflows/sonarcloud.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,17 @@ jobs:
run: npm run build:shared

- name: Run backend tests with coverage
run: npm run test:ci
run: npm run test --workspace=packages/backend -- --ci --coverage
continue-on-error: true

- name: Run bot tests with coverage
run: npm run test --workspace=packages/bot -- --ci --coverage
continue-on-error: true

- name: Run frontend tests with coverage (lcov)
run: npm run test --workspace=packages/frontend -- --coverage --coverage.reporter=lcov --coverage.reporter=text
continue-on-error: true
Comment on lines +46 to +48

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't swallow frontend test failures.

Line 48 makes this new test/coverage step non-blocking, so a broken frontend suite or bad coverage config can still leave the workflow green and Sonar running with incomplete data. Let the test step fail, and put if: ${{ always() }} on the Sonar step if you still want the scan to execute.

Suggested change
             - name: Run frontend tests with coverage (lcov)
               run: npm run test --workspace=packages/frontend -- --coverage --coverage.reporter=lcov --coverage.reporter=text
-              continue-on-error: true
 
             - name: SonarCloud Scan
+              if: ${{ always() }}
               uses: SonarSource/sonarqube-scan-action@v6
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/sonarcloud.yml around lines 46 - 48, The "Run frontend
tests with coverage (lcov)" step currently uses continue-on-error: true which
swallows frontend test failures; remove the continue-on-error: true setting from
that step so the workflow fails on test/coverage errors, and instead ensure the
Sonar scan step uses if: ${{ always() }} (add that condition to the
Sonar-related job/step) so the scan still runs even when tests fail.


- name: SonarCloud Scan
uses: SonarSource/sonarqube-scan-action@v6
env:
Expand Down
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Frontend theming now maps legacy `lucky-*` classes to the Lucky purple/gold palette
- Frontend typography now uses Lucky type tokens (`Sora`, `Manrope`, `JetBrains Mono`) instead of the old default stack
- Vercel build now generates Prisma client before shared/frontend builds to prevent missing generated client errors
- Autoplay no longer keeps cycling the same recommendations; queue top-up now uses anti-repeat filtering and keeps a 4-track buffer
- Shuffle now works reliably while autoplay is enabled because autoplay maintains enough upcoming tracks
- Web music repeat mode now supports `autoplay` end-to-end (bot mapper, backend validation, shared/frontend types)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Move the autoplay repeat mode note out of Fixed.

Line 43 documents a new end-to-end capability, not a bug fix. Keeping it under ### Fixed will make the release notes misleading; ### Added or ### Changed fits better.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CHANGELOG.md` at line 43, This line "Web music repeat mode now supports
`autoplay` end-to-end (bot mapper, backend validation, shared/frontend types)"
is a new feature note misplaced under the `### Fixed` section; move that bullet
out of the `Fixed` section and place it under `### Added` (or `### Changed`) in
CHANGELOG.md so the release notes correctly reflect it as a new capability
rather than a bug fix.

- OAuth callback now reuses the same redirect URI across auth start/callback token exchange, with forwarded-host fallback for proxied HTTPS deployments
- E2E stability improvements: dashboard/servers/track-history tests now use deterministic locators and route-delay handling
Comment on lines +41 to +45

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add the PR reference to these release notes.

These new unreleased bullets should point back to #129 so the shipped behavior can be traced after release.

As per coding guidelines, "Update CHANGELOG.md with all changes, include breaking changes documentation, and reference issues and PRs".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CHANGELOG.md` around lines 41 - 45, Update the unreleased bullets in
CHANGELOG.md by appending the PR reference " (`#129`)" to each of the listed items
(e.g., after the lines starting "Autoplay no longer keeps cycling the same
recommendations...", "Shuffle now works reliably...", "Web music repeat mode now
supports `autoplay`...", "OAuth callback now reuses the same redirect URI...",
and "E2E stability improvements...") so every bullet points back to PR `#129` for
traceability.


### Changed

Expand Down
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ packages/
### Bot
- Multi-platform music (YouTube, Spotify) with queue, shuffle, repeat, lyrics, autoplay
- Dynamic Discord presence rotation with live guild/member/session stats and command CTA
- Autoplay recommendations use anti-repeat filtering with queue buffering so shuffle stays useful during autoplay
- Now-playing card updates in place to avoid channel spam on track changes
- Video/audio downloads with format selection and progress tracking
- Moderation: warn, mute, kick, ban with case tracking
Expand Down Expand Up @@ -135,6 +136,8 @@ Without `VITE_API_BASE_URL`, frontend now auto-targets `lucky-api.lucassantana.t
When `WEBAPP_FRONTEND_URL` includes multiple origins, use comma-separated values
(example: `https://lucky.lucassantana.tech,https://lukbot.vercel.app`); backend CORS
accepts all configured entries while OAuth/Last.fm redirects use the first origin.
Set `WEBAPP_REDIRECT_URI` to the exact Discord OAuth callback URL registered in the
Discord Developer Portal (example: `https://lucky.lucassantana.tech/api/auth/callback`).

## Environment Variables

Expand All @@ -148,6 +151,7 @@ See `.env.example` for all available options. Key variables:
| `REDIS_HOST` | No | Redis host (default: localhost) |
| `WEBAPP_ENABLED` | No | Enable web dashboard (default: false) |
| `WEBAPP_SESSION_SECRET` | No | Session encryption key |
| `WEBAPP_REDIRECT_URI` | No | Explicit Discord OAuth callback URL (must match Discord app settings) |
| `CLIENT_SECRET` | No | Discord OAuth secret (for dashboard) |
| `SENTRY_DSN` | No | Error tracking |

Expand Down
125 changes: 106 additions & 19 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions packages/backend/src/routes/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { AppError } from '../errors/AppError'
import { authLimiter } from '../middleware/rateLimit'
import { handleOAuthCallback } from './authCallback'
import { getPrimaryFrontendUrl } from '../utils/frontendOrigin'
import { getOAuthRedirectUri } from '../utils/oauthRedirectUri'

const getFrontendUrl = (): string => {
return getPrimaryFrontendUrl()
Expand All @@ -19,6 +20,7 @@ export function setupAuthRoutes(app: Express): void {
async (req: Request, res: Response) => {
try {
req.session.oauthInitiated = true
req.session.oauthRedirectUri = getOAuthRedirectUri(req)

await new Promise<void>((resolve) => {
req.session.save((err) => {
Expand All @@ -38,9 +40,7 @@ export function setupAuthRoutes(app: Express): void {
})

const clientId = process.env.CLIENT_ID
const redirectUri =
process.env.WEBAPP_REDIRECT_URI ??
`http://localhost:${process.env.WEBAPP_PORT ?? '3000'}/api/auth/callback`
const redirectUri = req.session.oauthRedirectUri
const scope = 'identify guilds'

if (!clientId) {
Expand Down
10 changes: 9 additions & 1 deletion packages/backend/src/routes/authCallback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { debugLog, errorLog } from '@lucky/shared/utils'
import { discordOAuthService } from '../services/DiscordOAuthService'
import { sessionService } from '../services/SessionService'
import { getPrimaryFrontendUrl } from '../utils/frontendOrigin'
import { getOAuthRedirectUri } from '../utils/oauthRedirectUri'

export async function handleOAuthCallback(
req: Request,
Expand Down Expand Up @@ -30,7 +31,14 @@ export async function handleOAuthCallback(
data: { code: code.substring(0, 10) },
})

const tokenData = await discordOAuthService.exchangeCodeForToken(code)
const redirectUri = getOAuthRedirectUri(
req,
req.session.oauthRedirectUri,
)
const tokenData = await discordOAuthService.exchangeCodeForToken(
code,
redirectUri,
)
const userInfo = await discordOAuthService.getUserInfo(
tokenData.access_token,
)
Expand Down
6 changes: 4 additions & 2 deletions packages/backend/src/routes/music/playbackRoutes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,10 @@ export function setupPlaybackRoutes(app: Express): void {
asyncHandler(async (req: AuthenticatedRequest, res: Response) => {
const guildId = param(req.params.guildId)
const { mode } = req.body
if (!['off', 'track', 'queue'].includes(mode)) {
throw AppError.badRequest('Mode must be off, track, or queue')
if (!['off', 'track', 'queue', 'autoplay'].includes(mode)) {
throw AppError.badRequest(
'Mode must be off, track, queue, or autoplay',
)
Comment on lines 114 to +118

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Move repeat-mode validation into the backend schema layer.

This works, but it keeps HTTP input validation duplicated inside the route and creates another string-literal source of truth for repeat modes. Please push this through a backend/src/schemas/ Zod body schema and validateBody(...) instead of extending the inline includes(...) check here.

Suggested direction
-            const { mode } = req.body
-            if (!['off', 'track', 'queue', 'autoplay'].includes(mode)) {
-                throw AppError.badRequest(
-                    'Mode must be off, track, queue, or autoplay',
-                )
-            }
+            const { mode } = repeatModeBodySchema.parse(req.body)
export const repeatModeBodySchema = z.object({
    mode: z.enum(['off', 'track', 'queue', 'autoplay']),
})

As per coding guidelines "packages/backend/src/**/*.ts: Validation must use Zod schemas in backend/src/schemas/ and be applied via validateBody, validateParams, or validateQuery".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/backend/src/routes/music/playbackRoutes.ts` around lines 114 - 118,
Replace the inline includes(...) validation in the playback route with a Zod
body schema and validateBody: add and export repeatModeBodySchema = z.object({
mode: z.enum(['off','track','queue','autoplay']) }) in backend/src/schemas,
import that schema into the playback route and call
validateBody(repeatModeBodySchema, req) at the start of the handler, then use
the validated mode instead of reading req.body and remove the
AppError.badRequest(...) branch; ensure imports and types are updated so the
handler receives the validated payload.

}
res.json(
await musicControlService.sendCommand(
Expand Down
7 changes: 5 additions & 2 deletions packages/backend/src/services/DiscordOAuthService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ class DiscordOAuthService {
)
}

async exchangeCodeForToken(code: string): Promise<TokenResponse> {
async exchangeCodeForToken(
code: string,
redirectUri?: string,
): Promise<TokenResponse> {
try {
const response = await fetch(`${this.apiBaseUrl}/oauth2/token`, {
method: 'POST',
Expand All @@ -64,7 +67,7 @@ class DiscordOAuthService {
client_secret: this.getClientSecret(),
grant_type: 'authorization_code',
code,
redirect_uri: this.getRedirectUri(),
redirect_uri: redirectUri ?? this.getRedirectUri(),
}),
})

Expand Down
1 change: 1 addition & 0 deletions packages/backend/src/types/session.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@ declare module 'express-session' {
authenticated?: boolean
userId?: string
oauthInitiated?: boolean
oauthRedirectUri?: string
}
}
34 changes: 34 additions & 0 deletions packages/backend/src/utils/oauthRedirectUri.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import type { Request } from 'express'

const getForwardedHeader = (
req: Request,
headerName: string,
): string | undefined => {
const value = req.headers[headerName]
if (!value) return undefined
const raw = Array.isArray(value) ? value[0] : value
return raw.split(',')[0].trim() || undefined
}

const buildRequestRedirectUri = (req: Request): string => {
const forwardedProto = getForwardedHeader(req, 'x-forwarded-proto')
const forwardedHost = getForwardedHeader(req, 'x-forwarded-host')
const protocol = forwardedProto ?? req.protocol ?? 'http'
const host =
forwardedHost ??
req.get('host') ??
`localhost:${process.env.WEBAPP_PORT ?? '3000'}`

return `${protocol}://${host}/api/auth/callback`
}
Comment on lines +13 to +23

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if trust proxy is configured anywhere in the backend
rg -n "trust.?proxy" packages/backend/src/

Repository: LucasSantana-Dev/Lucky

Length of output: 48


🏁 Script executed:

# First, let's read the actual file
cat -n packages/backend/src/utils/oauthRedirectUri.ts

Repository: LucasSantana-Dev/Lucky

Length of output: 1302


🏁 Script executed:

# Check where and how getOAuthRedirectUri is used
rg -n "getOAuthRedirectUri\|buildRequestRedirectUri" packages/backend/src/ -B 2 -A 2

Repository: LucasSantana-Dev/Lucky

Length of output: 48


🏁 Script executed:

# Look at server.ts to see if express app configuration includes trust proxy
cat -n packages/backend/src/server.ts | head -50

Repository: LucasSantana-Dev/Lucky

Length of output: 2101


🏁 Script executed:

# Check how WEBAPP_FRONTEND_URL is used elsewhere
rg -n "WEBAPP_FRONTEND_URL" packages/ -B 2 -A 2

Repository: LucasSantana-Dev/Lucky

Length of output: 1924


🏁 Script executed:

# Search for auth routes that might use this utility
fd "auth" packages/backend/src/routes/ --type f

Repository: LucasSantana-Dev/Lucky

Length of output: 145


🏁 Script executed:

# Read the frontendOrigin.ts file to see the existing validation pattern
cat -n packages/backend/src/utils/frontendOrigin.ts

Repository: LucasSantana-Dev/Lucky

Length of output: 646


🏁 Script executed:

# Search for any other forwarded header usage in the backend
rg -n "x-forwarded|req\.headers" packages/backend/src/ -t ts

Repository: LucasSantana-Dev/Lucky

Length of output: 484


🏁 Script executed:

# Check the auth routes to see if and how getOAuthRedirectUri is used
cat -n packages/backend/src/routes/auth.ts | head -120

Repository: LucasSantana-Dev/Lucky

Length of output: 5135


🏁 Script executed:

# Also check authCallback.ts
cat -n packages/backend/src/routes/authCallback.ts | head -80

Repository: LucasSantana-Dev/Lucky

Length of output: 3195


🏁 Script executed:

# Search specifically for getOAuthRedirectUri import and usage
rg -n "getOAuthRedirectUri" packages/backend/src/

Repository: LucasSantana-Dev/Lucky

Length of output: 572


Add host validation to prevent header injection attacks.

The utility reads x-forwarded-host without validation, and Express doesn't have trust proxy configured. While Discord validates the redirect URI against registered callbacks (mitigating the direct risk), an attacker who can inject headers could still cause unexpected behavior. The codebase already has a validation pattern in frontendOrigin.ts—apply the same approach here to validate the derived host against configured allowed origins.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/backend/src/utils/oauthRedirectUri.ts` around lines 13 - 23,
buildRequestRedirectUri currently trusts x-forwarded-host and req.get('host')
without validation; replicate the allowed-origin validation used in
frontendOrigin.ts to avoid header injection. Update buildRequestRedirectUri to
derive the host (from getForwardedHeader or req.get('host')), then validate that
host against the configured allowed origins list (same source/logic as
frontendOrigin.ts) and reject/replace unrecognized hosts (fallback to localhost
with WEBAPP_PORT) before composing the final
`${protocol}://${host}/api/auth/callback`; ensure getForwardedHeader usage
remains but its result is validated and sanitized, and surface a safe default
when validation fails.


export function getOAuthRedirectUri(
req: Request,
sessionRedirectUri?: string,
): string {
return (
sessionRedirectUri ??
process.env.WEBAPP_REDIRECT_URI ??
buildRequestRedirectUri(req)
)
}
1 change: 1 addition & 0 deletions packages/backend/tests/integration/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ describe('API Integration Flows', () => {
)
expect(mockDiscordOAuth.exchangeCodeForToken).toHaveBeenCalledWith(
MOCK_AUTH_CODE,
expect.stringContaining('/api/auth/callback'),
)
expect(mockDiscordOAuth.getUserInfo).toHaveBeenCalledWith(
MOCK_TOKEN_RESPONSE.access_token,
Expand Down
Loading