feat: guild admin authentication & authorization#73
Conversation
Closes #72 - Add bot owner bypass to permission checks (BOT_OWNER_ID) - Add isGuildAdmin and isModerator permission utilities - Add permission checks to /config and /modlog slash commands - Add Discord OAuth2 authentication routes (/api/v1/auth/*) - Add JWT middleware for API session auth - Support dual auth (API secret + OAuth2 JWT) in middleware - Add guild admin verification for REST API endpoints - Add GET /guilds list endpoint with OAuth2 filtering - Add comprehensive tests for all new functionality Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Discord OAuth2 endpoints, server-side TTL sessions, JWT verification, per-user guild caching, dual auth (x-api-secret or Bearer JWT), bot-owner/moderator permission checks and config fields, a runtime jsonwebtoken dependency, test coverage, and shutdown cleanup wiring. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/routes/guilds.js (1)
243-276:⚠️ Potential issue | 🟠 MajorAdd
requireGuildAdminto stats, members, and moderation endpoints.The
/stats,/members, and/moderationendpoints lack therequireGuildAdminmiddleware while config and actions routes enforce it. This allows any OAuth-authenticated user to query member lists (with role hierarchies), moderation case histories (with reasons and moderator details), and guild stats for any guild the bot is in — without admin rights in those guilds. AddrequireGuildAdminto these three routes to restrict access to guild admins.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/routes/guilds.js` around lines 243 - 276, Add the requireGuildAdmin middleware to the guild admin-only routes: include requireGuildAdmin in the route handler chain for router.get('/:id/stats'), router.get('/:id/members') and router.get('/:id/moderation') so the middleware runs before the async handler; if requireGuildAdmin is not already imported in this file, import it and apply it as the second parameter (e.g., router.get('/:id/stats', requireGuildAdmin, async (req, res) => { ... })), ensuring the same change is made for the members and moderation route handlers to restrict access to guild admins only.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
config.jsonpackage.jsonsrc/api/index.jssrc/api/middleware/auth.jssrc/api/middleware/oauth.jssrc/api/routes/auth.jssrc/api/routes/guilds.jssrc/api/server.jssrc/commands/config.jssrc/commands/modlog.jssrc/utils/permissions.jstests/api/middleware/auth.test.jstests/api/middleware/oauth.test.jstests/api/routes/auth.test.jstests/api/routes/guilds.test.jstests/commands/config.test.jstests/commands/modlog.test.jstests/utils/permissions.test.js
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,mjs}: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix when importing Node.js built-in modules (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals; double quotes are not permitted
Use 2-space indentation for all code
Files:
src/api/server.jstests/api/routes/guilds.test.jstests/api/middleware/oauth.test.jssrc/api/routes/auth.jstests/api/middleware/auth.test.jssrc/commands/config.jstests/api/routes/auth.test.jssrc/utils/permissions.jstests/utils/permissions.test.jstests/commands/modlog.test.jssrc/api/middleware/oauth.jssrc/commands/modlog.jssrc/api/routes/guilds.jstests/commands/config.test.jssrc/api/middleware/auth.jssrc/api/index.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER useconsole.log(),console.warn(),console.error(), or anyconsole.*method in src/ files; always use Winston logger instead viaimport { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig()fromsrc/modules/config.jsto read runtime configuration
UsesetConfigValue(key, value)fromsrc/modules/config.jsto update configuration at runtime
Always usesafeSend()wrapper fromsrc/utils/safeSend.jsfor sending messages to enforce allowedMentions and prevent mention exploits
UsesplitMessage()utility fromsrc/utils/splitMessage.jsto split messages exceeding Discord's 2000-character limit
Files:
src/api/server.jssrc/api/routes/auth.jssrc/commands/config.jssrc/utils/permissions.jssrc/api/middleware/oauth.jssrc/commands/modlog.jssrc/api/routes/guilds.jssrc/api/middleware/auth.jssrc/api/index.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export adataobject usingSlashCommandBuilderand anexecute(interaction)async function
ExportadminOnly = truefrom command files to restrict execution to moderators
Files:
src/commands/config.jssrc/commands/modlog.js
🧠 Learnings (9)
📚 Learning: 2025-10-10T15:05:26.145Z
Learnt from: CR
Repo: BillChirico/LUA-Obfuscator PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-10T15:05:26.145Z
Learning: Applies to package.json : Only add new packages when absolutely necessary or explicitly requested
Applied to files:
package.json
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Use Vitest for unit and integration tests; run `pnpm test` and `pnpm test:coverage` before every commit
Applied to files:
tests/api/routes/guilds.test.jstests/api/middleware/oauth.test.jstests/api/middleware/auth.test.jstests/api/routes/auth.test.jstests/commands/modlog.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Test config structure, command exports, and utility functions; avoid testing live Discord connections (smoke/unit tests only)
Applied to files:
tests/api/routes/guilds.test.jssrc/commands/config.jstests/api/routes/auth.test.jstests/utils/permissions.test.jstests/commands/modlog.test.jssrc/commands/modlog.jstests/commands/config.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*.js : Export `adminOnly = true` from command files to restrict execution to moderators
Applied to files:
src/commands/config.jstests/utils/permissions.test.jstests/commands/modlog.test.jssrc/commands/modlog.jstests/commands/config.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Always call `checkHierarchy(moderator, target)` from `src/utils/permissions.js` before executing moderation actions to prevent role hierarchy violations
Applied to files:
src/commands/config.jssrc/utils/permissions.jstests/utils/permissions.test.jstests/commands/modlog.test.jssrc/commands/modlog.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*.js : Slash commands must export a `data` object using `SlashCommandBuilder` and an `execute(interaction)` async function
Applied to files:
src/commands/config.jssrc/commands/modlog.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Moderation commands must follow the pattern: deferReply → validate inputs → sendDmNotification → execute Discord action → createCase → sendModLogEmbed → checkEscalation
Applied to files:
src/commands/config.jstests/commands/modlog.test.jssrc/commands/modlog.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/index.js : Enable Discord intents for MessageContent, GuildMembers, and GuildVoiceStates in client initialization
Applied to files:
src/commands/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick}*.js : DM the target user *before* executing kicks/bans in moderation commands; once a user is kicked/banned they cannot receive DMs
Applied to files:
src/commands/modlog.js
🧬 Code graph analysis (12)
tests/api/routes/guilds.test.js (2)
src/api/routes/auth.js (2)
token(102-116)token(139-139)web/src/lib/auth.ts (1)
jwt(132-166)
tests/api/middleware/oauth.test.js (2)
tests/api/middleware/auth.test.js (3)
req(39-39)res(40-40)next(41-41)src/api/middleware/oauth.js (1)
requireOAuth(14-37)
src/api/routes/auth.js (2)
src/logger.js (2)
error(231-233)info(217-219)src/api/server.js (1)
dashboardUrl(35-35)
tests/api/middleware/auth.test.js (1)
src/api/middleware/auth.js (1)
requireAuth(36-82)
src/commands/config.js (2)
src/modules/config.js (1)
getConfig(133-135)src/utils/permissions.js (2)
isGuildAdmin(95-112)getPermissionError(146-148)
tests/api/routes/auth.test.js (3)
src/api/server.js (3)
app(25-25)app(93-93)createApp(24-78)src/api/routes/auth.js (2)
token(102-116)token(139-139)web/src/lib/auth.ts (1)
jwt(132-166)
src/utils/permissions.js (4)
src/api/routes/guilds.js (1)
config(178-178)src/commands/config.js (3)
config(125-125)config(161-161)config(198-198)src/commands/modlog.js (2)
config(35-35)config(178-178)src/index.js (1)
config(57-57)
src/api/middleware/oauth.js (1)
src/api/routes/auth.js (7)
req(40-40)authHeader(133-133)token(102-116)token(139-139)sessionSecret(49-49)sessionSecret(140-140)decoded(147-147)
src/commands/modlog.js (2)
src/utils/permissions.js (2)
isModerator(121-138)getPermissionError(146-148)src/utils/safeSend.js (1)
safeReply(138-145)
tests/commands/config.test.js (1)
src/modules/config.js (1)
getConfig(133-135)
src/api/middleware/auth.js (3)
src/api/routes/auth.js (7)
req(40-40)authHeader(133-133)token(102-116)token(139-139)sessionSecret(49-49)sessionSecret(140-140)decoded(147-147)src/logger.js (1)
warn(224-226)web/src/lib/auth.ts (1)
jwt(132-166)
src/api/index.js (3)
src/api/routes/auth.js (1)
router(10-10)src/api/routes/guilds.js (1)
router(11-11)src/api/routes/health.js (1)
router(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (19)
src/utils/permissions.js (1)
121-138:isModeratorimplementation looks correct.Distinct from
isGuildAdminby checkingManageGuildinstead ofAdministrator. The bot-owner bypass and adminRoleId fallback are consistent. Null guard onmemberand optional chaining onconfigare appropriate.package.json (1)
23-23: Addition ofjsonwebtokenis justified for OAuth2 JWT authentication.The package is actively used for creating and verifying JWT tokens in the OAuth2 flow:
jwt.sign()creates tokens in the Discord callback route (with 7-day expiration), andjwt.verify()validates Bearer tokens in auth middleware. Version 9.0.3 is the latest stable release.src/api/index.js (1)
17-21: LGTM!Auth routes are correctly mounted as public (needed for OAuth login flow), while guild routes remain protected by
requireAuth().src/api/server.js (1)
40-40: LGTM!Adding
Authorizationto the allowed CORS headers is necessary for the new Bearer token authentication to work with cross-origin dashboard requests.src/api/routes/guilds.js (1)
110-145: LGTM — guild listing correctly differentiates by auth method.OAuth users see only guilds where they have ADMINISTRATOR and the bot is present. API-secret users see all bot guilds. The filter-then-map pattern ensures
botGuilds.get()won't returnundefinedon line 124 sincebotGuilds.has()is checked first on line 121.src/commands/config.js (1)
160-167: LGTM — admin gate correctly placed before subcommand dispatch.The permission check runs early, uses the cached config for role-based admin lookup, and returns an ephemeral error on denial. Combined with the
adminOnly = trueexport on line 68, this provides defense-in-depth. Based on learnings, theadminOnlyflag export anddata/executeexports are both present as required.tests/api/middleware/oauth.test.js (1)
1-92: LGTM — solid test coverage for the OAuth middleware.Tests cover all branches: missing header, non-Bearer scheme, unconfigured session secret, invalid token, valid token with decoded payload assertion, and expired token. Using real JWT signing (not mocking
jsonwebtoken) gives higher confidence.tests/commands/config.test.js (1)
128-138: DualgetConfigmock pattern is well-documented.The comments on lines 137 clearly explain the two-call sequence (permission check →
handleView). This makes the mock setup easy to understand for future maintainers.tests/api/routes/guilds.test.js (3)
110-127: LGTM — good integration test for JWT authentication path.Correctly verifies that a Bearer token with ADMINISTRATOR permission (
'8') on the target guild results in a 200 response with the expected guild payload. The string-typepermissionsvalue matches Discord's API response format.
139-188: LGTM — thorough coverage of the guild listing filter logic.Tests cover all three branches: api-secret (all guilds), OAuth admin with mixed bot presence (filtered correctly), and OAuth non-admin (empty list). Good edge case with
guild-not-in-botproving the bot-presence filter works.
204-265: LGTM — solid admin verification test coverage.All four scenarios are covered: api-secret passthrough, OAuth admin allowed, OAuth non-admin denied with 403, and OAuth user not in guild denied with 403.
tests/api/middleware/auth.test.js (3)
57-67: API-secret path is exercised correctly.
92-101: Auth method assertion strengthens coverage.
104-153: JWT/OAuth fallback and error cases are well covered.tests/api/routes/auth.test.js (5)
1-31: Test harness setup looks solid.
33-65: OAuth redirect and not-configured cases are covered.
67-83: Callback error handling coverage is good.
85-136: /auth/me JWT success and error paths are well tested.
138-145: Logout route test looks good.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config.json`:
- Line 85: The BOT_OWNER_ID hardcoded constant in src/utils/permissions.js is
unused relative to config.permissions.botOwners — remove the hardcoded
BOT_OWNER_ID and update the permissions logic to read bot owner IDs from
config.permissions.botOwners (accepting an array or single string), e.g.,
normalize to an array and use that in owner checks (replace direct comparisons
like BOT_OWNER_ID === userId with an array.includes(userId) check), and ensure
any functions/methods that relied on BOT_OWNER_ID (permissions check functions)
fall back safely if the config value is missing.
In `@src/api/middleware/auth.js`:
- Around line 41-43: The warning for missing BOT_API_SECRET currently calls warn
without structured metadata; update that warn invocation to include the request
metadata (e.g., { ip: req.ip, path: req.path or req.originalUrl }) so it matches
other warns on lines 74/79; locate the check around process.env.BOT_API_SECRET
and replace the bare warn(...) with a version that passes the same { ip, path }
object used elsewhere in this middleware before returning the 401 response.
- Around line 36-81: The final duplicate unauthorized branches in requireAuth()
are unreachable/redundant; collapse them into a single fallback that logs the
warning once and returns the 401 JSON. Update the code in requireAuth to remove
the separate if (!apiSecret && !authHeader) block and the subsequent identical
warn/return, replacing both with one warn('Unauthorized API request', { ip:
req.ip, path: req.path }) followed by return res.status(401).json({ error:
'Unauthorized' }) so that both "no credentials" and "bad credentials" fall
through to the same single handler.
In `@src/api/middleware/oauth.js`:
- Around line 14-36: Extract the duplicated JWT extraction and verification from
the GET /me handler and reuse the existing requireOAuth middleware: remove the
Bearer token parsing, SESSION_SECRET check, and jwt.verify logic in the /me
route handler and instead apply requireOAuth(req,res,next) to that route so the
route reads the authenticated user from req.user; ensure requireOAuth sets
req.user to the decoded token (already present) and update the /me handler to
rely on req.user for response logic.
- Around line 25-27: The middleware currently returns a 500 when sessionSecret
is missing without logging; update the OAuth middleware that checks
sessionSecret (variable sessionSecret) to log a contextual error via the Winston
logger before sending the response—e.g., use the module's logger instance
(import or require the project's Winston logger) to log an error like "Session
secret not configured (SESSION_SECRET missing)" including any request context
(route/method) and then return res.status(500).json({ error: 'Session not
configured' }); so the error is recorded per src/ coding guidelines.
- Around line 29-35: The jwt.verify call in the middleware (where token and
sessionSecret are used and req.user is set before calling next) does not specify
allowed algorithms, leaving it open to algorithm confusion attacks; update the
verify invocation to pass an explicit algorithms array matching your
issuer/signing method (e.g., the HS or RS algorithm you actually use) so only
that algorithm is accepted, and ensure the token validation still throws so the
existing catch returns the 401 via res.status when verification fails.
In `@src/api/routes/auth.js`:
- Around line 101-116: The JWT currently embeds the full guild list which causes
staleness and large payloads; update the token creation in auth.js (the jwt.sign
call that assigns token, using sessionSecret and expiresIn '7d') to include only
minimal, stable claims (e.g., userId and maybe username) and remove the guilds
array from the payload, and then implement one of two options: (A) change
expiresIn to a short lifetime (e.g., '1h') and add a refresh token flow to mint
new JWTs, or (B) keep longer JWTs but always fetch/cached guild membership and
permissions at request time (or via a short-TTL cache) instead of reading them
from the token; ensure any authorization checks read guilds from that runtime
source rather than from the JWT.
- Around line 120-122: Replace the current query-parameter redirect that exposes
the JWT (res.redirect(`${dashboardUrl}?token=${token}`)) with a safer delivery:
set the JWT in an HttpOnly, Secure, SameSite=Strict cookie (use
res.cookie('token', token, { httpOnly: true, secure: true, sameSite: 'Strict',
maxAge: ... })) and then call res.redirect(dashboardUrl) so the token is not in
the URL; alternatively implement a short-lived single-use authorization code
flow (issue code server-side, redirect with code, then exchange via POST) or, if
a quicker mitigation is needed, switch to a fragment-based redirect
(res.redirect(`${dashboardUrl}#token=${token}`)) while noting fragments are
client-only.
- Around line 55-97: The three Discord API fetches in auth.js (the token
exchange fetch that yields tokenResponse/tokenData/accessToken, the user info
fetch that yields userResponse/user, and the guilds fetch that yields
guildsResponse) lack timeouts and can hang; update each fetch call to use an
AbortController or AbortSignal.timeout with a sensible timeout (e.g., 10s), pass
the resulting signal in the fetch options, and ensure you abort/cleanup the
controller on completion or error so requests are reliably cancelled if Discord
is slow.
In `@src/api/routes/guilds.js`:
- Around line 54-67: isOAuthGuildAdmin currently trusts JWT-embedded guild
permissions (using ADMINISTRATOR_FLAG) which can be stale for up to 7 days;
update the auth flow and permission checks by either (A) reducing the JWT
expiresIn in auth.js to shorten the window of stale claims, or (B) changing
sensitive write endpoints to revalidate admin status on-demand via the Discord
API instead of relying on isOAuthGuildAdmin, or (C) implement a token
revocation/blacklist that invalidates JWTs when a user's guild roles change;
locate uses of isOAuthGuildAdmin in src/api/routes/guilds.js and replace or
augment them with an async Discord permission check (or gate them behind a
helper that consults the revocation list), and update the JWT creation in
auth.js where expiresIn is set to apply option (A) if chosen.
In `@src/commands/modlog.js`:
- Around line 35-41: The permission error message for /modlog is misleading:
update the permission helper so it can produce a moderator-specific message and
use it from the modlog check. Either add a new function
getModeratorPermissionError() in the permissions utility (or extend
getPermissionError(requiredLevel)) to return a message referencing
moderator/ManageGuild access, then replace the call to
getPermissionError('modlog') in src/commands/modlog.js with the new
getModeratorPermissionError() (or
getPermissionError('moderator'/'ManageGuild')), keeping the existing
isModerator(interaction.member, config) check unchanged.
In `@src/utils/permissions.js`:
- Around line 95-112: The two functions isGuildAdmin and isAdmin duplicate
logic; consolidate by making one the single source of truth—e.g., relax the
strict config null-check in isAdmin so it safely handles optional config and
then have isGuildAdmin simply call and return isAdmin(member, config). Ensure
isAdmin still performs isBotOwner(member), checks
member.permissions.has(PermissionFlagsBits.Administrator), and if
config?.permissions?.adminRoleId is present uses
member.roles.cache.has(config.permissions.adminRoleId); remove the duplicated
logic from isGuildAdmin so all checks live in isAdmin.
- Around line 9-10: BOT_OWNER_ID is hardcoded; switch to reading the owners list
from config.permissions.botOwners and remove the constant. Update the module to
export a helper that accepts the config (or export a function isBotOwner(config,
userId)) and use config.permissions.botOwners (supporting multiple IDs) inside
isBotOwner instead of BOT_OWNER_ID; then update all callers of isBotOwner to
pass the app config through. Ensure the exported symbol BOT_OWNER_ID is
removed/deprecated and references are replaced with the new config-driven
isBotOwner usage.
In `@tests/commands/config.test.js`:
- Around line 15-18: Add a new test that simulates permission denial by mocking
isGuildAdmin to return false and asserting the command returns the ephemeral
permission error from getPermissionError; specifically, in
tests/commands/config.test.js override vi.mock for isGuildAdmin to
vi.fn().mockReturnValue(false), call the code path that executes the /config
command (the same test helper used elsewhere in this file), and assert the reply
is ephemeral and equals getPermissionError() so the permission guard for the
config command is exercised end-to-end.
In `@tests/commands/modlog.test.js`:
- Around line 32-35: Add a test covering the permission-denial path by mocking
isModerator to return false, invoking the modlog command's execute function with
a created interaction, and asserting the interaction.reply was called with an
ephemeral error containing the permission message; specifically, in the test
file use isModerator.mockReturnValueOnce(false), call createInteraction('view')
(or same helper used elsewhere) then await execute(interaction) and
expect(interaction.reply).toHaveBeenCalledWith(expect.objectContaining({
content: expect.stringContaining("don't have permission"), ephemeral: true }));
reference isModerator, execute and createInteraction to locate where to add this
case.
In `@tests/utils/permissions.test.js`:
- Around line 244-284: Add a test that passes a null config into isModerator to
cover the nullable-config code path: create a member (e.g., with permissions.has
mocked to false and roles.cache.has mocked to false) and assert
isModerator(member, null) returns false (and add a separate case if needed where
member is bot owner or has ManageGuild permission to assert true when config is
null). Reference the isModerator function and the existing test patterns
(permissions.has and roles.cache.has mocks) to add the new null-config
assertions so the implementation's null-config handling is exercised.
- Around line 207-242: Add a test that verifies isGuildAdmin handles a null
config without throwing: create a non-admin member stub (permissions.has ->
false, roles.cache.has -> false) and call isGuildAdmin(member, null) asserting
it returns false; place this new case inside the existing
describe('isGuildAdmin') block so it covers the optional chaining path in
isGuildAdmin.
---
Outside diff comments:
In `@src/api/routes/guilds.js`:
- Around line 243-276: Add the requireGuildAdmin middleware to the guild
admin-only routes: include requireGuildAdmin in the route handler chain for
router.get('/:id/stats'), router.get('/:id/members') and
router.get('/:id/moderation') so the middleware runs before the async handler;
if requireGuildAdmin is not already imported in this file, import it and apply
it as the second parameter (e.g., router.get('/:id/stats', requireGuildAdmin,
async (req, res) => { ... })), ensuring the same change is made for the members
and moderation route handlers to restrict access to guild admins only.
---
Duplicate comments:
In `@src/api/routes/auth.js`:
- Around line 146-147: The jwt.verify call in the authentication flow
(jwt.verify(token, sessionSecret)) must include an explicit algorithms option to
prevent algorithm confusion; update the verify call used in the auth route to
pass a third parameter like { algorithms: ['HS256'] } (or the exact algorithm
your tokens use) so jwt.verify(token, sessionSecret, { algorithms: [...] })
explicitly restricts allowed signing algorithms and uses the same algorithm
setting as in oauth.js.
- Add CSRF state parameter to OAuth2 flow with expiry map (Thread 1)
- Switch JWT redirect from query param to fragment-based (#token=) (Thread 3+14)
- Add AbortSignal.timeout(10_000) to all Discord API fetch calls (Thread 12)
- Store Discord accessToken in JWT, remove guilds, shorten expiry to 1h (Thread 13+15)
- Use requireOAuth middleware on /me route instead of inline JWT verification (Thread 4+9)
- Add error logging when SESSION_SECRET is missing in oauth.js (Thread 10)
- Add { algorithms: ['HS256'] } to jwt.verify in auth.js and oauth.js (Thread 11)
- Collapse duplicate unauthorized branches in requireAuth (Thread 7)
- Add structured metadata to BOT_API_SECRET warning log (Thread 8)
- Replace hardcoded BOT_OWNER_ID with config.permissions.botOwners array (Thread 2+6+17)
- Delegate isGuildAdmin to isAdmin to remove duplication (Thread 5+18)
- Parameterize getPermissionError with level parameter (Thread 16)
- Fetch fresh guilds from Discord in guilds.js routes (Thread 13+15)
- Add permission denial tests for config and modlog commands (Thread 19+20)
- Add tests for isGuildAdmin/isModerator with null config (Thread 21+22)
- Update existing API tests for new OAuth flow (accessToken in JWT, fetch mocks)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The fix commit (38feff0) addressed many of the earlier review findings well — CSRF state parameter, algorithm pinning, fragment-based token redirect, fresh guild fetching, isGuildAdmin consolidation, and getPermissionError level parameter are all solid improvements.
Remaining issues:
🔴 Critical (1)
- Discord access token stored in plaintext JWT (
src/api/routes/auth.js:126-136): The Discord OAuth2 access token is embedded directly in the JWT payload. JWTs are signed but not encrypted — anyone who obtains the token can base64-decode it and extract the raw Discord access token. Store the access token server-side instead.
🟡 Warning (3)
- Missing
requireGuildAdminon stats/members/moderation routes (src/api/routes/guilds.js): Config and actions endpoints correctly have the middleware, but/:id/stats,/:id/members, and/:id/moderationdo not. Any OAuth user can access sensitive data for any guild. oauthStatesmap grows unboundedly (src/api/routes/auth.js:16-28): Cleanup only runs on new OAuth redirects. Add interval-based cleanup or bound the map size.fetchUserGuildscalled on every admin-gated request (src/api/routes/guilds.js:79-89): Each request to a protected endpoint triggers a Discord API round-trip. Add a short-lived cache to avoid rate limiting.
🔵 Nitpick (1)
- Missing env vars in
.env.example:SESSION_SECRETandDISCORD_REDIRECT_URIare used but not documented per AGENTS.md guidelines.
JWTs are signed but not encrypted — anyone who receives the token can decode it with base64 and extract the raw Discord access token. Store the access token in an in-memory Map keyed by userId instead. The JWT now only contains userId, username, discriminator, and avatar. Routes that need the access token (auth /me, guilds routes) look it up from the session store. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cleanExpiredStates() was only called when a new /discord redirect was initiated. If OAuth is rarely used, expired state entries accumulate indefinitely. Add a setInterval that runs cleanExpiredStates every 5 minutes (unref'd to not block shutdown). Also call cleanExpiredStates() at the start of the callback handler. Wire the interval cleanup into stopServer via the exported stopAuthCleanup function. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Every request to a requireGuildAdmin-protected endpoint called fetchUserGuilds, making a full HTTP round-trip to Discord's API. Add a short-lived in-memory cache (90s TTL) keyed by userId so repeated requests within the TTL window reuse the cached result. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
The major security issues from previous rounds (access token in JWT, CSRF state, algorithm confusion, query-param token) have been addressed well. Four remaining issues:
🟡 Warning (4)
- Missing
requireGuildAdminon stats/members/moderation routes — Config and actions endpoints have the middleware, but/:id/stats,/:id/members, and/:id/moderationdo not. Any OAuth user can access sensitive data for any guild. sessionStoregrows unboundedly — No TTL or cleanup for stored Discord access tokens. Entries persist for the lifetime of the process.guildCachenever evicts expired entries — Stale entries are bypassed but never deleted, causing unbounded Map growth./logoutdoes not invalidate server-side session — The access token persists insessionStoreafter logout.
🔵 Nitpick (1)
5. Missing env vars in .env.example — SESSION_SECRET and DISCORD_REDIRECT_URI are used but not documented per AGENTS.md guidelines.
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
src/api/middleware/auth.jssrc/api/middleware/oauth.jssrc/api/routes/auth.jssrc/api/routes/guilds.jssrc/api/server.jssrc/commands/modlog.jssrc/utils/permissions.jstests/api/routes/auth.test.jstests/api/routes/guilds.test.jstests/commands/config.test.jstests/commands/modlog.test.jstests/utils/permissions.test.js
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,mjs}: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix when importing Node.js built-in modules (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals; double quotes are not permitted
Use 2-space indentation for all code
Files:
src/commands/modlog.jstests/commands/modlog.test.jssrc/api/server.jssrc/api/middleware/auth.jssrc/api/middleware/oauth.jstests/utils/permissions.test.jstests/api/routes/guilds.test.jstests/commands/config.test.jssrc/api/routes/auth.jssrc/utils/permissions.jssrc/api/routes/guilds.jstests/api/routes/auth.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER useconsole.log(),console.warn(),console.error(), or anyconsole.*method in src/ files; always use Winston logger instead viaimport { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig()fromsrc/modules/config.jsto read runtime configuration
UsesetConfigValue(key, value)fromsrc/modules/config.jsto update configuration at runtime
Always usesafeSend()wrapper fromsrc/utils/safeSend.jsfor sending messages to enforce allowedMentions and prevent mention exploits
UsesplitMessage()utility fromsrc/utils/splitMessage.jsto split messages exceeding Discord's 2000-character limit
Files:
src/commands/modlog.jssrc/api/server.jssrc/api/middleware/auth.jssrc/api/middleware/oauth.jssrc/api/routes/auth.jssrc/utils/permissions.jssrc/api/routes/guilds.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export adataobject usingSlashCommandBuilderand anexecute(interaction)async function
ExportadminOnly = truefrom command files to restrict execution to moderators
Files:
src/commands/modlog.js
🧠 Learnings (13)
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Always call `checkHierarchy(moderator, target)` from `src/utils/permissions.js` before executing moderation actions to prevent role hierarchy violations
Applied to files:
src/commands/modlog.jstests/commands/modlog.test.jstests/utils/permissions.test.jstests/commands/config.test.jssrc/utils/permissions.jssrc/api/routes/guilds.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*.js : Export `adminOnly = true` from command files to restrict execution to moderators
Applied to files:
src/commands/modlog.jstests/commands/modlog.test.jstests/utils/permissions.test.jstests/commands/config.test.jssrc/utils/permissions.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Moderation commands must follow the pattern: deferReply → validate inputs → sendDmNotification → execute Discord action → createCase → sendModLogEmbed → checkEscalation
Applied to files:
src/commands/modlog.jstests/commands/modlog.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick}*.js : DM the target user *before* executing kicks/bans in moderation commands; once a user is kicked/banned they cannot receive DMs
Applied to files:
src/commands/modlog.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*.js : Slash commands must export a `data` object using `SlashCommandBuilder` and an `execute(interaction)` async function
Applied to files:
src/commands/modlog.jstests/commands/modlog.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Test config structure, command exports, and utility functions; avoid testing live Discord connections (smoke/unit tests only)
Applied to files:
src/commands/modlog.jstests/commands/modlog.test.jstests/utils/permissions.test.jstests/api/routes/guilds.test.jstests/commands/config.test.jssrc/api/routes/guilds.jstests/api/routes/auth.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Pass structured metadata to Winston logging calls (e.g., `info('Message processed', { userId, channelId })`)
Applied to files:
src/api/middleware/auth.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Use Vitest for unit and integration tests; run `pnpm test` and `pnpm test:coverage` before every commit
Applied to files:
tests/api/routes/guilds.test.jstests/api/routes/auth.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Always use `safeSend()` wrapper from `src/utils/safeSend.js` for sending messages to enforce allowedMentions and prevent mention exploits
Applied to files:
tests/api/routes/guilds.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/index.js : Enable Discord intents for MessageContent, GuildMembers, and GuildVoiceStates in client initialization
Applied to files:
src/api/routes/auth.jssrc/api/routes/guilds.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*timeout*.js : Discord timeouts have a maximum duration of 28 days; enforce this cap in timeout commands
Applied to files:
src/api/routes/auth.jssrc/api/routes/guilds.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Keep documentation up to date after every code change; update README.md, AGENTS.md, CONTRIBUTING.md, .env.example, and config.json as needed
Applied to files:
src/api/routes/auth.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Update .env.example immediately when adding, removing, or renaming environment variables
Applied to files:
src/api/routes/auth.js
🧬 Code graph analysis (11)
src/commands/modlog.js (3)
src/modules/config.js (1)
getConfig(133-135)src/utils/permissions.js (2)
isModerator(111-130)getPermissionError(139-141)src/utils/safeSend.js (1)
safeReply(138-145)
tests/commands/modlog.test.js (2)
src/commands/modlog.js (2)
subcommand(43-43)execute(34-61)src/utils/permissions.js (1)
isModerator(111-130)
src/api/server.js (1)
src/api/routes/auth.js (1)
stopAuthCleanup(43-45)
src/api/middleware/auth.js (2)
src/api/routes/auth.js (2)
token(148-157)sessionSecret(102-102)src/logger.js (1)
warn(224-226)
tests/utils/permissions.test.js (1)
src/utils/permissions.js (5)
isAdmin(32-51)hasPermission(61-91)isGuildAdmin(100-102)isModerator(111-130)getPermissionError(139-141)
tests/api/routes/guilds.test.js (2)
src/api/routes/auth.js (3)
sessionStore(17-17)sessionStore(17-17)guilds(178-178)src/api/routes/guilds.js (5)
guildCache(58-58)guildCache(58-58)guilds(79-79)guilds(97-97)guilds(184-189)
tests/commands/config.test.js (3)
src/utils/permissions.js (1)
isGuildAdmin(100-102)src/commands/config.js (1)
execute(160-188)src/modules/config.js (1)
getConfig(133-135)
src/api/routes/auth.js (2)
src/logger.js (2)
error(231-233)info(217-219)src/api/middleware/oauth.js (1)
requireOAuth(15-39)
src/utils/permissions.js (5)
src/api/routes/guilds.js (1)
config(225-225)src/commands/modlog.js (2)
config(35-35)config(178-178)src/commands/config.js (3)
config(125-125)config(161-161)config(198-198)src/index.js (1)
config(57-57)src/modules/welcome.js (1)
level(246-246)
src/api/routes/guilds.js (2)
src/api/routes/auth.js (7)
DISCORD_API(14-14)accessToken(129-129)accessToken(176-176)guilds(178-178)user(142-142)sessionStore(17-17)sessionStore(17-17)web/src/lib/discord.server.ts (1)
fetchUserGuilds(70-128)
tests/api/routes/auth.test.js (2)
src/api/server.js (3)
app(26-26)app(94-94)createApp(25-79)src/api/routes/auth.js (3)
sessionStore(17-17)sessionStore(17-17)token(148-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (20)
src/api/middleware/oauth.js (1)
1-39: Overall implementation looks solid.Algorithm restriction to HS256, proper error logging import, clean Bearer extraction, and appropriate HTTP status codes. Previous review concerns have been addressed.
src/api/middleware/auth.js (1)
36-78: Dual-auth flow is clean and well-structured.The API-secret-first, JWT-fallback approach with
req.authMethodtagging is a sensible design. Constant-time comparison, HS256 algorithm pinning, and structured log metadata are all in place.src/api/routes/auth.js (2)
50-72: LGTM — OAuth initiation flow is well-structured.CSRF state generation, env var validation, expired state cleanup on entry, and proper URL construction are all handled correctly.
78-168: Callback flow handles errors properly.CSRF state validation with delete-on-use, abort timeouts on all fetches, server-side access token storage, and fragment-based token delivery are all solid. Error logging includes structured metadata.
src/api/server.js (1)
10-10: LGTM — Clean integration of auth cleanup and CORS update.
stopAuthCleanup()is correctly invoked early instopServer()to prevent further interval ticks, and theAuthorizationheader addition in CORS is required for Bearer token authentication.Also applies to: 41-41, 126-127
src/api/routes/guilds.js (1)
110-121: Guild admin middleware and route guards look correct.The
requireGuildAdminmiddleware properly gates onauthMethod, and its application to config read/write and actions endpoints aligns with the PR objectives. API-secret passthrough is appropriate for internal/bot calls.Also applies to: 224-224, 246-246, 407-407
src/commands/modlog.js (1)
34-41: LGTM — Permission guard is well-implemented.Runtime
isModeratorcheck withgetPermissionError('modlog', 'moderator')provides defense-in-depth alongside theadminOnlyexport, and the ephemeral reply prevents leaking error details to other users.tests/commands/modlog.test.js (1)
32-35: LGTM — Permission denial test properly exercises the guard.The
isModerator.mockReturnValueOnce(false)correctly tests the early-return path, and the assertion checks both the error content and ephemeral flag.Also applies to: 76-88
tests/utils/permissions.test.js (1)
1-321: LGTM — Thorough test coverage for the permission utilities.Tests cover all key paths: bot owner bypass (via multiple ID sources), Discord permission flags, config-based admin roles, null/missing config, and the parameterized error message. The null-config edge cases from previous review feedback are now included.
tests/commands/config.test.js (2)
9-9: Permission mocking setup looks good.The permissions module mock with
isGuildAdmindefaulting totrue(allowing most tests to pass the guard) andgetPermissionErrorreturning a realistic denial message is a clean approach. Thepermissionsfield in the config mock aligns with the new config shape.Also applies to: 15-18, 22-22
38-58: Permission denial test is well-structured.This test correctly overrides
isGuildAdminto returnfalsefor a single invocation and verifies the ephemeral error reply. Aligns with the source code guard insrc/commands/config.js(lines 161–166).src/utils/permissions.js (3)
9-23: Fallback bot owners with config-driven approach is a good improvement.The
DEFAULT_BOT_OWNERSfallback andisBotOwneracceptingconfigcleanly address the prior review feedback about the hardcoded constant. The null-safeconfig?.permissions?.botOwners || DEFAULT_BOT_OWNERScorrectly handles missing config while still allowing the override.
93-130:isGuildAdminandisModeratorare well-structured.
isGuildAdmincorrectly delegates toisAdmin(addressing the prior DRY feedback), andisModeratorprovides a sensible lower-privilege tier (ManageGuild + adminRoleId). The guard/bypass ordering mirrorsisAdmin, which is consistent.
132-141:getPermissionErrorupdate is clean.The
levelparameter with a sensible default provides flexible, user-facing error messages without breaking existing callers.tests/api/routes/guilds.test.js (3)
93-99: Good test isolation with cleanup of shared state.Clearing
sessionStore,guildCache, mocks, env stubs, and restoring spies ensures no leakage between tests.
136-146: JWT Bearer authentication test is well-constructed.The SESSION_SECRET env stub matches the signing secret used in
createOAuthToken, and the test correctly validates that the Bearer token is accepted as an alternative to thex-api-secretheader.
158-195: GET/tests cover the key authorization matrix well.The three tests verify: (1) API-secret gets unfiltered results, (2) OAuth gets only admin+bot-present guilds, and (3) OAuth with no admin perms gets empty results. The use of
permissions: '8'(ADMINISTRATOR bitmask) vs'0'is correct for Discord's permission model.tests/api/routes/auth.test.js (3)
1-34: Test setup is clean and minimal.Logger mock, lightweight client stub, and proper cleanup in
afterEachare all correct. PassingnullfordbPoolis appropriate since auth routes don't need database access.
36-69: OAuth redirect tests cover the happy path and both misconfiguration cases.Good coverage of the redirect URL components (client_id, scope, state) and the two failure modes (missing
DISCORD_CLIENT_ID, missingDISCORD_REDIRECT_URI).
71-85: Callback input validation tests are appropriate.Testing the
codeandstateparameter guards without mocking the full Discord token exchange keeps these tests focused and avoids coupling to external API behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/middleware/auth.js`:
- Around line 61-63: The check for a missing SESSION_SECRET (the sessionSecret
variable) wrongly returns 401; treat this as a server misconfiguration by
returning 500 and logging an error like src/api/middleware/oauth.js does. Update
the block that currently does "if (!sessionSecret) { return res.status(401)...
}" to log a clear error including the missing SESSION_SECRET (use the existing
logger variable in this module, or console.error if none) and respond with
res.status(500). Keep the response body similar (e.g., { error: 'Session not
configured' }) but ensure an error log is emitted before sending the 500.
In `@src/api/middleware/oauth.js`:
- Around line 26-28: The error log for the missing SESSION_SECRET (the call to
error(...) when sessionSecret is falsy in oauth middleware) should include
structured Winston metadata: update the error(...) invocation to pass an object
with request context like { method: req.method, url: req.originalUrl || req.url,
ip: req.ip, headers: req.headers && { host: req.headers.host },
sessionConfigured: false } so the log includes request-scoped details and a flag
indicating session config; keep the existing message string but add this
metadata object as the second argument before returning the
res.status(500).json(...) response.
In `@src/api/routes/auth.js`:
- Around line 16-17: sessionStore currently holds raw entries and grows without
eviction; implement TTL-based entries and periodic cleanup similar to
oauthStates. Change sessionStore to map userId → { accessToken, createdAt } (or
expiresAt) and update code that writes to it (login flow) to store the
timestamp; update the logout handler to remove the userId from sessionStore; add
a cleanup job (setInterval) that iterates sessionStore and deletes entries older
than the chosen TTL (e.g., 1 hour, matching JWT expiry); ensure any code that
reads sessionStore (token refresh or auth checks) respects the expiry and
removes stale entries.
- Around line 147-157: The jwt.sign call that creates `token` omits the explicit
signing algorithm; update the `jwt.sign` invocation (the token creation block
using `jwt.sign` and `sessionSecret`) to include the option `{ algorithm:
'HS256' }` alongside `expiresIn: '1h'` so it explicitly matches the verification
code that uses `algorithms: ['HS256']`.
- Around line 200-205: The logout route currently just returns a JSON message
but doesn't revoke the server-side session stored in sessionStore; update the
router.post('/logout', ...) handler to remove the user's session/Discord access
token from sessionStore (use whichever key you use elsewhere — e.g.,
req.user.id, req.sessionID, or the JWT subject) before returning the response,
handle the sessionStore removal as an async operation or check its return value
and log or return an error if deletion fails, and ensure the route still
responds with the success message only after the session has been cleared.
In `@src/api/routes/guilds.js`:
- Around line 74-83: fetchUserGuilds currently returns an empty array on any
non-OK Discord response which causes isOAuthGuildAdmin/requireGuildAdmin to
treat API failures as “no guilds” and return a misleading 403; change
fetchUserGuilds to throw an error (including response status/text) when
response.ok is false so callers can distinguish failure from an empty list, and
update requireGuildAdmin (or its caller) to catch that specific fetch error and
translate it into a 502 response (with an appropriate message) instead of a 403;
reference the functions fetchUserGuilds and requireGuildAdmin/isOAuthGuildAdmin
when making these changes.
In `@src/utils/permissions.js`:
- Around line 61-66: The early return guard in hasPermission currently returns
false when config is null before checking isBotOwner, making bot-owner bypass
unreachable; update hasPermission so the bot-owner check (call to isBotOwner)
runs before or regardless of the config null/undefined guard (e.g., validate
member and commandName first, then if isBotOwner(member, config) return true,
and only afterwards return false if config is missing or perform other
permission checks) to mirror the isAdmin pattern.
In `@tests/api/routes/auth.test.js`:
- Around line 173-180: Test currently only asserts a 200 and message for the
'POST /api/v1/auth/logout' endpoint; update the test to (1) create a logged-in
session (add a session entry to sessionStore or obtain a valid token via the
login helper), call the logout route with that token, then assert the
sessionStore no longer contains that session (or token is invalidated), and (2)
assert the endpoint enforces auth by calling it without a token and expecting a
401/unauthorized response; reference the POST /api/v1/auth/logout test, the
sessionStore object, and the auth middleware/token helper to locate where to add
these assertions.
- Around line 87-171: Add a test case to verify the /api/v1/auth/me route
rejects revoked/missing server-side sessions: in auth.test.js create a test
(e.g., "should return 401 when session is revoked/missing") that stubs
SESSION_SECRET, signs a JWT for userId '123' with jwt.sign, deliberately do NOT
set sessionStore.set('123', ...), call GET /api/v1/auth/me with Authorization:
Bearer <token>, and assert the response status is 401 and the response
body.error indicates the session is missing/revoked; reference the sessionStore,
jwt.sign, and the /api/v1/auth/me request to locate where to add this test.
In `@tests/api/routes/guilds.test.js`:
- Around line 101-120: The fetch mock in mockFetchGuilds is using
vi.spyOn(...).mockResolvedValue (persistent) which can hide extra fetch calls;
change mockResolvedValue to mockResolvedValueOnce in the mockFetchGuilds helper
so each test gets a single-use response (or explicitly chain multiple
mockResolvedValueOnce calls when a test expects multiple fetches), keeping
vi.restoreAllMocks() in afterEach as cleanup; update the helper name
mockFetchGuilds and its implementation to call
globalThis.fetch.mockResolvedValueOnce(...) to make single-request tests more
precise.
- Around line 211-257: Add a test that verifies a structurally valid JWT is
rejected when the server-side session is revoked: stub SESSION_SECRET, call
createOAuthToken() to get a token, ensure the in-memory/sessionStore entry for
that user is removed or mocked to return null (e.g., clear sessionStore or stub
its getter), mockFetchGuilds([{ id: 'guild1', permissions: '8' }]) so the guild
would be allowed by OAuth perms, then request GET '/api/v1/guilds/guild1/config'
with Authorization: Bearer <token> and assert a 403 response and an error
message indicating the session is invalid/revoked.
In `@tests/commands/config.test.js`:
- Around line 160-161: The test's dual getConfig mock
(getConfig.mockReturnValueOnce({}).mockReturnValueOnce(largeConfig)) is fragile
because the first empty object only works due to the isGuildAdmin mock; change
the first mocked return to the actual default config shape used by
execute()/permission checks so the permission guard passes regardless of
isGuildAdmin being modified—update references to getConfig in this test (and the
other instances around lines mentioned) to mockReturnValueOnce(defaultConfig)
before the second mockReturnValueOnce(largeConfig), ensuring execute() and
handleView() receive realistic config objects.
- Add requireGuildAdmin middleware to stats/members/moderation routes - Add SESSION_SECRET and DISCORD_REDIRECT_URI to .env.example - Fix logout to require auth and delete session from store - Add TTL-based SessionStore class with periodic cleanup - Clean expired guildCache entries on cache miss - Fix missing SESSION_SECRET to return 500 (not 401) with logging - Add request context metadata to oauth.js error() call - Add algorithm: 'HS256' to jwt.sign() for consistency - Throw on Discord API errors in fetchUserGuilds, return 502 - Move bot-owner check before config guard in hasPermission() - Add session revocation check to requireOAuth and requireAuth - Add revoked session tests for auth and guild routes - Update logout tests to verify session deletion and require auth - Use mockResolvedValueOnce in guild test helper - Use realistic config shapes in config.test.js mocks
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
196-198:⚠️ Potential issue | 🟡 MinorStale statement —
/modlogno longer requires admin.Line 198 says "All moderation commands require the admin role", but this PR demotes
/modlogtomoderatorlevel (config.json line 106) and its permission check resolves toisModerator(), which usesmoderatorRoleId.📝 Suggested fix
-All moderation commands require the admin role (configured via `permissions.adminRoleId`). +Most moderation commands require the admin role (`permissions.adminRoleId`). `/modlog` requires the moderator role (`permissions.moderatorRoleId`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 196 - 198, The README's statement "All moderation commands require the admin role" is stale because the /modlog command now checks isModerator() (using moderatorRoleId) instead of permissions.adminRoleId; update the README text under "Moderation Commands" to reflect that /modlog requires the moderator role (or change the sentence to say that most moderation commands require admin except /modlog which requires moderator) and reference the /modlog command and isModerator() permission check so the documentation matches the implementation.tests/commands/modlog.test.js (1)
149-162:⚠️ Potential issue | 🟡 Minor
should handle missing logging configno longer tests the intended scenario.Before this PR,
execute()didn't callgetConfig(), so the singlemockReturnValueOnce({ moderation: {} })was consumed byhandleView()— correctly simulating missing logging config. Now thatexecute()callsgetConfig()for the permission check, that one-time mock is consumed there;handleView()falls back to the factory default (which contains valid channels), so the test passes trivially without ever exercising the missing-config path.🛠️ Suggested fix
it('should handle missing logging config', async () => { - getConfig.mockReturnValueOnce({ moderation: {} }); + getConfig + .mockReturnValueOnce({ moderation: {} }) // execute() permission check + .mockReturnValueOnce({ moderation: {} }); // handleView() data fetch const interaction = createInteraction('view');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/commands/modlog.test.js` around lines 149 - 162, The test's single getConfig.mockReturnValueOnce is consumed by execute() now, so update the test to supply two sequential mocks: first return a config that satisfies the permission check (e.g., the factory/default config object) and then return { moderation: {} } for handleView(); implement this by calling getConfig.mockReturnValueOnce(defaultConfig) followed by getConfig.mockReturnValueOnce({ moderation: {} }) before creating the interaction and calling execute(interaction) so handleView() exercises the missing-logging-config path.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
README.mdconfig.jsonsrc/api/middleware/auth.jssrc/api/middleware/oauth.jssrc/api/middleware/oauthJwt.jssrc/api/routes/guilds.jssrc/api/utils/discordApi.jssrc/commands/config.jssrc/commands/modlog.jssrc/utils/errors.jstests/api/middleware/auth.test.jstests/api/middleware/oauth.test.jstests/commands/config.test.jstests/commands/modlog.test.js
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,mjs}: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix when importing Node.js built-in modules (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals; double quotes are not permitted
Use 2-space indentation for all code
Files:
tests/commands/modlog.test.jstests/api/middleware/auth.test.jssrc/commands/modlog.jssrc/api/middleware/oauthJwt.jstests/api/middleware/oauth.test.jssrc/utils/errors.jssrc/api/middleware/auth.jssrc/api/routes/guilds.jssrc/api/middleware/oauth.jstests/commands/config.test.jssrc/api/utils/discordApi.jssrc/commands/config.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER useconsole.log(),console.warn(),console.error(), or anyconsole.*method in src/ files; always use Winston logger instead viaimport { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig()fromsrc/modules/config.jsto read runtime configuration
UsesetConfigValue(key, value)fromsrc/modules/config.jsto update configuration at runtime
Always usesafeSend()wrapper fromsrc/utils/safeSend.jsfor sending messages to enforce allowedMentions and prevent mention exploits
UsesplitMessage()utility fromsrc/utils/splitMessage.jsto split messages exceeding Discord's 2000-character limit
Files:
src/commands/modlog.jssrc/api/middleware/oauthJwt.jssrc/utils/errors.jssrc/api/middleware/auth.jssrc/api/routes/guilds.jssrc/api/middleware/oauth.jssrc/api/utils/discordApi.jssrc/commands/config.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export adataobject usingSlashCommandBuilderand anexecute(interaction)async function
ExportadminOnly = truefrom command files to restrict execution to moderators
Files:
src/commands/modlog.jssrc/commands/config.js
🧠 Learnings (14)
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Test config structure, command exports, and utility functions; avoid testing live Discord connections (smoke/unit tests only)
Applied to files:
tests/commands/modlog.test.jstests/api/middleware/auth.test.jssrc/commands/modlog.jssrc/api/routes/guilds.jstests/commands/config.test.jssrc/api/utils/discordApi.jssrc/commands/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Always call `checkHierarchy(moderator, target)` from `src/utils/permissions.js` before executing moderation actions to prevent role hierarchy violations
Applied to files:
tests/commands/modlog.test.jsREADME.mdsrc/commands/modlog.jssrc/api/routes/guilds.jstests/commands/config.test.jsconfig.jsonsrc/commands/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*.js : Export `adminOnly = true` from command files to restrict execution to moderators
Applied to files:
tests/commands/modlog.test.jsREADME.mdsrc/commands/modlog.jssrc/api/routes/guilds.jstests/commands/config.test.jssrc/commands/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Moderation commands must follow the pattern: deferReply → validate inputs → sendDmNotification → execute Discord action → createCase → sendModLogEmbed → checkEscalation
Applied to files:
tests/commands/modlog.test.jsREADME.mdsrc/commands/modlog.jssrc/api/routes/guilds.jssrc/commands/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*.js : Slash commands must export a `data` object using `SlashCommandBuilder` and an `execute(interaction)` async function
Applied to files:
tests/commands/modlog.test.jssrc/commands/modlog.jssrc/commands/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/modules/*.js : Check `config.yourModule.enabled` before processing any module logic to allow runtime enable/disable
Applied to files:
src/commands/modlog.jssrc/commands/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use custom error classes from `src/utils/errors.js` for error handling
Applied to files:
src/utils/errors.jssrc/api/utils/discordApi.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use `splitMessage()` utility from `src/utils/splitMessage.js` to split messages exceeding Discord's 2000-character limit
Applied to files:
src/utils/errors.jssrc/api/utils/discordApi.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Pass structured metadata to Winston logging calls (e.g., `info('Message processed', { userId, channelId })`)
Applied to files:
src/api/middleware/auth.jssrc/api/routes/guilds.jssrc/api/middleware/oauth.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*timeout*.js : Discord timeouts have a maximum duration of 28 days; enforce this cap in timeout commands
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/index.js : Enable Discord intents for MessageContent, GuildMembers, and GuildVoiceStates in client initialization
Applied to files:
src/api/routes/guilds.jssrc/api/utils/discordApi.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/modules/moderation.js : Case numbering must be per-guild sequential and assigned atomically using `COALESCE(MAX(case_number), 0) + 1` in a single INSERT statement within `createCase()`
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-04T02:20:09.131Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T02:20:09.131Z
Learning: Applies to src/**/*.{ts,tsx} : Use `reportError(context, error)` from `src/lib/logger.ts` to report errors to Sentry with context metadata, falling back to console.error if Sentry is disabled
Applied to files:
src/api/middleware/oauth.jssrc/api/utils/discordApi.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Always log errors with context before re-throwing
Applied to files:
src/api/utils/discordApi.js
🧬 Code graph analysis (11)
tests/api/middleware/auth.test.js (3)
src/api/utils/sessionStore.js (2)
sessionStore(65-65)sessionStore(65-65)src/api/middleware/verifyJwt.js (1)
_resetSecretCache(18-20)src/api/middleware/auth.js (1)
requireAuth(36-70)
src/commands/modlog.js (3)
src/modules/config.js (1)
getConfig(133-135)src/utils/permissions.js (2)
hasPermission(61-97)getPermissionError(162-164)src/utils/safeSend.js (1)
safeReply(138-145)
src/api/middleware/oauthJwt.js (2)
src/api/middleware/verifyJwt.js (1)
verifyJwtToken(37-50)src/logger.js (1)
error(231-233)
tests/api/middleware/oauth.test.js (4)
src/api/utils/sessionStore.js (2)
sessionStore(65-65)sessionStore(65-65)src/api/middleware/verifyJwt.js (1)
_resetSecretCache(18-20)src/api/middleware/oauth.js (1)
requireOAuth(14-18)src/api/middleware/oauthJwt.js (1)
token(31-31)
src/utils/errors.js (2)
src/api/utils/discordApi.js (1)
status(58-58)src/commands/status.js (2)
status(78-78)status(115-115)
src/api/middleware/auth.js (3)
src/api/routes/auth.js (2)
req(140-140)req(263-263)src/logger.js (1)
warn(224-226)src/api/middleware/oauthJwt.js (1)
handleOAuthJwt(30-56)
src/api/routes/guilds.js (3)
src/api/utils/sessionStore.js (1)
getSessionToken(74-76)src/api/utils/discordApi.js (2)
guilds(62-62)fetchUserGuilds(36-72)src/logger.js (2)
error(231-233)warn(224-226)
src/api/middleware/oauth.js (1)
src/api/middleware/oauthJwt.js (1)
handleOAuthJwt(30-56)
tests/commands/config.test.js (2)
src/utils/permissions.js (1)
hasPermission(61-97)src/commands/config.js (4)
execute(160-189)config(125-125)config(161-161)config(199-199)
src/api/utils/discordApi.js (2)
src/logger.js (1)
error(231-233)src/utils/errors.js (2)
status(63-63)DiscordApiError(39-49)
src/commands/config.js (2)
src/modules/config.js (1)
getConfig(133-135)src/utils/permissions.js (2)
hasPermission(61-97)getPermissionError(162-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Cursor Bugbot
🔇 Additional comments (13)
src/commands/config.js (1)
161-168: Permission gate is correct — clean implementation.
getConfig()is called once here for the guard and again within the subcommand handlers, which is intentional. ThepermLevelfallback to'administrator'when the command isn't inallowedCommandsis the right safe default, consistent withhasPermission's own fallback insrc/utils/permissions.js(line 79).src/commands/modlog.js (1)
35-42: LGTM — mirrors the config.js guard, and thepermLevellookup correctly surfaces'moderator'fromconfig.permissions.allowedCommands.modlog. Both past review comments (misleading error message, missing denial test) are addressed.config.json (1)
85-86: LGTM — the past issue ofbotOwnersbeing unused (hardcoded constant) is resolved. Deployer notice in README line 194 appropriately flags that the default value should be replaced before deployment.src/api/middleware/oauthJwt.js (1)
14-55: LGTM — well-factored helper with correct token extraction, error delegation, and boolean sentinel return. The selective logging (only on 500 misconfiguration, not on 401 token failures) is appropriate.src/api/middleware/auth.js (1)
36-68: LGTM — dual-auth flow is logically complete and correct. The explicit fail-fast onisValidSecret === false(lines 52–57) deliberately avoids JWT fallthrough for a configured-but-wrong secret, which is the right security posture. All three past review issues (duplicate branch, missing metadata, inconsistent log level) are resolved.src/api/middleware/oauth.js (1)
1-18: Clean middleware delegation — LGTM.The
requireOAuthmiddleware cleanly delegates tohandleOAuthJwtwith appropriate options. Previous review concerns (algorithm confusion, missing logging, duplication) have all been addressed in the sharedoauthJwt.jsmodule.tests/api/middleware/oauth.test.js (1)
1-136: Solid test coverage for the OAuth middleware — LGTM.All key scenarios are covered: missing/malformed tokens, missing
SESSION_SECRET, invalid/expired JWTs, revoked sessions, and the happy path withreq.authMethodandreq.userassertions. Previous review feedback (mockip/path, session revocation test,authMethodassertion) has been addressed.src/utils/errors.js (1)
36-49: Well-placed custom error class — LGTM.
DiscordApiErroris correctly centralized in the canonical error module per coding guidelines. The class properly extendsError, setsthis.name, and carries the HTTP status code.tests/api/middleware/auth.test.js (1)
1-193: Comprehensive dual-auth test suite — LGTM.Good coverage of the interplay between API-secret and JWT authentication paths: fallback behavior when
BOT_API_SECRETis unconfigured, immediate rejection on invalid API secret (no JWT fallback), and properauthMethod/req.userpopulation. Previousasyncnit has been addressed.src/api/routes/guilds.js (3)
58-100: Well-structured permission model — LGTM.The
hasOAuthGuildPermission→isOAuthGuildAdmin/isOAuthGuildModeratorhierarchy is clean. Admin checks onlyADMINISTRATOR_FLAG, moderator checksADMINISTRATOR_FLAG | MANAGE_GUILD_FLAG— consistent with the slash-command permission model per the PR objectives.
110-136: Good defensive middleware with proper error handling — LGTM.The
requireGuildPermissionfactory correctly handles all three auth-method branches: API-secret passthrough, OAuth with Discord API error handling (502), and unknown auth rejection. Structured logging includesuserIdandguildcontext.
184-197: Nice use ofreducewith singlebotGuilds.get()to avoid TOCTOU — LGTM.The single-lookup pattern at line 188-189 avoids the previous
has/getrace condition. Thereduceaccumulator pattern is clean and efficient.src/api/utils/discordApi.js (1)
36-72: Overall: well-implemented caching utility — LGTM on the rest.The access token validation, AbortSignal timeout, response shape validation, DiscordApiError usage, TTL-based caching with periodic cleanup, and
unref()'d interval are all solid. The function correctly importsDiscordApiErrorfrom the canonicalsrc/utils/errors.jsmodule per coding guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/routes/guilds.js`:
- Around line 171-222: The OAuth guild list currently filters with
ADMINISTRATOR_FLAG | MANAGE_GUILD_FLAG but admin routes (requireGuildAdmin) only
check ADMINISTRATOR_FLAG, causing UX inconsistency; update the router.get
handler (the anonymous route registered with router.get) to either (A) only
include guilds where (Number(ug.permissions) & ADMINISTRATOR_FLAG) !== 0 so the
list matches requireGuildAdmin, or (B) keep both flags but augment each returned
guild object (built in the reduce/map that references botGuilds.get and
g.iconURL()) with a permissions or role field (e.g., permissions:
Number(ug.permissions) and/or isAdmin: Boolean(Number(ug.permissions) &
ADMINISTRATOR_FLAG)) so the dashboard can decide which actions to show; ensure
fetchUserGuilds/getSessionToken error handling remains unchanged and update any
client expectations accordingly.
In `@src/api/utils/discordApi.js`:
- Around line 65-70: The eviction logic for guildCache only deletes a single
oldest entry when size exceeds MAX_GUILD_CACHE_SIZE, which can let the cache
grow beyond the limit under burst/concurrent calls; update the eviction in the
same area that sets guildCache (related to fetchUserGuilds) to loop while
(guildCache.size > MAX_GUILD_CACHE_SIZE), deleting the oldest key each iteration
(using guildCache.keys().next().value) until the size is within the cap so
concurrent inserts cannot temporarily exceed the configured maximum.
In `@tests/commands/config.test.js`:
- Around line 65-111: The hasPermission.mockImplementationOnce calls are
duplicating the real logic and coupling the tests to implementation; replace
each hasPermission.mockImplementationOnce(...) in these two tests with a simple
hasPermission.mockReturnValueOnce(true) (or remove the mock entirely if
getConfig already implies permission) so the tests only assert behavior of
execute(interaction) using getConfig and interaction.reply, referencing
hasPermission, execute, getConfig, and interaction.reply to locate the changes.
In `@tests/commands/modlog.test.js`:
- Around line 100-102: The test uses hasPermission.mockImplementationOnce(...)
to replicate the module's internal bypass logic; replace those implementations
(the occurrences using hasPermission.mockImplementationOnce at the shown call
sites) with hasPermission.mockReturnValueOnce(true) to simplify the mock and
remove coupling to the internal config logic—do the same for the similar
occurrences around lines 117-119.
---
Outside diff comments:
In `@README.md`:
- Around line 196-198: The README's statement "All moderation commands require
the admin role" is stale because the /modlog command now checks isModerator()
(using moderatorRoleId) instead of permissions.adminRoleId; update the README
text under "Moderation Commands" to reflect that /modlog requires the moderator
role (or change the sentence to say that most moderation commands require admin
except /modlog which requires moderator) and reference the /modlog command and
isModerator() permission check so the documentation matches the implementation.
In `@tests/commands/modlog.test.js`:
- Around line 149-162: The test's single getConfig.mockReturnValueOnce is
consumed by execute() now, so update the test to supply two sequential mocks:
first return a config that satisfies the permission check (e.g., the
factory/default config object) and then return { moderation: {} } for
handleView(); implement this by calling
getConfig.mockReturnValueOnce(defaultConfig) followed by
getConfig.mockReturnValueOnce({ moderation: {} }) before creating the
interaction and calling execute(interaction) so handleView() exercises the
missing-logging-config path.
---
Duplicate comments:
In `@src/api/utils/discordApi.js`:
- Around line 62-63: When the Discord API response parsed into "guilds" is not
an array, log an error with context before throwing: use the same logger used
elsewhere in this module (the logger/processLogger identifier already used on
lines ~38–60) to emit a descriptive message that includes the offending "guilds"
payload (or its type/summary) and any relevant request context, then throw the
Error as currently done; modify the block around the "const guilds = await
response.json()" check to call logger.error(...) with this information
immediately before the throw.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/routes/guilds.js (1)
343-345:⚠️ Potential issue | 🟡 MinorAdd
guildto the error log metadata for diagnosability.The error log at line 344 includes
pathanderrorbut omits the guild context available inreq.params.id. Other error logs in this file consistently include the guild.🐛 Proposed fix
- error('Failed to update config via API', { path, error: err.message }); + error('Failed to update config via API', { path, error: err.message, guild: req.params.id });Based on learnings: "Pass structured metadata to Winston logging calls (e.g.,
info('Message processed', { userId, channelId }))"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/routes/guilds.js` around lines 343 - 345, The error log inside the catch block for the config update is missing the guild context; update the metadata passed to the error logger (the call to error('Failed to update config via API', { path, error: err.message })) to include the guild id from req.params.id (e.g., add guild: req.params.id) so the log becomes consistent with other logs in this file and aids diagnosability.tests/commands/config.test.js (1)
142-158: 🧹 Nitpick | 🔵 TrivialInteraction fixtures in existing tests lack
memberfield.The
interactionobjects inviewsubcommand tests (and some others likeset,reset) don't include amemberproperty, unlike the new permission tests. This works only becausehasPermissionis globally mocked to returntrue. If the mock is ever reset or removed in a future refactor, these tests will break for the wrong reason (memberbeingundefinedrather than a permission issue).Consider adding
member: {}to all interaction fixtures for consistency with the new tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/commands/config.test.js` around lines 142 - 158, The interaction fixtures in the config command tests (e.g., the 'view' subcommand test that calls execute(interaction)) are missing a member property, which can mask permission-related failures tied to hasPermission; update the interaction objects used in the 'view', 'set', and 'reset' subcommand tests to include member: {} so interactions mimic real Discord payloads and future changes to the global hasPermission mock won't cause unrelated failures, ensuring execute and its permission checks receive a defined interaction.member.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
README.mdsrc/api/routes/guilds.jssrc/api/utils/discordApi.jssrc/utils/permissions.jstests/api/routes/guilds.test.jstests/api/utils/discordApi.test.jstests/commands/config.test.jstests/commands/modlog.test.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,mjs}: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix when importing Node.js built-in modules (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals; double quotes are not permitted
Use 2-space indentation for all code
Files:
tests/commands/modlog.test.jstests/api/utils/discordApi.test.jstests/commands/config.test.jssrc/utils/permissions.jssrc/api/routes/guilds.jstests/api/routes/guilds.test.jssrc/api/utils/discordApi.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER useconsole.log(),console.warn(),console.error(), or anyconsole.*method in src/ files; always use Winston logger instead viaimport { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig()fromsrc/modules/config.jsto read runtime configuration
UsesetConfigValue(key, value)fromsrc/modules/config.jsto update configuration at runtime
Always usesafeSend()wrapper fromsrc/utils/safeSend.jsfor sending messages to enforce allowedMentions and prevent mention exploits
UsesplitMessage()utility fromsrc/utils/splitMessage.jsto split messages exceeding Discord's 2000-character limit
Files:
src/utils/permissions.jssrc/api/routes/guilds.jssrc/api/utils/discordApi.js
🧠 Learnings (15)
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Test config structure, command exports, and utility functions; avoid testing live Discord connections (smoke/unit tests only)
Applied to files:
tests/commands/modlog.test.jstests/api/utils/discordApi.test.jstests/commands/config.test.jssrc/utils/permissions.jssrc/api/routes/guilds.jstests/api/routes/guilds.test.jssrc/api/utils/discordApi.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Always call `checkHierarchy(moderator, target)` from `src/utils/permissions.js` before executing moderation actions to prevent role hierarchy violations
Applied to files:
tests/commands/modlog.test.jstests/commands/config.test.jsREADME.mdsrc/utils/permissions.jssrc/api/routes/guilds.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*.js : Export `adminOnly = true` from command files to restrict execution to moderators
Applied to files:
tests/commands/modlog.test.jstests/commands/config.test.jsREADME.mdsrc/utils/permissions.jssrc/api/routes/guilds.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Moderation commands must follow the pattern: deferReply → validate inputs → sendDmNotification → execute Discord action → createCase → sendModLogEmbed → checkEscalation
Applied to files:
tests/commands/modlog.test.jsREADME.mdsrc/utils/permissions.jssrc/api/routes/guilds.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*.js : Slash commands must export a `data` object using `SlashCommandBuilder` and an `execute(interaction)` async function
Applied to files:
tests/commands/modlog.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Use Vitest for unit and integration tests; run `pnpm test` and `pnpm test:coverage` before every commit
Applied to files:
tests/api/utils/discordApi.test.jstests/api/routes/guilds.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*timeout*.js : Discord timeouts have a maximum duration of 28 days; enforce this cap in timeout commands
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Pass structured metadata to Winston logging calls (e.g., `info('Message processed', { userId, channelId })`)
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/index.js : Enable Discord intents for MessageContent, GuildMembers, and GuildVoiceStates in client initialization
Applied to files:
src/api/routes/guilds.jssrc/api/utils/discordApi.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/modules/moderation.js : Case numbering must be per-guild sequential and assigned atomically using `COALESCE(MAX(case_number), 0) + 1` in a single INSERT statement within `createCase()`
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Always use `safeSend()` wrapper from `src/utils/safeSend.js` for sending messages to enforce allowedMentions and prevent mention exploits
Applied to files:
tests/api/routes/guilds.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Always log errors with context before re-throwing
Applied to files:
src/api/utils/discordApi.js
📚 Learning: 2026-02-04T02:20:09.131Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T02:20:09.131Z
Learning: Applies to src/**/*.{ts,tsx} : Use `reportError(context, error)` from `src/lib/logger.ts` to report errors to Sentry with context metadata, falling back to console.error if Sentry is disabled
Applied to files:
src/api/utils/discordApi.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use `splitMessage()` utility from `src/utils/splitMessage.js` to split messages exceeding Discord's 2000-character limit
Applied to files:
src/api/utils/discordApi.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use custom error classes from `src/utils/errors.js` for error handling
Applied to files:
src/api/utils/discordApi.js
🧬 Code graph analysis (6)
tests/api/utils/discordApi.test.js (1)
src/api/utils/discordApi.js (3)
guildCache(10-10)guildCache(10-10)fetchUserGuilds(36-72)
tests/commands/config.test.js (3)
src/utils/permissions.js (1)
hasPermission(61-97)src/commands/config.js (1)
execute(160-189)src/modules/config.js (1)
getConfig(133-135)
src/utils/permissions.js (1)
src/index.js (2)
config(57-57)owners(322-322)
src/api/routes/guilds.js (4)
src/api/utils/sessionStore.js (1)
getSessionToken(74-76)src/api/utils/discordApi.js (2)
guilds(62-62)fetchUserGuilds(36-72)src/modules/config.js (2)
getConfig(133-135)err(33-33)src/logger.js (2)
error(231-233)warn(224-226)
tests/api/routes/guilds.test.js (4)
src/api/utils/sessionStore.js (2)
sessionStore(65-65)sessionStore(65-65)src/api/utils/discordApi.js (2)
guildCache(10-10)guildCache(10-10)src/api/middleware/verifyJwt.js (2)
_resetSecretCache(18-20)secret(38-38)src/modules/config.js (1)
getConfig(133-135)
src/api/utils/discordApi.js (2)
src/logger.js (1)
error(231-233)src/utils/errors.js (2)
status(63-63)DiscordApiError(39-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (19)
src/utils/permissions.js (2)
16-23: Clean config-driven bot owner check.The
isBotOwnerhelper properly validates the owners array and safely extracts the user ID. Previous feedback about hardcoded owner IDs and nullish coalescing has been addressed well.
111-114: Alias pattern for future divergence is well-documented.The JSDoc and TODO reference to Issue
#71clearly communicates intent. Delegating toisAdminavoids the prior DRY violation.tests/commands/modlog.test.js (3)
76-93: Permission denial test is well-structured.This addresses the prior feedback about missing denial path coverage. The test properly verifies both the
hasPermissioninvocation args and the ephemeral error reply.
95-123: Permission-disabled tests usemockReturnValueOnce(true)correctly.Previous feedback about
mockImplementationOncereplicating internal logic has been addressed — tests now use simple return value mocks, which is the right approach when the dependency is fully mocked.
147-153: DualgetConfigmock chain reflects the two-phase config retrieval.The first call returns a config with a
permissionsstructure for the permission check inexecute(), while the second returns partial config forhandleView(). This is correct for exercising the "missing logging config" path.src/api/utils/discordApi.js (2)
1-13: Well-structured module with proper imports from canonical locations.
DiscordApiErroris now imported fromsrc/utils/errors.js(addressing prior feedback), and the logger is correctly used. The cache constants are reasonable.
36-43: Defensive token validation is solid.Checks both type and emptiness, logs with structured metadata before throwing. This addresses previous feedback well.
tests/api/utils/discordApi.test.js (1)
15-28: Eviction test correctly validates the cache cap behavior.The test seeds 10,005 entries, triggers one more via
fetchUserGuilds, and asserts the cache is capped at 10,000. This exercises thewhileloop eviction path well.Consider adding tests for cache hit/miss, invalid token, and Discord API error paths to improve coverage of this utility module.
README.md (1)
190-194: Permissions configuration documentation is thorough.
moderatorRoleId,botOwners, andallowedCommandsare now properly documented, addressing the prior review concern aboutmoderatorRoleIdbeing undocumented. The fork/deployer warning about defaultbotOwnersis a good security practice.tests/commands/config.test.js (2)
38-63: Permission denial test is comprehensive.Verifies both the
hasPermissioncall signature and the ephemeral error reply. This addresses the prior review feedback about missing denial path coverage.
209-212: DualgetConfigmock chain is correct and well-commented.The first call returns a minimal config with permissions for the
execute()guard, and the second returns the large config forhandleView(). The inline comment explains the pattern clearly.src/api/routes/guilds.js (4)
58-76:hasOAuthGuildPermissionfetches fresh permissions — good resolution of stale JWT issue.Fetching guild permissions on-demand via
fetchUserGuilds(with its 90s TTL cache) instead of relying on JWT-embedded permissions addresses the prior staleness concern effectively.
122-150:requireGuildPermissionmiddleware is well-layered.Clean separation of api-secret bypass, bot-owner bypass, OAuth permission check, and unknown auth fallback. The 502 on Discord API errors (vs. misleading 403) addresses prior feedback.
188-254: GET/guild list handles all auth paths correctly with access metadata.The
accessfield ('bot-owner','admin','moderator') in the response is a good addition for dashboard UX — it lets the frontend decide which actions to show per guild, addressing the prior concern about users seeing guilds where they'd get 403 on admin endpoints.The single
botGuilds.get(ug.id)lookup (line 219) eliminates the TOCTOU race from the previoushas/getpattern.
256-278: Middleware ordering (requireGuildAdmin→validateGuild) is correct for security.Placing the permission check before guild validation prevents leaking bot guild presence to unauthorized OAuth users. An unauthorized user gets 403 rather than 404.
tests/api/routes/guilds.test.js (4)
104-124: OAuth test helpers are clean and reusable.
createOAuthTokencorrectly pairs JWT creation with session store population, andmockFetchGuildsusesmockResolvedValueOnceas suggested in prior feedback. The default parameters make tests concise while remaining explicit.
154-167: Revoked session test is a valuable addition.This addresses the prior feedback about testing structurally valid JWTs with no server-side session, ensuring the session store check isn't bypassed.
301-381: Guild admin verification suite is thorough and well-structured.Covers all critical authorization paths: api-secret bypass, OAuth admin, MANAGE_GUILD-only denied on admin endpoints, no permissions, not in guild, and bot-owner bypass. The test at lines 322–334 specifically validates that
isOAuthGuildAdminchecks onlyADMINISTRATOR_FLAG(notMANAGE_GUILD_FLAG), which aligns with the fix for the prior permission inconsistency.
637-687: Moderation endpoint OAuth tests correctly validate moderator-level access.The MANAGE_GUILD test (line 640) confirms that the
/moderationendpoint usesrequireGuildModeratorrather thanrequireGuildAdmin, and the bot-owner test verifies the bypass path withfetchSpyassertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/permissions.js`:
- Around line 123-153: The isModerator function mixes two styles for role
checks: one for adminRoleId that uses an if and explicit return true and another
for moderatorRoleId that returns the boolean expression directly; make them
consistent by changing the moderatorRoleId block to mirror the adminRoleId
pattern (check config.permissions?.moderatorRoleId then if
(member.roles.cache.has(config.permissions.moderatorRoleId)) return true) so the
function uniformly uses explicit role-check branches; reference isModerator,
config.permissions?.moderatorRoleId, config.permissions?.adminRoleId,
member.roles.cache.has, PermissionFlagsBits.Administrator,
PermissionFlagsBits.ManageGuild, and isBotOwner when locating the code to
update.
---
Outside diff comments:
In `@src/api/routes/guilds.js`:
- Around line 343-345: The error log inside the catch block for the config
update is missing the guild context; update the metadata passed to the error
logger (the call to error('Failed to update config via API', { path, error:
err.message })) to include the guild id from req.params.id (e.g., add guild:
req.params.id) so the log becomes consistent with other logs in this file and
aids diagnosability.
In `@tests/commands/config.test.js`:
- Around line 142-158: The interaction fixtures in the config command tests
(e.g., the 'view' subcommand test that calls execute(interaction)) are missing a
member property, which can mask permission-related failures tied to
hasPermission; update the interaction objects used in the 'view', 'set', and
'reset' subcommand tests to include member: {} so interactions mimic real
Discord payloads and future changes to the global hasPermission mock won't cause
unrelated failures, ensuring execute and its permission checks receive a defined
interaction.member.
---
Duplicate comments:
In `@README.md`:
- Around line 92-108: The main environment variables table is missing the
OAuth/dashboard vars—add rows for SESSION_SECRET, DISCORD_CLIENT_SECRET, and
DISCORD_REDIRECT_URI to that table (with requiredness set to ✅ where
applicable), matching the descriptions from the Railway Bot Service section:
SESSION_SECRET (✅) — session signing secret for dashboard OAuth,
DISCORD_CLIENT_SECRET (✅) — Discord application client secret used in OAuth, and
DISCORD_REDIRECT_URI (✅) — OAuth redirect URL for Discord; ensure names match
exactly (SESSION_SECRET, DISCORD_CLIENT_SECRET, DISCORD_REDIRECT_URI) and keep
any notes about Railway-specific usage consistent with the existing footnotes.
In `@src/api/utils/discordApi.js`:
- Around line 62-63: Before throwing when the parsed guilds is not an array, log
the failure with context first: call error(...) with a descriptive message
including the raw response status/text or the parsed value (the variable guilds)
and any request identifying info, then throw the same Error; locate the check
after const guilds = await response.json() and mirror the logging pattern used
in the earlier error paths (lines checking response.ok and empty body) so the
non-array path logs before throwing.
| export function isModerator(member, config) { | ||
| if (!member) return false; | ||
|
|
||
| // Bot owner always returns true | ||
| if (isBotOwner(member, config)) return true; | ||
|
|
||
| if (!config) return false; | ||
|
|
||
| // Administrator is strictly higher privilege — always implies moderator | ||
| if (member.permissions.has(PermissionFlagsBits.Administrator)) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check Discord Manage Guild permission | ||
| if (member.permissions.has(PermissionFlagsBits.ManageGuild)) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check bot admin role from config | ||
| if (config.permissions?.adminRoleId) { | ||
| if (member.roles.cache.has(config.permissions.adminRoleId)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (config.permissions?.moderatorRoleId) { | ||
| return member.roles.cache.has(config.permissions.moderatorRoleId); | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
isModerator has an inconsistent early-return pattern for the moderatorRoleId check.
Lines 142–146 use an if/nested-if with explicit return true, while the structurally identical moderatorRoleId block at lines 148–150 returns the boolean expression directly. This is harmless but inconsistent within the same function.
More importantly, the moderatorRoleId config key is now documented in the README (line 190), which addresses the prior review concern. The logic itself is correct.
♻️ Minor consistency cleanup
// Check bot admin role from config
if (config.permissions?.adminRoleId) {
- if (member.roles.cache.has(config.permissions.adminRoleId)) {
- return true;
- }
+ if (member.roles.cache.has(config.permissions.adminRoleId)) return true;
}
if (config.permissions?.moderatorRoleId) {
- return member.roles.cache.has(config.permissions.moderatorRoleId);
+ if (member.roles.cache.has(config.permissions.moderatorRoleId)) return true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/permissions.js` around lines 123 - 153, The isModerator function
mixes two styles for role checks: one for adminRoleId that uses an if and
explicit return true and another for moderatorRoleId that returns the boolean
expression directly; make them consistent by changing the moderatorRoleId block
to mirror the adminRoleId pattern (check config.permissions?.moderatorRoleId
then if (member.roles.cache.has(config.permissions.moderatorRoleId)) return
true) so the function uniformly uses explicit role-check branches; reference
isModerator, config.permissions?.moderatorRoleId,
config.permissions?.adminRoleId, member.roles.cache.has,
PermissionFlagsBits.Administrator, PermissionFlagsBits.ManageGuild, and
isBotOwner when locating the code to update.
- Add CSRF state parameter to OAuth2 flow with expiry map (Thread 1)
- Switch JWT redirect from query param to fragment-based (#token=) (Thread 3+14)
- Add AbortSignal.timeout(10_000) to all Discord API fetch calls (Thread 12)
- Store Discord accessToken in JWT, remove guilds, shorten expiry to 1h (Thread 13+15)
- Use requireOAuth middleware on /me route instead of inline JWT verification (Thread 4+9)
- Add error logging when SESSION_SECRET is missing in oauth.js (Thread 10)
- Add { algorithms: ['HS256'] } to jwt.verify in auth.js and oauth.js (Thread 11)
- Collapse duplicate unauthorized branches in requireAuth (Thread 7)
- Add structured metadata to BOT_API_SECRET warning log (Thread 8)
- Replace hardcoded BOT_OWNER_ID with config.permissions.botOwners array (Thread 2+6+17)
- Delegate isGuildAdmin to isAdmin to remove duplication (Thread 5+18)
- Parameterize getPermissionError with level parameter (Thread 16)
- Fetch fresh guilds from Discord in guilds.js routes (Thread 13+15)
- Add permission denial tests for config and modlog commands (Thread 19+20)
- Add tests for isGuildAdmin/isModerator with null config (Thread 21+22)
- Update existing API tests for new OAuth flow (accessToken in JWT, fetch mocks)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add requireGuildAdmin middleware to stats/members/moderation routes - Add SESSION_SECRET and DISCORD_REDIRECT_URI to .env.example - Fix logout to require auth and delete session from store - Add TTL-based SessionStore class with periodic cleanup - Clean expired guildCache entries on cache miss - Fix missing SESSION_SECRET to return 500 (not 401) with logging - Add request context metadata to oauth.js error() call - Add algorithm: 'HS256' to jwt.sign() for consistency - Throw on Discord API errors in fetchUserGuilds, return 502 - Move bot-owner check before config guard in hasPermission() - Add session revocation check to requireOAuth and requireAuth - Add revoked session tests for auth and guild routes - Update logout tests to verify session deletion and require auth - Use mockResolvedValueOnce in guild test helper - Use realistic config shapes in config.test.js mocks
- Add file-level docblock and JSDoc to verifyJwt.js - Add file-level docblock to discordApi.js - Wire stopGuildCacheCleanup into server shutdown (was re-exported but never called) - Remove redundant re-export from guilds.js (server imports directly from discordApi) Note: permissions.js || vs ?? thread is already fixed (uses ??)
Auth middleware (5 threads): - When BOT_API_SECRET configured but wrong secret: reject 401 immediately instead of silently falling through to JWT (security + consistency) - Explicit 'Invalid API secret' error message for failed API-secret auth Guild routes (3 + 1 + 1 + 1 threads): - Extract shared hasOAuthGuildPermission() helper to eliminate duplication in isOAuthGuildAdmin/isOAuthGuildModerator - Extract requireGuildPermission() factory to eliminate duplication in requireGuildAdmin/requireGuildModerator middleware - Add MANAGE_GUILD (0x20) check alongside ADMINISTRATOR (0x8) in OAuth permission checks, aligning API dashboard with Discord slash commands - Add explicit auth method check in GET / guilds route — reject unknown auth methods instead of falling through to full guild list - Warning log for unknown authMethod already added (via factory) Auth routes (2 threads): - Fix DASHBOARD_URL validation: use URL parsing with hostname check to prevent open redirect via http://localhost.evil.com - Remove sessionStore re-export; update all test imports to canonical sessionStore.js path Permissions (3 + 1 threads): - Add Administrator check to isModerator — Administrator is strictly higher privilege and always implies moderator - Add 'moderator' permission level to hasPermission - Enhance isGuildAdmin JSDoc explaining alias intent (Issue #71) - Change modlog config from 'admin' to 'moderator' so the isModerator check in modlog.js is reachable for ManageGuild-only users Session store (2 threads): - Expand JSDoc with scaling limitations, migration path, Map override gaps - Override delete() for consistency OAuth middleware (1 thread): - Add return before next() for consistency Documentation (2 threads): - Document botOwners config in .env.example for fork deployers - Add botOwners and moderator level to README permissions table
- Differentiate requireGuildAdmin (ADMINISTRATOR only) from requireGuildModerator (ADMINISTRATOR | MANAGE_GUILD), aligning REST admin check with slash-command isAdmin (#1, #2, #12) - Add botOwners startup warning when using default upstream ID (#3) - Add SESSION_SECRET, DISCORD_CLIENT_SECRET, DISCORD_REDIRECT_URI to README deployment table (#4) - Pass actual permission level to getPermissionError so modlog denial says 'moderator' not 'administrator' (#5) - Guard _seedOAuthState with NODE_ENV production check (#6) - Add test: valid JWT with no server-side session (#7) - Add DiscordApiError class with HTTP status (#8) - Add moderatorRoleId support to isModerator (#9) - Remove no-op delete override from SessionStore (#10) - Cap oauthStates at 10k entries (#11) - Fix hasOAuthGuildPermission docstring for bitwise OR semantics (#12) - Handle dashboard URL fragment collision (#13) - Cap guildCache at 10k entries (#14) - Add SESSION_TTL_MS co-location comment with JWT expiry (#15) - Cache SESSION_SECRET via lazy getter in verifyJwt (#16) - Remove PII (username) from OAuth auth log (#17)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| return null; | ||
| } | ||
| return authHeader.slice(7); | ||
| } |
There was a problem hiding this comment.
Exported function never imported outside its module
Low Severity
getBearerToken is exported from oauthJwt.js but is never imported anywhere else in the codebase — not in production code nor in tests. It's only called internally by handleOAuthJwt within the same file. The export keyword unnecessarily expands the module's public API.
1 participant
Closes #72
Summary
Adds guild-level authentication and authorization to bills-bot. Previously, the
/configcommand had no permission checks, and the REST API used only a shared secret with no concept of user identity or guild admin rights.Changes
Permission Utilities (
src/utils/permissions.js)BOT_OWNER_IDconstant (191633014441115648) — always bypasses all permission checksisAdmin()andhasPermission()functionsisGuildAdmin(member, config)— checks ADMINISTRATOR permission or configured bot admin roleisModerator(member, config)— checks MANAGE_GUILD permission or configured bot admin roleSlash Command Permission Checks
/config— now requires guild admin (isGuildAdmin) at start of execute()/modlog— now requires moderator (isModerator) at start of execute()REST API Authentication
x-api-secretheader) and OAuth2 JWT (Authorization: Bearer) worksrc/api/routes/auth.js):GET /api/v1/auth/discord— redirect to Discord OAuth2GET /api/v1/auth/discord/callback— exchange code, create JWTGET /api/v1/auth/me— return current user infoPOST /api/v1/auth/logout— placeholder (stateless JWT)src/api/middleware/oauth.js) — verifies Bearer tokensGET /api/v1/guildsreturns guilds filtered by user admin perms + bot presenceConfig
botOwnerstopermissionssection in config.jsonDependencies
jsonwebtokenfor JWT signing/verificationTests
tests/api/routes/auth.test.js,tests/api/middleware/oauth.test.jsNew Environment Variables
DISCORD_CLIENT_ID— Discord app client IDDISCORD_CLIENT_SECRET— Discord app client secretDISCORD_REDIRECT_URI— OAuth2 callback URLSESSION_SECRET— JWT signing secret