feat/b2b-teams #15
feat/b2b-teams #15
Conversation
📝 WalkthroughWalkthroughAdds Phase 3 B2B & Teams: team management and invitations, Slack integration (OAuth, signature verification, message posting), Conflict Radar for simultaneous file editing via Redis, Prisma models for teams/invitations/slack workspaces, route handlers, WebSocket and shared payload changes, config/env and crypto utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Browser (owner/admin)
participant Server as App Server
participant Slack as Slack OAuth
participant DB as Database
participant Redis as Redis
User->>Server: GET /slack/install?teamId=X
Server->>DB: Verify team exists & permissions
Server->>Redis: Store CSRF state token
Server->>User: Set secure cookie, redirect to Slack (install URL)
User->>Slack: Authorize app (client_id, state)
Slack->>Server: GET /slack/callback?code=...&state=...
Server->>Redis: Verify CSRF state token
Server->>Slack: Exchange code for tokens
Slack->>Server: Return access tokens & workspace info
Server->>DB: Persist SlackWorkspace (encrypted tokens)
Server->>User: Redirect to dashboard / return JSON (dev)
sequenceDiagram
participant Client as WebSocket Client
participant Server as App Server
participant DB as Database
participant Redis as Redis/Conflict Radar
Client->>Server: Connect + authenticate
Server->>DB: Lookup user's primary team
Server->>Server: Set ws.teamId
Client->>Server: Send Activity (fileHash, teamId)
Server->>Redis: checkConflicts(userId, teamId, fileHash)
Redis-->>Server: { hasConflict, editors[] }
alt hasConflict
Server->>AllClients: Broadcast CONFLICT_ALERT (fileHash, editors, teamId)
else no conflict
Server->>Redis: Add user to file editors set (sliding TTL)
end
Client->>Server: Disconnect
Server->>Redis: clearAllUserEditing(userId, teamId)
sequenceDiagram
participant Owner as Team Owner
participant Server as App Server
participant DB as Database
participant Invitee as Invitee (email)
Owner->>Server: POST /teams/:id/invite (email, role)
Server->>Server: Validate permissions, generate token & hash
Server->>DB: Create TeamInvitation (PENDING, expiration)
Server->>Owner: Return invite link (token)
Invitee->>Server: GET /teams/join?token=...
Server->>DB: Validate token & expiration
alt valid
Server->>DB: Add member to Team, set invitation ACCEPTED
Server->>Invitee: Success (joined)
else invalid/expired
Server->>Invitee: 400 Invalid or expired
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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: 19
🤖 Fix all issues with AI agents
In @apps/server/package.json:
- Around line 31-32: The package versions for Slack libraries are invalid;
update the dependency entries for "@slack/types" and "@slack/web-api" to valid
npm versions by changing "@slack/types": "^2.19.0" -> "^2.16.0" and
"@slack/web-api": "^7.13.0" -> "^7.12.0" so npm can resolve them; ensure you run
an install afterwards to verify and update lockfile.
In @apps/server/prisma/schema.prisma:
- Around line 175-195: Add a compound index to the TeamInvitation model to
support lookups by teamId, email, and status: update the TeamInvitation model
(where fields like teamId, email, status, token are defined) to include a new
@@index([teamId, email, status]) entry so queries in the /invite route can use
that composite index for efficient lookups.
In @apps/server/src/integrations/slack.ts:
- Around line 398-406: The timestamp in the Slack message is generated with new
Date().toLocaleTimeString(), which varies by server locale; update the
blocks.push(...) mrkdwn text to use a deterministic timezone-aware format
instead — either use a UTC ISO/time portion (e.g., Date.toISOString() or an
ISO-derived time) or format via toLocaleTimeString with an explicit timeZone
option driven by a configurable setting (e.g., app config/ENV) so deployments
produce consistent timestamps; locate the blocks.push call that builds the
context element containing `_Updated: ${new Date().toLocaleTimeString()}_` and
replace it with the deterministic/timezone-configurable formatter.
- Around line 155-207: Before calling db.slackWorkspace.upsert in
handleSlackOAuthCallback, verify the Team exists for the provided teamId by
querying db.team.findUnique (or findFirst) with that teamId; if no Team is
found, throw a clear error (e.g., "Team not found") and avoid performing the
upsert to prevent FK constraint failures. Use the existing getDb() instance (db)
and reference teamId in the check, then only run db.slackWorkspace.upsert when
the team existence check passes.
In @apps/server/src/routes/slack.ts:
- Around line 111-157: The OAuth callback route handler for '/callback' (the
app.get that parses CallbackQuerySchema, verifies request.cookies.slack_csrf and
calls handleSlackOAuthCallback) has no rate limiting; update the route
registration to include a route-level config.rateLimit (e.g., max: 10,
timeWindow: '1 minute') so the endpoint is protected from floods and timing
attacks, and ensure your Fastify/HTTP rate-limit plugin is registered globally
so the per-route config is honored; consider tightening the limit in production
(e.g., 5/min) while allowing a more permissive limit in development.
- Around line 256-285: Move the Slack URL verification response after signature
validation: in the app.post('/events') handler, first ensure
env.SLACK_SIGNING_SECRET exists, extract x-slack-request-timestamp and
x-slack-signature, compute bodyString and call
verifySlackRequest(env.SLACK_SIGNING_SECRET, timestamp, bodyString, signature);
only after a successful verification should you check body.type ===
'url_verification' and return the challenge. Keep the same error handling
(ForbiddenError for missing secret/headers or invalid signature) and retain
logger.debug and the 200 acknowledgement behavior for valid requests.
- Around line 159-182: The current /commands route reconstructs the Slack
request body via URLSearchParams into bodyString which can change
ordering/encoding and break verifySlackRequest; instead preserve and use the raw
request body for signature verification by configuring Fastify to keep the raw
body for application/x-www-form-urlencoded (so request.rawBody or equivalent
contains the original bytes) and pass that raw string into verifySlackRequest
(replace usage of bodyString in the app.post('/commands' ...) handler and ensure
the content-type parser returns the raw string for the route).
- Around line 60-108: The handler uses crypto.randomUUID() (in the '/install'
route) but the crypto module isn't imported; add an import for the Node crypto
module at the top of the file (e.g., import crypto from 'crypto'; or import {
randomUUID } from 'crypto' and update usage) so crypto.randomUUID() resolves
correctly and TypeScript/ESM builds succeed, ensuring any tsconfig/node type
settings support the chosen import style.
In @apps/server/src/routes/teams.ts:
- Around line 510-524: The response currently includes the raw invitation.token
in the reply (the object returned from reply.status(201).send), which must not
be exposed in production; update the send payload to include token only when
running in a non-production environment or when a dedicated feature flag is
enabled (e.g., process.env.NODE_ENV === 'development' or
config.showInvitationTokenForTests). Concretely, modify the response
construction around the return reply.status(201).send call that uses invitation
to conditionally add the token field (keep all other fields unchanged) and
ensure the default behavior in production omits the token entirely.
- Around line 143-210: The pagination is incorrectly applied to ownedTeams and
memberships separately; remove skip/take from the db.team.findMany and
db.teamMember.findMany calls in the app.get('/me' ...) handler so you fetch the
full candidate sets (or a sufficiently large window), merge them into a single
teams array, dedupe by team id (handle cases where owner also appears as
member), map to the unified shape (use owner role for owner entries), sort the
merged list by createdAt desc, then apply pagination by slicing using skip and
limit and compute total from the deduped full list (update hasMore accordingly);
also adjust ownedCount/memberCount usage to reflect deduped total.
In @apps/server/src/ws/handler.ts:
- Around line 324-327: The close handler currently awaits
clearAllUserEditing(ws.userId, ws.teamId), which can block the unsubscribe path
(unsubscribeFromFriends) if Redis is slow; change the call to fire-and-forget
(e.g., use void clearAllUserEditing(...) or otherwise start it without awaiting)
so cleanup is best-effort and does not delay unsubscribeFromFriends(ws), keeping
the existing error handling/logging inside clearAllUserEditing.
- Around line 374-393: Load the user's tier via prisma.user.findUnique
(selecting tier) before applying the existing team lookup logic in the WS
connection setup; only run the teamMember.findFirst / team.findFirst checks and
assign ws.teamId when the retrieved user.tier === 'TEAM' so conflict radar is
enabled strictly for TEAM-tier users (keep using getDb(), teamMember.findFirst
and team.findFirst as the existing selectors).
- Around line 200-220: The new 'CONFLICT_ALERT' message is used but not declared
in the OutboundMessageType/WsMessage types, causing send() to cast a string and
lose type safety; add 'CONFLICT_ALERT' to the OutboundMessageType union, define
a ConflictAlertPayload type (e.g., { fileHash: string; editors: string[];
teamId: string }), extend WsMessage to accept this payload for type ===
'CONFLICT_ALERT', and update the send() signature (or its overloads) to accept
typed message types so calls like send(editorWs, 'CONFLICT_ALERT', { fileHash,
editors: conflictResult.editors, teamId: ws.teamId }) are fully typed without
casting.
- Around line 200-220: The readyState checks use the instance property (e.g.,
editorWs.OPEN or ws.OPEN) which is undefined; import the WebSocket value from
'ws' (e.g., "import WebSocket from 'ws'") and replace all instance-based
comparisons with the static constant WebSocket.OPEN (e.g., change editorWs.OPEN
-> WebSocket.OPEN and any ws.OPEN checks similarly) in the four places in this
file (the CONFLICT_ALERT loop uses connections, editorWs and send; update those
and the other three readyState comparisons to use WebSocket.OPEN).
In @apps/server/src/ws/types.ts:
- Around line 16-18: The optional field teamId in the WebSocket types is
ambiguous about intent; rename it to primaryTeamId in the type definition (and
update the JSDoc comment to “User's primary team ID for conflict radar (Phase
3)”) and then update all usages, serializers/deserializers, and payload handling
to reference primaryTeamId instead of teamId; to avoid breaking clients, add a
small compatibility mapping when parsing incoming payloads (if teamId exists,
copy it to primaryTeamId) and mark teamId as deprecated in comments.
In @packages/shared/src/types.ts:
- Around line 52-55: The comment for the fileHash field is inconsistent with the
contract; change the documentation for fileHash in packages/shared/src/types.ts
to state "full SHA256 hex (64 chars)" and then make matching changes in any
validator and server code that parses/validates fileHash (update validation
regex/length checks and any schema/types that reference fileHash) so all places
expect and enforce a 64-character hex SHA256 rather than 16 chars; ensure
unit/contract tests are updated accordingly.
In @packages/shared/src/validators.ts:
- Around line 33-36: Tighten the validators for fileHash and teamId in
validators.ts: update fileHash to be an optional string that must be exactly 64
hex characters (e.g., z.string().length(64).regex(/^[0-9a-f]{64}$/i).optional())
to enforce SHA256 format and reject empty strings, and update teamId to be an
optional CUID (e.g., z.string().cuid().optional()) so it matches Prisma's
Team.id and disallows empty strings.
📜 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 (17)
.env.exampleapps/extension/CHANGELOG.mdapps/extension/package.jsonapps/server/package.jsonapps/server/prisma/schema.prismaapps/server/src/config.tsapps/server/src/integrations/slack.tsapps/server/src/lib/errors.tsapps/server/src/routes/slack.tsapps/server/src/routes/teams.tsapps/server/src/routes/webhooks.tsapps/server/src/server.tsapps/server/src/services/conflictRadar.tsapps/server/src/ws/handler.tsapps/server/src/ws/types.tspackages/shared/src/types.tspackages/shared/src/validators.ts
💤 Files with no reviewable changes (1)
- apps/server/src/routes/webhooks.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/server/src/routes/teams.ts (4)
apps/server/src/services/db.ts (1)
getDb(20-33)apps/server/src/lib/errors.ts (5)
NotFoundError(148-156)ForbiddenError(141-141)ValidationError(78-97)AuthenticationError(113-121)ConflictError(163-174)packages/shared/src/validators.ts (1)
PaginationQuerySchema(68-71)apps/server/src/lib/logger.ts (1)
logger(68-68)
apps/server/src/integrations/slack.ts (4)
apps/server/src/lib/logger.ts (1)
logger(68-68)apps/server/src/config.ts (1)
env(81-81)apps/server/src/services/db.ts (1)
getDb(20-33)apps/server/src/services/redis.ts (1)
getPresences(212-237)
apps/server/src/routes/slack.ts (1)
apps/server/src/integrations/slack.ts (5)
getSlackInstallUrl(119-132)handleSlackOAuthCallback(155-207)verifySlackRequest(88-109)getTeamStatus(236-317)formatStatusForSlack(339-409)
apps/server/src/ws/handler.ts (2)
apps/server/src/services/conflictRadar.ts (2)
checkConflicts(40-86)clearAllUserEditing(151-176)apps/server/src/services/db.ts (1)
getDb(20-33)
🪛 Biome (2.1.2)
apps/server/src/lib/errors.ts
[error] 49-49: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 GitHub Check: CodeQL
apps/server/src/routes/slack.ts
[failure] 111-157: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 160-253: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 256-285: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
🔇 Additional comments (48)
apps/extension/package.json (1)
5-5: LGTM! Version bump aligns with new features.The minor version increment from 0.3.0 to 0.4.0 correctly follows semantic versioning for the new B2B team features, Slack integration, and Conflict Radar functionality.
apps/extension/CHANGELOG.md (1)
8-30: LGTM! Comprehensive and well-structured changelog entry.The changelog entry clearly documents the new B2B features, breaking changes to
ActivityPayload, and technical implementation details. The format follows Keep a Changelog conventions.apps/server/src/lib/errors.ts (4)
1-69: LGTM! Documentation improvements enhance clarity.The enhanced documentation clearly explains the error hierarchy, operational vs programming errors, and API serialization behavior. These improvements will aid future maintainability.
138-141: LGTM! ForbiddenError alias improves developer experience.The alias provides more intuitive naming for 403 errors while maintaining backward compatibility with the existing
AuthorizationErrorclass.
32-32: LGTM! Redundant type annotations cleaned up.Removing
| undefinedfrom optional properties improves readability without changing behavior, as TypeScript automatically treats optional properties as potentially undefined.Also applies to: 187-187
44-53: Static analysis false positive can be safely ignored.The Biome warning about shadowing the "constructor" property at line 49 is a false positive. This is a type annotation for the native V8
Error.captureStackTraceAPI, not a variable declaration. The parameter name matches the native API signature and cannot be changed..env.example (1)
32-36: LGTM! Slack environment variables properly documented.The Slack integration variables are well-documented with helpful comments. The "Optional" label correctly indicates these are not required for basic functionality. Ensure developers populate these in their local
.envfiles (not committed to version control) when enabling Slack integration.apps/server/src/config.ts (1)
85-87:isDevelopment/isTestexports are a good addition.apps/server/src/ws/handler.ts (1)
31-32: Conflict radar imports look fine; ensure they’re non-blocking in hot paths.apps/server/prisma/schema.prisma (3)
35-37: LGTM!The
sentTeamInvitationsrelation is correctly defined with a named relation to avoid ambiguity with other User relations.
123-126: LGTM!The Team model extensions are well-structured with proper optional one-to-one relation for SlackWorkspace.
164-169: LGTM!The
InvitationStatusenum covers the complete invitation lifecycle.apps/server/src/routes/teams.ts (13)
1-24: LGTM!Imports are well-organized and all dependencies are appropriately sourced from the project's error handling and service layers.
29-62: LGTM!Validation schemas are well-defined with appropriate constraints. The slug regex correctly enforces URL-safe characters.
68-71: LGTM!Using
crypto.randomBytes(32)provides 256 bits of entropy, which is cryptographically secure for invitation tokens.
74-104: LGTM!The helper correctly checks ownership first, then membership, and properly handles the
OWNERrole stored in TeamMember.
107-132: LGTM!Clean implementation that correctly treats the owner as an implicit member.
213-277: LGTM!Team creation logic is solid with proper tier checking, slug uniqueness validation, and appropriate logging.
280-348: LGTM!Team details endpoint correctly checks membership before returning data and includes all relevant nested information.
351-392: LGTM!Update logic correctly restricts to admins and handles the case where no updates are provided.
395-422: LGTM!Delete is correctly restricted to owner only with appropriate cascading handled by the schema.
560-574: Token-based join allows any authenticated user to accept an invitation.The comment on lines 564-565 notes that email verification could be added. Currently, any user with the token can join regardless of whether the invitation was meant for their email. This could be intentional (shareable invite links), but if invitations should be email-specific, verification should be enforced.
Consider whether this is the intended behavior. If invitations should be restricted to the specific email address, add verification:
const user = await db.user.findUnique({ where: { id: userId }, select: { email: true } }); if (user?.email && user.email !== invitation.email) { throw new ForbiddenError('This invitation was sent to a different email address'); }
607-668: LGTM!Role update logic correctly restricts to owner-only and prevents self-modification.
671-740: LGTM!Member removal has comprehensive permission checks: owner can remove anyone, admins can remove non-admins, users can remove themselves, and owner cannot be removed.
743-829: LGTM!Invitation listing and revocation endpoints are properly protected with admin checks.
apps/server/src/routes/slack.ts (4)
1-22: LGTM!Imports are well-structured with proper type imports and service layer dependencies.
27-49: LGTM!Validation schemas and type definitions are appropriate for Slack integration.
288-327: LGTM!Status endpoint correctly requires authentication and returns appropriate connection information.
329-370: LGTM!Disconnect endpoint properly restricts to team owner only and cleans up the workspace connection.
apps/server/src/services/conflictRadar.ts (6)
1-30: LGTM!Well-documented module with clear interface definitions and appropriate constants.
40-86: LGTM!The conflict detection logic is sound:
- Checks existing editors before adding user
- Uses sliding window TTL for automatic cleanup
- Returns comprehensive result with all editors
- Gracefully handles errors with safe fallback
94-118: LGTM!Publishing to individual presence channels ensures targeted delivery and the
Promise.allwith individual error handling prevents one failure from blocking others.
128-142: LGTM!Simple and effective cleanup with appropriate error logging.
151-176: LGTM!The SCAN-based cleanup is correctly implemented:
- Uses cursor-based iteration to avoid blocking
- Pipelines
SREMoperations for efficiency- COUNT 100 is a reasonable batch size
- Gracefully handles errors
185-195: LGTM!Clean utility function with appropriate error handling.
apps/server/src/integrations/slack.ts (7)
1-24: LGTM!Well-documented module header and appropriate imports from internal services and Slack SDK.
25-65: LGTM!Type definitions are comprehensive and well-structured for Slack OAuth responses and team status.
88-109: LGTM!Excellent implementation of Slack request verification:
- Replay attack protection via timestamp validation
- Timing-safe comparison prevents timing attacks
- Proper error handling for buffer length mismatches
119-145: LGTM!OAuth URL generation is correctly implemented with proper state serialization for CSRF protection.
215-228: LGTM!Clean client factory that handles the case where no workspace is connected.
236-317: LGTM!Team status aggregation is well-implemented with proper presence lookup and categorization.
420-475: LGTM!Message posting functions are well-implemented with proper error handling and logging.
apps/server/src/server.ts (6)
1-39: LGTM!Well-structured imports with new route modules properly integrated. Updated documentation accurately describes the server architecture.
40-97: LGTM!Server configuration is well-tuned:
- Rate limiting appropriately adjusted for production vs development
- CORS configuration is secure with proper domain restrictions
- WebSocket max payload of 64KB is reasonable for presence updates
98-117: LGTM!Token blacklist verification is an excellent security addition that properly handles revoked tokens.
119-189: LGTM!Request logging, error handling, and health check are well-implemented with appropriate operational details.
191-211: LGTM!Route registration is correctly structured:
- Team routes under versioned API (
/api/v1/teams)- Slack routes at root level (
/slack) - appropriate for OAuth callback URL stability
213-290: LGTM!Server lifecycle management is robust with:
- Proper service connection ordering
- Graceful shutdown with 30-second timeout
- Comprehensive process signal handling
| /** User's primary team ID for conflict radar (Phase 3) */ | ||
| teamId?: string; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider naming/semantics: teamId is “primary team”, not “current team”.
If users can belong to multiple teams, teamId?: string can be ambiguous at call sites; consider primaryTeamId?: string (or documenting selection rules in this type) to prevent accidental misuse.
🤖 Prompt for AI Agents
In @apps/server/src/ws/types.ts around lines 16 - 18, The optional field teamId
in the WebSocket types is ambiguous about intent; rename it to primaryTeamId in
the type definition (and update the JSDoc comment to “User's primary team ID for
conflict radar (Phase 3)”) and then update all usages,
serializers/deserializers, and payload handling to reference primaryTeamId
instead of teamId; to avoid breaking clients, add a small compatibility mapping
when parsing incoming payloads (if teamId exists, copy it to primaryTeamId) and
mark teamId as deprecated in comments.
packages/shared/src/types.ts
Outdated
| /** SHA256 hash of project_root + relative_path (first 16 chars) for conflict detection */ | ||
| fileHash?: string; | ||
| /** Team ID for conflict radar (required for TEAM tier users) */ | ||
| teamId?: string; |
There was a problem hiding this comment.
Fix spec mismatch: “first 16 chars” vs max-64/sha256.
Either update the comment to “full SHA256 hex (64 chars)” or change the contract to 16 chars everywhere (types + validators + server). Right now it’s ambiguous and will cause inconsistent client implementations.
🤖 Prompt for AI Agents
In @packages/shared/src/types.ts around lines 52 - 55, The comment for the
fileHash field is inconsistent with the contract; change the documentation for
fileHash in packages/shared/src/types.ts to state "full SHA256 hex (64 chars)"
and then make matching changes in any validator and server code that
parses/validates fileHash (update validation regex/length checks and any
schema/types that reference fileHash) so all places expect and enforce a
64-character hex SHA256 rather than 16 chars; ensure unit/contract tests are
updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Fix all issues with AI agents
In @apps/server/src/integrations/slack.ts:
- Around line 166-176: The code casts the result of client.oauth.v2.access to
SlackOAuthResponse and then uses fields like response.team.id without validating
the structure; add runtime validation (e.g., create a SlackOAuthResponseSchema
using zod or perform explicit checks) to validate ok, access_token, and team.id
before using them, log validation errors (logger.error) and throw the existing
error when validation fails; update the code around client.oauth.v2.access /
SlackOAuthResponse to parse/validate the response into a safe variable (e.g.,
validResponse) and then reference validResponse.team.id instead of the
unvalidated response.
- Around line 140-146: The getSlackRedirectUri function currently hardcodes the
production base URL; update it to use a configurable API base URL from your
config (e.g., API_BASE_URL) falling back to the existing logic for
non-production or missing config. Add API_BASE_URL to your config schema (as
suggested) and then modify getSlackRedirectUri to read config.API_BASE_URL when
present (otherwise use env.NODE_ENV/PORT logic) so the redirect URI becomes
`${baseUrl}/slack/callback`; ensure the symbol names referenced are
config.API_BASE_URL and function getSlackRedirectUri.
- Around line 276-280: The combined arrays allMemberIds and allMembers can
include the owner twice if team.owner also exists in team.members; update the
construction of allMemberIds and allMembers to deduplicate by user id (use
team.owner.id and members' user.id as keys), e.g., build a Set of seen IDs or a
Map keyed by id and only add unique entries so both allMemberIds and allMembers
contain each user once (refer to variables allMemberIds, allMembers, team.owner,
and team.members).
In @apps/server/src/lib/crypto.ts:
- Around line 20-26: The current encrypt(text: string) uses synchronous cipher
ops which can block the event loop for large inputs; add an async alternative
(e.g., encryptAsync) that creates the iv and cipher via crypto.createCipheriv
and streams the plaintext through the cipher using a Readable stream + pipeline
(or pipe to a concat/collecting Writable) to produce the encrypted hex, then
return iv:encrypted; keep the existing encrypt for small payloads and expose
encryptAsync for callers that may handle large strings.
- Around line 8-12: Replace use of env.JWT_SECRET for encryption key derivation
with a dedicated ENCRYPTION_KEY: update your config schema to require
ENCRYPTION_KEY (e.g., add ENCRYPTION_KEY: z.string().min(32, 'ENCRYPTION_KEY
must be at least 32 characters') in apps/server/src/config.ts and .env.example),
then change the SECRET_KEY constant in apps/server/src/lib/crypto.ts to derive
from env.ENCRYPTION_KEY
(crypto.createHash('sha256').update(env.ENCRYPTION_KEY).digest()) instead of
env.JWT_SECRET; do not fall back to JWT_SECRET to enforce key separation.
- Around line 35-52: The decrypt function lacks input validation for empty or
whitespace-only strings; update decrypt to first check that the incoming text is
a non-empty, non-whitespace string and throw a clear Error (e.g., "Invalid
encrypted text: empty input") if it fails, and also validate that after
splitting the IV (ivHex) the remaining encryptedText is not empty before
attempting to create the decipher; reference the decrypt function and its local
values ivHex and encryptedText when adding these guards so you avoid calling
crypto.createDecipheriv/decipher.update on invalid input.
In @apps/server/src/routes/slack.ts:
- Around line 299-309: The signature verification uses
JSON.stringify(request.body) which can differ from the raw payload; update the
slackRoutes setup to preserve the raw JSON string for verification by adding a
JSON content-type parser (similar to the existing form-urlencoded parser) that
parses as string, then in the /events handler use that raw string (cast
request.body to string) to call verifySlackRequest(env.SLACK_SIGNING_SECRET,
timestamp, bodyString, signature), and only JSON.parse the raw string afterwards
for event handling; reference addContentTypeParser, slackRoutes, the /events
handler, verifySlackRequest, and request.body when making this change.
- Around line 399-402: Deleting a Slack workspace with db.slackWorkspace.delete
can throw if no record exists for the given teamId; change the logic to use
db.slackWorkspace.deleteMany({ where: { teamId } }) or check existence first
(e.g., findUnique/findFirst on db.slackWorkspace with teamId) and only call
delete when present so the endpoint handles missing workspaces gracefully and
avoids Prisma throw; update any surrounding error handling to reflect the
non-throwing deleteMany behavior if chosen.
- Around line 141-145: The CSRF check uses a regular !== comparison between
csrfCookie and state.csrf; replace it with a timing-safe comparison by
converting both values to Buffers and using crypto.timingSafeEqual, but first
ensure equal byte length (if lengths differ, treat as non-match without calling
timingSafeEqual to avoid exceptions). Update the check around
csrfCookie/state.csrf in slack.ts to coerce undefined to empty string, create
Buffer.from(csrfCookie) and Buffer.from(state.csrf), verify lengths match and
then call crypto.timingSafeEqual to decide whether to throw the ForbiddenError.
In @apps/server/src/routes/teams.ts:
- Around line 154-221: The current logic collects ownedTeams and memberships,
merges into teamMap, builds allTeams and performs in-memory sorting and
pagination (variables: ownedTeams, memberships, teamMap, allTeams, skip, limit),
which won't scale for large team counts; change to perform pagination at the DB
level by either (A) issuing a single query that returns the union of owned and
member teams with proper role resolution and ORDER BY/OFFSET LIMIT, or (B) first
query for paginated team IDs (using UNION or two queries with LIMIT/OFFSET and
de-duplication via SQL), then fetch the full team details for that paginated ID
set—replace the in-memory dedupe/sort/paginate block (where teamMap/allTeams are
used) with the DB-driven approach to ensure scalable sorting and pagination.
- Around line 99-104: The role check in checkTeamAdmin unnecessarily includes
'OWNER' even though owners are represented by team.ownerId and already handled
earlier; update the isAdmin calculation to only check membership.role ===
'ADMIN' (remove membership.role === 'OWNER') and keep returning { isOwner,
isAdmin } so owner status continues to come from the team.ownerId logic.
- Around line 197-209: The code in the memberships.forEach block uses m.joinedAt
for createdAt which can cause inconsistent sorting; change the createdAt
assignment to use the team's actual created timestamp (m.team.createdAt) instead
of the membership joined timestamp (m.joinedAt) when setting the map entry (keep
the OWNER precedence logic intact in the teamMap.set call).
In @apps/server/src/ws/handler.ts:
- Around line 375-402: The current WebSocket connection setup makes three
sequential Prisma queries (getDb() then prisma.user.findUnique then
prisma.teamMember.findFirst possibly followed by prisma.team.findFirst), which
increases latency; refactor the team assignment in handler.ts so after
confirming user?.tier === 'TEAM' you fetch teamMembership and ownedTeam
concurrently (e.g., Promise.all) or use a single include/raw query and then set
ws.teamId = teamMembership?.teamId ?? ownedTeam?.id; keep getDb() and the
initial user tier check but collapse the subsequent teamMember and team lookups
into one concurrent or consolidated query to avoid sequential waits.
In @packages/shared/src/validators.ts:
- Around line 33-34: Remove the duplicated comment "SHA256 hash of file path for
conflict detection (Phase 3)" so it only appears once; locate the repeated
comment lines in packages/shared/src/validators.ts (the two identical comment
lines) and delete one of them so there is a single, non-duplicated comment.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
apps/server/package.jsonapps/server/prisma/schema.prismaapps/server/src/config.tsapps/server/src/integrations/slack.tsapps/server/src/lib/crypto.tsapps/server/src/routes/slack.tsapps/server/src/routes/teams.tsapps/server/src/ws/handler.tspackages/shared/src/types.tspackages/shared/src/validators.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/server/src/routes/slack.ts (1)
apps/server/src/integrations/slack.ts (5)
getSlackInstallUrl(120-133)handleSlackOAuthCallback(156-218)verifySlackRequest(89-110)getTeamStatus(253-334)formatStatusForSlack(356-426)
apps/server/src/ws/handler.ts (2)
apps/server/src/services/conflictRadar.ts (2)
checkConflicts(40-86)clearAllUserEditing(151-176)apps/server/src/services/db.ts (1)
getDb(20-33)
apps/server/src/integrations/slack.ts (5)
apps/server/src/lib/logger.ts (1)
logger(68-68)apps/server/src/config.ts (1)
env(95-95)apps/server/src/services/db.ts (1)
getDb(20-33)apps/server/src/lib/crypto.ts (2)
encrypt(20-26)decrypt(35-52)apps/server/src/services/redis.ts (1)
getPresences(212-237)
apps/server/src/lib/crypto.ts (1)
apps/server/src/config.ts (1)
env(95-95)
🪛 GitHub Actions: CI
apps/server/package.json
[error] 1-1: pnpm install --frozen-lockfile failed: lockfile (pnpm-lock.yaml) is not up to date with apps/server/package.json. Cannot install with frozen-lockfile.
🔇 Additional comments (17)
apps/server/prisma/schema.prisma (2)
191-196: LGTM: Well-designed indexes and cascade deletions.The indexes on
TeamInvitation(including the composite index onteamId, email, status) andSlackWorkspaceare appropriate for the expected query patterns. The cascade deletions ensure proper cleanup when teams are deleted.Also applies to: 212-213
199-213: TheaccessTokenfield is already properly encrypted in the application code. Theencrypt()function is applied when creating/updating SlackWorkspace records (apps/server/src/integrations/slack.ts, lines 194 and 200), and thedecrypt()function is used when retrieving the token (line 239). Error handling is in place for decryption failures. No action needed.packages/shared/src/validators.ts (1)
35-41: LGTM: Correct validation for SHA256 hash and team ID.The validation logic is well-designed:
fileHashcorrectly validates 64-character hexadecimal strings (SHA256 output)teamIduses.cuid()validation matching the Prisma schema- Both fields appropriately optional for backward compatibility
packages/shared/src/types.ts (1)
52-55: LGTM: ActivityPayload extensions align with conflict detection requirements.The new
fileHashandteamIdfields are well-documented and appropriately optional. The comments clearly explain their purpose for conflict detection in Phase 3 B2B features, and the types align with the validation schemas invalidators.ts.apps/server/package.json (1)
31-32: Remove the lockfile update requirement—the pnpm-lock.yaml already contains the Slack dependencies.The Slack packages are already present in pnpm-lock.yaml and resolved to valid versions (@slack/types@2.19.0 and @slack/web-api@7.13.0) that satisfy the caret ranges specified in package.json. These versions have no known security vulnerabilities and are production-ready. No additional lockfile updates are needed.
Likely an incorrect or invalid review comment.
apps/server/src/config.ts (2)
14-68: LGTM! Well-structured Slack configuration validation.The cross-field validation using
.superRefine()correctly enforces the all-or-none constraint for Slack configuration variables. The schema is comprehensive with appropriate defaults and validation rules.
97-101: LGTM! Useful environment flag exports.The addition of
isDevelopmentandisTestalongsideisProductionprovides a consistent and type-safe way to check the runtime environment throughout the codebase.apps/server/src/ws/handler.ts (3)
60-60: LGTM! Correct WebSocket state check.Using
WebSocket.OPEN(the static class constant) instead of an instance property is the correct approach for comparingreadyState.
201-221: Conflict detection logic is sound but consider edge cases.The conflict radar integration correctly checks for file conflicts when a user is in a team and has a
fileHash. BroadcastingCONFLICT_ALERTto all editors is appropriate.However, consider what happens if
checkConflictsthrows an error—it would propagate and potentially fail the entire status update. SincecheckConflictsalready handles errors internally (returninghasConflict: false), this should be safe, but you may want to wrap this in a try-catch for defense-in-depth.
325-328: Verify timing of editing state cleanup.The
clearAllUserEditingcall happens after initiating the grace period timeout. Since the grace period allows 60 seconds for reconnection, clearing the editing state immediately on close means a reconnecting user would lose their editing markers even during the grace window.Consider whether this is the intended behavior, or if the editing state cleanup should also be deferred.
apps/server/src/routes/teams.ts (2)
69-72: LGTM! Secure token generation.Using
crypto.randomBytes(32)provides 256 bits of entropy, which is cryptographically secure for invitation tokens.
541-544: Token exposure in non-production is intentional but should be documented.Exposing the invitation token in development/test environments (line 543) is useful for testing, but consider adding a comment explaining this is intentional to prevent future removal.
apps/server/src/routes/slack.ts (2)
54-60: Content type parser affects all routes in this router.The custom parser for
application/x-www-form-urlencodedreturns the raw string body. This is correct for/commandssignature verification, but be aware this parser is scoped to the Slack routes only (when registered viaapp.register).
96-106: LGTM! Proper CSRF protection for OAuth flow.Using a secure, httpOnly cookie with
sameSite: 'lax'provides good CSRF protection for the OAuth flow. The 10-minute expiry is appropriate.apps/server/src/integrations/slack.ts (3)
89-110: LGTM! Robust Slack request verification.The implementation correctly:
- Validates timestamp freshness (5-minute window)
- Uses
crypto.timingSafeEqualto prevent timing attacks- Handles potential buffer length mismatches with try-catch
342-348: LGTM! Clean duration formatting.The
formatDurationhelper handles seconds, minutes, and hours appropriately with readable output.
450-460: LGTM! Proper error handling for Slack message posting.The function correctly catches errors and returns
nullrather than throwing, allowing callers to handle failures gracefully.
| const allMemberIds = [team.owner.id, ...team.members.map((m) => m.user.id)]; | ||
| const allMembers = [ | ||
| { id: team.owner.id, username: team.owner.username, displayName: team.owner.displayName }, | ||
| ...team.members.map((m) => m.user), | ||
| ]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential duplicate if owner is also in members.
The code concatenates owner and all members into allMembers. If the owner also has a TeamMember record (shouldn't happen normally), they would appear twice in the status.
Based on routes/teams.ts logic, owners don't get TeamMember records, so this is likely fine, but consider adding deduplication for safety.
♻️ Deduplicate member list
- const allMemberIds = [team.owner.id, ...team.members.map((m) => m.user.id)];
- const allMembers = [
- { id: team.owner.id, username: team.owner.username, displayName: team.owner.displayName },
- ...team.members.map((m) => m.user),
- ];
+ const memberMap = new Map<string, { id: string; username: string; displayName: string | null }>();
+ memberMap.set(team.owner.id, {
+ id: team.owner.id,
+ username: team.owner.username,
+ displayName: team.owner.displayName
+ });
+ for (const m of team.members) {
+ if (!memberMap.has(m.user.id)) {
+ memberMap.set(m.user.id, m.user);
+ }
+ }
+ const allMemberIds = Array.from(memberMap.keys());
+ const allMembers = Array.from(memberMap.values());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const allMemberIds = [team.owner.id, ...team.members.map((m) => m.user.id)]; | |
| const allMembers = [ | |
| { id: team.owner.id, username: team.owner.username, displayName: team.owner.displayName }, | |
| ...team.members.map((m) => m.user), | |
| ]; | |
| const memberMap = new Map<string, { id: string; username: string; displayName: string | null }>(); | |
| memberMap.set(team.owner.id, { | |
| id: team.owner.id, | |
| username: team.owner.username, | |
| displayName: team.owner.displayName | |
| }); | |
| for (const m of team.members) { | |
| if (!memberMap.has(m.user.id)) { | |
| memberMap.set(m.user.id, m.user); | |
| } | |
| } | |
| const allMemberIds = Array.from(memberMap.keys()); | |
| const allMembers = Array.from(memberMap.values()); |
🤖 Prompt for AI Agents
In @apps/server/src/integrations/slack.ts around lines 276 - 280, The combined
arrays allMemberIds and allMembers can include the owner twice if team.owner
also exists in team.members; update the construction of allMemberIds and
allMembers to deduplicate by user id (use team.owner.id and members' user.id as
keys), e.g., build a Set of seen IDs or a Map keyed by id and only add unique
entries so both allMemberIds and allMembers contain each user once (refer to
variables allMemberIds, allMembers, team.owner, and team.members).
| memberships.forEach((m) => { | ||
| // Owner role takes precedence if present in both lists | ||
| if (!teamMap.has(m.team.id) || teamMap.get(m.team.id)?.role !== 'OWNER') { | ||
| teamMap.set(m.team.id, { | ||
| id: m.team.id, | ||
| name: m.team.name, | ||
| slug: m.team.slug, | ||
| role: m.role, | ||
| memberCount: m.team._count.members + 1, | ||
| createdAt: new Date(m.joinedAt), | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Membership's createdAt overwrites owned team's createdAt.
When a user is both owner and member of a team (shouldn't happen normally), the membership data would overwrite the owned team entry, but the check at line 199 correctly prevents this for the OWNER role. However, note that m.joinedAt is used as createdAt for memberships, which may cause inconsistent sorting.
🐛 Use team's createdAt consistently
teamMap.set(m.team.id, {
id: m.team.id,
name: m.team.name,
slug: m.team.slug,
role: m.role,
memberCount: m.team._count.members + 1,
- createdAt: new Date(m.joinedAt),
+ createdAt: new Date(m.team.createdAt),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| memberships.forEach((m) => { | |
| // Owner role takes precedence if present in both lists | |
| if (!teamMap.has(m.team.id) || teamMap.get(m.team.id)?.role !== 'OWNER') { | |
| teamMap.set(m.team.id, { | |
| id: m.team.id, | |
| name: m.team.name, | |
| slug: m.team.slug, | |
| role: m.role, | |
| memberCount: m.team._count.members + 1, | |
| createdAt: new Date(m.joinedAt), | |
| }); | |
| } | |
| }); | |
| memberships.forEach((m) => { | |
| // Owner role takes precedence if present in both lists | |
| if (!teamMap.has(m.team.id) || teamMap.get(m.team.id)?.role !== 'OWNER') { | |
| teamMap.set(m.team.id, { | |
| id: m.team.id, | |
| name: m.team.name, | |
| slug: m.team.slug, | |
| role: m.role, | |
| memberCount: m.team._count.members + 1, | |
| createdAt: new Date(m.team.createdAt), | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In @apps/server/src/routes/teams.ts around lines 197 - 209, The code in the
memberships.forEach block uses m.joinedAt for createdAt which can cause
inconsistent sorting; change the createdAt assignment to use the team's actual
created timestamp (m.team.createdAt) instead of the membership joined timestamp
(m.joinedAt) when setting the map entry (keep the OWNER precedence logic intact
in the teamMap.set call).
| // Cannot change owner's role | ||
| if (targetUserId === currentUserId) { | ||
| throw new ConflictError('Cannot change your own role'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Self-role change check may be incorrect for owner.
The check targetUserId === currentUserId at line 655 prevents owners from changing their own role. However, owners don't have a TeamMember record, so this check would only trigger if an owner tries to change their own role as a member, which shouldn't be possible.
This is defensive but the error message "Cannot change your own role" could be confusing. Consider clarifying or removing this check since owners can't be in TeamMember.
| /* Phase 3: Get user's primary team for conflict radar (only for TEAM tier users) */ | ||
| const prisma = getDb(); | ||
|
|
||
| const user = await prisma.user.findUnique({ | ||
| where: { id: userId }, | ||
| select: { tier: true }, | ||
| }); | ||
|
|
||
| if (user?.tier === 'TEAM') { | ||
| const teamMembership = await prisma.teamMember.findFirst({ | ||
| where: { userId }, | ||
| orderBy: { joinedAt: 'asc' }, | ||
| select: { teamId: true }, | ||
| }); | ||
| if (teamMembership) { | ||
| ws.teamId = teamMembership.teamId; | ||
| } else { | ||
| /* Check if user owns a team */ | ||
| const ownedTeam = await prisma.team.findFirst({ | ||
| where: { ownerId: userId }, | ||
| orderBy: { createdAt: 'asc' }, | ||
| select: { id: true }, | ||
| }); | ||
| if (ownedTeam) { | ||
| ws.teamId = ownedTeam.id; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sequential database queries during WebSocket connection setup.
The team assignment logic performs up to 3 sequential database queries (user tier, team membership, owned team). For high connection volumes, this could add latency.
Consider consolidating into a single query using Prisma's relation includes or a raw query, though the current approach is functionally correct.
♻️ Optional: Consolidate queries
if (user?.tier === 'TEAM') {
// Single query to get membership or owned team
const [membership, ownedTeam] = await Promise.all([
prisma.teamMember.findFirst({
where: { userId },
orderBy: { joinedAt: 'asc' },
select: { teamId: true },
}),
prisma.team.findFirst({
where: { ownerId: userId },
orderBy: { createdAt: 'asc' },
select: { id: true },
}),
]);
ws.teamId = membership?.teamId ?? ownedTeam?.id;
}🤖 Prompt for AI Agents
In @apps/server/src/ws/handler.ts around lines 375 - 402, The current WebSocket
connection setup makes three sequential Prisma queries (getDb() then
prisma.user.findUnique then prisma.teamMember.findFirst possibly followed by
prisma.team.findFirst), which increases latency; refactor the team assignment in
handler.ts so after confirming user?.tier === 'TEAM' you fetch teamMembership
and ownedTeam concurrently (e.g., Promise.all) or use a single include/raw query
and then set ws.teamId = teamMembership?.teamId ?? ownedTeam?.id; keep getDb()
and the initial user tier check but collapse the subsequent teamMember and team
lookups into one concurrent or consolidated query to avoid sequential waits.
| /** SHA256 hash of file path for conflict detection (Phase 3) */ | ||
| /** SHA256 hash of file path for conflict detection (Phase 3) */ |
There was a problem hiding this comment.
Remove duplicate comment line.
The comment "SHA256 hash of file path for conflict detection (Phase 3)" appears twice on lines 33-34.
📝 Proposed fix
- /** SHA256 hash of file path for conflict detection (Phase 3) */
/** SHA256 hash of file path for conflict detection (Phase 3) */
fileHash: z
.string()
.length(64)
.regex(/^[0-9a-fA-F]{64}$/)
.optional(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** SHA256 hash of file path for conflict detection (Phase 3) */ | |
| /** SHA256 hash of file path for conflict detection (Phase 3) */ | |
| /** SHA256 hash of file path for conflict detection (Phase 3) */ | |
| fileHash: z | |
| .string() | |
| .length(64) | |
| .regex(/^[0-9a-fA-F]{64}$/) | |
| .optional(), |
🤖 Prompt for AI Agents
In @packages/shared/src/validators.ts around lines 33 - 34, Remove the
duplicated comment "SHA256 hash of file path for conflict detection (Phase 3)"
so it only appears once; locate the repeated comment lines in
packages/shared/src/validators.ts (the two identical comment lines) and delete
one of them so there is a single, non-duplicated comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/server/package.json:
- Around line 31-32: Remove the explicit "@slack/types" dependency from
package.json and bump "@slack/web-api" to the latest recommended version
("@slack/web-api": "^7.13.0"); this lets the web-api package bring in the
correct @slack/types transitively. After editing package.json, run the package
manager (npm/yarn/pnpm) to update the lockfile and install deps, and run the
test/build scripts to ensure no breaking changes from the version bump.
📜 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 (1)
apps/server/package.json
🔇 Additional comments (1)
apps/server/package.json (1)
39-39: Bothwsand@fastify/websocketserve complementary purposes—no redundancy.The
wslibrary provides the underlying WebSocket protocol implementation (theWebSocketclass used inapps/server/src/ws/types.tsandapps/server/src/ws/handler.ts), while@fastify/websocketis a Fastify plugin for integrating WebSocket routes into the Fastify server. Both are necessary and appropriately used. The version8.19.0is current and secure with no known vulnerabilities.Likely an incorrect or invalid review comment.
| "@slack/types": "^2.16.0", | ||
| "@slack/web-api": "^7.12.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking @slack/types latest version ==="
npm view @slack/types@latest version 2>&1 || echo "Failed to fetch"
echo -e "\n=== Checking @slack/types version 2.16.0 ==="
npm view "@slack/types@2.16.0" 2>&1 | head -20
echo -e "\n=== Checking @slack/web-api latest version ==="
npm view @slack/web-api@latest version 2>&1 || echo "Failed to fetch"
echo -e "\n=== Checking @slack/web-api version 7.12.0 ==="
npm view "@slack/web-api@7.12.0" 2>&1 | head -20
echo -e "\n=== Checking npm audit for these packages ==="
npm audit --omit=dev 2>&1 | grep -A 5 -i "@slack" || echo "No vulnerabilities found in audit"Repository: senutpal/devRadar
Length of output: 1659
Consider updating Slack SDK versions and remove redundant @slack/types dependency.
The versions specified are secure (no vulnerabilities detected), but @slack/types@^2.19.0 and @slack/web-api@^7.13.0 are the latest available. Additionally, @slack/types is already a transitive dependency of @slack/web-api@7.12.0 (which requires @slack/types@^2.18.0), so explicitly listing it in package.json is redundant. Consider removing the explicit @slack/types entry and updating @slack/web-api to the latest version, which will pull in the required types.
🤖 Prompt for AI Agents
In @apps/server/package.json around lines 31 - 32, Remove the explicit
"@slack/types" dependency from package.json and bump "@slack/web-api" to the
latest recommended version ("@slack/web-api": "^7.13.0"); this lets the web-api
package bring in the correct @slack/types transitively. After editing
package.json, run the package manager (npm/yarn/pnpm) to update the lockfile and
install deps, and run the test/build scripts to ensure no breaking changes from
the version bump.
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Fix all issues with AI agents
In @apps/server/prisma/schema.prisma:
- Around line 176-196: The TeamInvitation schema contains a redundant index on
tokenHash: since tokenHash is declared @unique, remove the @@index([tokenHash])
line from the TeamInvitation model, save the schema, run prisma format/migrate
to update the schema and regenerate the Prisma client so changes take effect;
target the TeamInvitation model and the tokenHash field when making this edit.
- Around line 199-213: SlackWorkspace.accessToken is documented as "encrypted at
rest" but the codebase must enforce encryption on every write and support
ciphertext versioning/rotation: update the code paths that create or update
SlackWorkspace records (e.g., createSlackWorkspace, updateSlackWorkspace, or
Prisma write middleware like any Prisma $use that intercepts writes) to run
accessToken through your encrypt(...) function before persisting and store
accompanying metadata (ciphertext version or KMS key id, e.g.,
accessTokenVersion or accessTokenKeyId); ensure read paths decrypt the token
with decrypt(...) based on that metadata; add a migration/rotation helper to
re-encrypt existing rows when rotating keys and validate tests cover
create/read/update flows to prove encryption and versioning are used.
- Around line 164-196: The TeamInvitation model can still get duplicate PENDING
rows under concurrent requests; add a DB-level unique constraint on the tuple
(teamId, email, status) in the TeamInvitation model to prevent duplicate pending
invites (referencing model TeamInvitation and fields teamId, email, status),
then run the Prisma migration to apply it and update the invitation creation
code to handle unique constraint violations (catch the unique constraint/db
error on create and convert it into the existing "duplicate pending invitation"
response) so concurrent creates fail gracefully.
In @apps/server/src/integrations/slack.ts:
- Around line 361-367: formatDuration is duplicated across modules (e.g., the
local function in slack.ts and the one in friendsProvider) causing divergence;
extract a single implementation into the shared package (packages/shared) as an
exported utility (e.g., export function formatDuration(seconds: number): string)
and replace the local definitions by importing that shared function in both
places; ensure you keep the canonical behavior (seconds <60 -> "Xs", <3600 ->
"Ym", otherwise "Hh Mm") and update any imports/usages to the new shared symbol.
- Around line 307-314: The code in the for loop over allMembers uses a raw
assertion (presence.status as TeamMemberStatus['status']) when building the
TeamMemberStatus, which can be unsafe; modify the logic in the loop (around
presences.get(member.id) and the status assignment) to validate presence?.status
against the allowed TeamMemberStatus values (e.g., via a small helper
isValidTeamStatus(status) or a Set/array of allowed statuses) and only cast when
valid, otherwise fall back to 'offline' — update the status assignment for
TeamMemberStatus accordingly so no unchecked type assertion is used.
In @apps/server/src/lib/crypto.ts:
- Around line 5-12: The current SECRET_KEY is derived via
crypto.createHash('sha256').update(env.ENCRYPTION_KEY).digest(); replace this
with a proper KDF (e.g., PBKDF2 or scrypt) to derive a 32-byte key from
env.ENCRYPTION_KEY: call crypto.pbkdf2Sync or crypto.scryptSync with a stable
salt (or env.SALT), appropriate iterations (PBKDF2) or N/r/p (scrypt), and
length 32, then use that output as SECRET_KEY while keeping ALGORITHM and
IV_LENGTH unchanged; ensure the salt and KDF parameters are documented and
configurable via environment variables.
- Around line 35-51: encryptAsync collects all cipher chunks into memory before
resolving, so it still loads large payloads entirely into memory; replace or
supplement it with a streaming API: implement a new function (e.g.,
encryptStream or encryptStreamFromReadable) that accepts a Readable input (or
returns a Readable/Transform) and pipes it through
crypto.createCipheriv(ALGORITHM, SECRET_KEY, iv) so data flows through without
buffering everything, and ensure the IV is prepended to the output stream
(hex-encoded) rather than concatenating chunks in memory; keep encryptAsync for
small payloads or adapt it to use the streaming helper internally.
In @apps/server/src/routes/slack.ts:
- Around line 121-127: The CSRF cookie is set with specific options
(reply.setCookie('slack_csrf', csrfToken, { path: '/' , httpOnly, secure,
sameSite, maxAge })); update the corresponding reply.clearCookie('slack_csrf')
calls to pass at least the same path ('/') so the cookie is actually removed in
all clients (and preferably match secure, httpOnly and sameSite attributes as
well); make the same change for the other clearCookie instance referenced around
the later block (lines ~168-170) so both clear operations use matching cookie
options.
- Around line 179-192: The redirect uses a hard-coded production URL; change the
reply.redirect call to build the redirect URL from an environment/config value
(e.g., use env.APP_BASE_URL or a configured BASE_URL) or derive it from the
incoming request, and then append
`/dashboard/team/${state.teamId}/settings?slack=connected`; update the code that
checks env.NODE_ENV and the return reply.redirect(...) invocation so it uses the
configurable base URL instead of `https://devradar.io` (refer to reply.redirect,
env.NODE_ENV, and state.teamId to locate the change).
- Around line 227-233: The code currently assumes payload fields exist after
`const payload = Object.fromEntries(searchParams.entries()) as unknown as
SlashCommandPayload`, so calling `payload.text.trim().toLowerCase()` can throw;
add runtime validation for `payload` and its properties (at minimum ensure
`typeof payload.text === 'string'` and `typeof payload.command === 'string'`)
before using them, and handle missing/invalid values by returning a 400/early
error or defaulting `text` to an empty string; update the logic around
`searchParams`, `payload`, `command`, and `text` to use these checks (or a small
type guard function) so malformed requests no longer cause a 500.
In @apps/server/src/routes/teams.ts:
- Around line 52-56: InviteMemberSchema currently accepts mixed-case emails but
downstream lookups do exact matches, causing duplicate/failed invites; update
InviteMemberSchema to normalize the email to lowercase at validation (use a
transform to toLowerCase) and ensure all places that create or query invites use
the normalized value (references: InviteMemberSchema and any findFirst({ email,
... }) usages and comparisons in the invite handling code noted around the other
ranges). Apply the same normalization to the other invite-related
schemas/validators mentioned (the blocks at ~472-546 and ~589-599) so all
incoming emails are consistently lowercased before storage or lookup.
- Around line 573-625: The acceptance flow is not atomic: two concurrent
requests can both see a PENDING invite and race to accept it, causing duplicate
member creation or a 500. Fix by performing a conditional update and create
inside a single transaction using the transactional client: replace the separate
teamInvitation.update and teamMember.create calls inside db.$transaction with an
interactive/atomic transaction that first does a conditional update (e.g.,
tx.teamInvitation.updateMany or tx.teamInvitation.update with a where that
includes status: 'PENDING' and expiresAt > new Date()) and check the affected
count/result; if the update did not affect exactly one row, throw a
ConflictError/NotFoundError; then create the team member with
tx.teamMember.create. Also catch/map unique-constraint errors from
tx.teamMember.create to a ConflictError to avoid returning 500. Ensure you still
use the original teamInvitation.findFirst / teamMember.findUnique checks only
for early validation or remove them to rely on the atomic transaction checks.
- Around line 238-268: The code does a separate slug existence check
(db.team.findUnique) before creating a team, which risks a TOCTOU race; remove
the pre-check and instead perform db.team.create directly inside the createTeam
handler (where CreateTeamSchema is used) and wrap it in a try/catch that catches
Prisma.PrismaClientKnownRequestError (import Prisma) and when err.code ===
'P2002' throws a ConflictError(`Team slug "${slug}" is already taken`); reapply
the same replace-and-catch pattern to the other create path that currently does
a findUnique (the block around the alternate creation logic referenced in the
review).
In @apps/server/src/scripts/verify-security.ts:
- Around line 23-31: The verifyHashing function performs a redundant
deterministic check by computing hash1 and hash2 from the same token; simplify
it by removing the duplicate computation and equality check: compute a single
SHA-256 hash of the random token (keep the console messages) and, if you want to
preserve the intent, add a short comment noting Node's crypto is deterministic;
update references to hash2/remove the if (hash1 !== hash2) throw line and ensure
only the single variable (hash1 or renamed hash) is logged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
apps/server/prisma/schema.prismaapps/server/src/config.tsapps/server/src/integrations/slack.tsapps/server/src/lib/crypto.tsapps/server/src/routes/slack.tsapps/server/src/routes/teams.tsapps/server/src/routes/webhooks.tsapps/server/src/scripts/verify-security.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/server/src/routes/slack.ts (5)
apps/server/src/services/db.ts (1)
getDb(20-33)apps/server/src/lib/errors.ts (3)
ValidationError(78-97)NotFoundError(148-156)ForbiddenError(141-141)apps/server/src/config.ts (1)
env(99-99)apps/server/src/integrations/slack.ts (5)
getSlackInstallUrl(121-134)handleSlackOAuthCallback(170-237)verifySlackRequest(90-111)getTeamStatus(272-353)formatStatusForSlack(375-445)apps/server/src/lib/logger.ts (1)
logger(68-68)
apps/server/src/integrations/slack.ts (6)
apps/server/src/lib/logger.ts (1)
logger(68-68)apps/server/src/config.ts (1)
env(99-99)apps/server/src/services/db.ts (1)
getDb(20-33)apps/server/src/lib/crypto.ts (2)
encrypt(20-26)decrypt(60-83)apps/server/src/services/redis.ts (1)
getPresences(212-237)apps/extension/src/views/friendsProvider.ts (1)
formatDuration(144-152)
apps/server/src/lib/crypto.ts (1)
apps/server/src/config.ts (1)
env(99-99)
🪛 GitHub Check: CodeQL
apps/server/src/routes/slack.ts
[failure] 146-192: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 207-299: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 314-343: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
🔇 Additional comments (17)
apps/server/src/scripts/verify-security.ts (1)
6-21: LGTM - Comprehensive crypto verification.The function properly tests both synchronous and asynchronous encryption paths with appropriate error handling.
apps/server/src/routes/webhooks.ts (2)
27-27: LGTM - Type expansion for raw body flexibility.The addition of
stringto the rawBody type provides flexibility for different content-addRawBody configurations while maintaining backward compatibility.
139-142: LGTM - Safe string-to-Buffer conversion.The conditional conversion properly handles both string and Buffer raw body types before signature verification, preserving verification semantics.
apps/server/src/config.ts (4)
49-52: LGTM - Slack configuration properly defined.The optional Slack variables with trim() and minimum length validation align with the cross-field validation that ensures all three are provided together.
54-56: LGTM - Strong encryption key validation.The ENCRYPTION_KEY requires a minimum of 32 characters, which is appropriate for AES-256-CBC. The optional API_BASE_URL has fallback handling in dependent code (e.g.,
getSlackRedirectUri()in slack.ts).
60-72: Cross-field validation correctly enforces all-or-nothing Slack configuration.The validation ensures that Slack integration requires all three credentials together, preventing partial configuration.
103-105: LGTM - Useful environment flags.The addition of
isDevelopmentandisTestflags complementsisProductionand improves code clarity across the application.apps/server/src/lib/crypto.ts (2)
20-26: LGTM - Correct synchronous encryption implementation.The function properly generates a random IV for each encryption operation and returns the result in "iv:encrypted" format. The use of
crypto.randomBytes()for IV generation ensures uniqueness.
60-83: LGTM - Robust decrypt implementation with proper validation.The function includes comprehensive validation:
- Empty input check
- Format validation (requires ':' delimiter)
- Component validation (IV and ciphertext presence)
The use of
parts.join(':')(Line 71) is defensive programming that handles edge cases, though in practice hex-encoded ciphertext won't contain colons.apps/server/src/integrations/slack.ts (7)
90-111: LGTM - Secure Slack request verification.The implementation correctly follows Slack's verification specification:
- Prevents replay attacks via timestamp validation (5-minute window)
- Uses HMAC-SHA256 for signature computation
- Employs
crypto.timingSafeEqual()to prevent timing attacks- Properly handles exceptions during comparison
121-134: LGTM - Proper OAuth URL generation with CSRF protection.The function correctly constructs the Slack OAuth authorization URL with appropriate parameters, including state-based CSRF protection.
170-237: LGTM - Secure OAuth callback handling.The OAuth flow implementation includes important security measures:
- Validates OAuth response with Zod schema (Lines 188-193)
- Verifies team existence before persisting workspace (Lines 198-205)
- Encrypts access token before storage (Lines 213, 219)
- Uses upsert for idempotency
245-264: LGTM - Proper client provisioning with error handling.The function safely decrypts the stored access token and gracefully handles decryption failures by logging and returning null, allowing callers to handle missing or corrupted tokens appropriately.
375-445: LGTM - Well-structured Slack Block Kit formatting.The function creates properly formatted Slack blocks with:
- Clear status categorization (online/idle/offline)
- Rich activity information for online members
- Appropriate use of Block Kit components (header, sections, context)
- Timestamp for freshness indication
456-480: LGTM - Clean message posting with error handling.The function appropriately handles missing Slack clients and API errors, logging failures while returning null to allow graceful degradation.
487-511: LGTM - Daily summary implementation.The function correctly orchestrates status retrieval, formatting, and posting. Error propagation is appropriate for a scheduled task that should fail visibly if the operation cannot complete.
apps/server/src/routes/slack.ts (1)
134-193: No issues found—rate limiting configuration is valid and correctly implemented.@fastify/rate-limit 10.3.0 fully supports per-route overrides via
config.rateLimit, which is exactly the pattern used in the slack.ts routes. The plugin is properly registered globally, slackRoutes is correctly mounted as a plugin with the/slackprefix, and all route handlers (lines 134–193, 195–206, 302–313) will correctly enforce their respective per-route limits without additional changes.
| enum InvitationStatus { | ||
| PENDING | ||
| ACCEPTED | ||
| EXPIRED | ||
| REVOKED | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Phase 3: B2B & Teams | ||
| // ============================================================================ | ||
|
|
||
| /// Team invitation for adding members via email link. | ||
| model TeamInvitation { | ||
| id String @id @default(cuid()) | ||
| teamId String | ||
| email String | ||
| role Role @default(MEMBER) | ||
| tokenHash String @unique // SHA-256 hash of the invitation token | ||
| expiresAt DateTime // Token expiration (e.g., 7 days from creation) | ||
| status InvitationStatus @default(PENDING) | ||
| inviterId String | ||
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
|
|
||
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | ||
| inviter User @relation("SentTeamInvitations", fields: [inviterId], references: [id], onDelete: Cascade) | ||
|
|
||
| @@index([teamId]) | ||
| @@index([email]) | ||
| @@index([tokenHash]) | ||
| @@index([status]) | ||
| @@index([teamId, email, status]) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the current schema file
cat -n apps/server/prisma/schema.prisma | sed -n '164,196p'Repository: senutpal/devRadar
Length of output: 1369
🏁 Script executed:
# Search for team invitation creation logic
rg -t ts -t tsx "TeamInvitation" --max-count=20 -A 3Repository: senutpal/devRadar
Length of output: 88
🏁 Script executed:
# Look for endpoint handlers that create invitations
rg -t ts -t tsx "team.*invitation|invitation.*team" -i --max-count=20Repository: senutpal/devRadar
Length of output: 88
🏁 Script executed:
# Search for any transaction handling around invitation creation
rg -t ts -t tsx "@transaction|transaction\(" --max-count=15Repository: senutpal/devRadar
Length of output: 88
🏁 Script executed:
# Search for TeamInvitation references without type restriction
rg "TeamInvitation" --max-count=25 -A 2 -B 1Repository: senutpal/devRadar
Length of output: 1496
🏁 Script executed:
# Look for invitation creation endpoints (routes)
fd -e ts -e js | head -20Repository: senutpal/devRadar
Length of output: 850
🏁 Script executed:
# Search for "pending" invitation checks in the codebase
rg "PENDING.*invitation|invitation.*PENDING" -i --max-count=15Repository: senutpal/devRadar
Length of output: 391
🏁 Script executed:
# Search for where invitations are created
rg "create.*[Ii]nvitation|[Ii]nvitation.*create" --max-count=15 -A 3Repository: senutpal/devRadar
Length of output: 562
🏁 Script executed:
# Get the full invitation creation logic from teams.ts
rg "Check for existing pending" -A 30 -B 10 apps/server/src/routes/teams.tsRepository: senutpal/devRadar
Length of output: 1277
🏁 Script executed:
# Check if there are any database transactions around invitation operations
rg "transaction|findFirst.*PENDING.*invitation|uniqueConstraint" -A 5 apps/server/src/routes/teams.tsRepository: senutpal/devRadar
Length of output: 297
🏁 Script executed:
# Check if there are any operations that rely on multiple rows with same (teamId, email, status)
rg "findMany.*teamInvitation|teamInvitation.*findMany" -A 5 apps/server/src/Repository: senutpal/devRadar
Length of output: 429
🏁 Script executed:
# Check for any code that updates or queries without status filter
rg "teamInvitation\.(update|delete|findUnique)" -B 2 -A 3 apps/server/src/Repository: senutpal/devRadar
Length of output: 772
🏁 Script executed:
# Look for any bulk operations on TeamInvitation
rg "teamInvitation\.(updateMany|deleteMany)" apps/server/src/Repository: senutpal/devRadar
Length of output: 43
Add unique constraint on (teamId, email, status) to prevent duplicate pending invitations.
The current code checks for existing PENDING invitations before creating a new one, but the check and create are not transactional. Concurrent requests can both pass the findFirst check and create duplicate invitations. A unique constraint on (teamId, email, status) enforces this at the database level while still allowing the PENDING → ACCEPTED transition.
Proposed schema change
model TeamInvitation {
id String @id @default(cuid())
teamId String
email String
role Role @default(MEMBER)
tokenHash String @unique // SHA-256 hash of the invitation token
expiresAt DateTime // Token expiration (e.g., 7 days from creation)
status InvitationStatus @default(PENDING)
inviterId String
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
team Team @relation(fields: [teamId], references: [id], onDelete: Cascade)
inviter User @relation("SentTeamInvitations", fields: [inviterId], references: [id], onDelete: Cascade)
@@index([teamId])
@@index([email])
@@index([tokenHash])
@@index([status])
- @@index([teamId, email, status])
+ @@unique([teamId, email, status])
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enum InvitationStatus { | |
| PENDING | |
| ACCEPTED | |
| EXPIRED | |
| REVOKED | |
| } | |
| // ============================================================================ | |
| // Phase 3: B2B & Teams | |
| // ============================================================================ | |
| /// Team invitation for adding members via email link. | |
| model TeamInvitation { | |
| id String @id @default(cuid()) | |
| teamId String | |
| email String | |
| role Role @default(MEMBER) | |
| tokenHash String @unique // SHA-256 hash of the invitation token | |
| expiresAt DateTime // Token expiration (e.g., 7 days from creation) | |
| status InvitationStatus @default(PENDING) | |
| inviterId String | |
| createdAt DateTime @default(now()) | |
| updatedAt DateTime @updatedAt | |
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | |
| inviter User @relation("SentTeamInvitations", fields: [inviterId], references: [id], onDelete: Cascade) | |
| @@index([teamId]) | |
| @@index([email]) | |
| @@index([tokenHash]) | |
| @@index([status]) | |
| @@index([teamId, email, status]) | |
| } | |
| enum InvitationStatus { | |
| PENDING | |
| ACCEPTED | |
| EXPIRED | |
| REVOKED | |
| } | |
| // ============================================================================ | |
| // Phase 3: B2B & Teams | |
| // ============================================================================ | |
| /// Team invitation for adding members via email link. | |
| model TeamInvitation { | |
| id String @id @default(cuid()) | |
| teamId String | |
| email String | |
| role Role @default(MEMBER) | |
| tokenHash String @unique // SHA-256 hash of the invitation token | |
| expiresAt DateTime // Token expiration (e.g., 7 days from creation) | |
| status InvitationStatus @default(PENDING) | |
| inviterId String | |
| createdAt DateTime @default(now()) | |
| updatedAt DateTime @updatedAt | |
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | |
| inviter User @relation("SentTeamInvitations", fields: [inviterId], references: [id], onDelete: Cascade) | |
| @@index([teamId]) | |
| @@index([email]) | |
| @@index([tokenHash]) | |
| @@index([status]) | |
| @@unique([teamId, email, status]) | |
| } |
🤖 Prompt for AI Agents
In @apps/server/prisma/schema.prisma around lines 164 - 196, The TeamInvitation
model can still get duplicate PENDING rows under concurrent requests; add a
DB-level unique constraint on the tuple (teamId, email, status) in the
TeamInvitation model to prevent duplicate pending invites (referencing model
TeamInvitation and fields teamId, email, status), then run the Prisma migration
to apply it and update the invitation creation code to handle unique constraint
violations (catch the unique constraint/db error on create and convert it into
the existing "duplicate pending invitation" response) so concurrent creates fail
gracefully.
| model TeamInvitation { | ||
| id String @id @default(cuid()) | ||
| teamId String | ||
| email String | ||
| role Role @default(MEMBER) | ||
| tokenHash String @unique // SHA-256 hash of the invitation token | ||
| expiresAt DateTime // Token expiration (e.g., 7 days from creation) | ||
| status InvitationStatus @default(PENDING) | ||
| inviterId String | ||
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
|
|
||
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | ||
| inviter User @relation("SentTeamInvitations", fields: [inviterId], references: [id], onDelete: Cascade) | ||
|
|
||
| @@index([teamId]) | ||
| @@index([email]) | ||
| @@index([tokenHash]) | ||
| @@index([status]) | ||
| @@index([teamId, email, status]) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
@@index([tokenHash]) is redundant when tokenHash is @unique.
Not harmful, but unnecessary.
🤖 Prompt for AI Agents
In @apps/server/prisma/schema.prisma around lines 176 - 196, The TeamInvitation
schema contains a redundant index on tokenHash: since tokenHash is declared
@unique, remove the @@index([tokenHash]) line from the TeamInvitation model,
save the schema, run prisma format/migrate to update the schema and regenerate
the Prisma client so changes take effect; target the TeamInvitation model and
the tokenHash field when making this edit.
| model SlackWorkspace { | ||
| id String @id @default(cuid()) | ||
| slackWorkspaceId String @unique // Slack's workspace/team ID (T01234567) | ||
| slackTeamName String // Human-readable workspace name | ||
| accessToken String // Bot OAuth token (encrypted at rest in production) | ||
| botUserId String? // Slack bot user ID | ||
| channelId String? // Default channel for daily summaries | ||
| teamId String @unique // One Slack workspace per DevRadar team | ||
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
|
|
||
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | ||
|
|
||
| @@index([slackWorkspaceId]) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
SlackWorkspace.accessToken storage: verify encryption and key management assumptions.
Schema comment says “encrypted at rest in production”; ensure all writes use encrypt(...) and you have a rotation story (or at least versioning) for the ciphertext format.
🤖 Prompt for AI Agents
In @apps/server/prisma/schema.prisma around lines 199 - 213,
SlackWorkspace.accessToken is documented as "encrypted at rest" but the codebase
must enforce encryption on every write and support ciphertext
versioning/rotation: update the code paths that create or update SlackWorkspace
records (e.g., createSlackWorkspace, updateSlackWorkspace, or Prisma write
middleware like any Prisma $use that intercepts writes) to run accessToken
through your encrypt(...) function before persisting and store accompanying
metadata (ciphertext version or KMS key id, e.g., accessTokenVersion or
accessTokenKeyId); ensure read paths decrypt the token with decrypt(...) based
on that metadata; add a migration/rotation helper to re-encrypt existing rows
when rotating keys and validate tests cover create/read/update flows to prove
encryption and versioning are used.
| for (const member of allMembers) { | ||
| const presence = presences.get(member.id); | ||
| const status: TeamMemberStatus = { | ||
| userId: member.id, | ||
| username: member.username, | ||
| displayName: member.displayName, | ||
| status: presence?.status ? (presence.status as TeamMemberStatus['status']) : 'offline', | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type assertion could be unsafe without validation.
Line 313 uses a type assertion for presence.status without validating that the value is actually one of the expected status types. Consider validating the status value before assertion to prevent runtime type mismatches.
♻️ Safer status handling
+ const validStatuses = ['online', 'idle', 'offline', 'dnd'] as const;
+ const presenceStatus = presence?.status;
+ const status: TeamMemberStatus['status'] =
+ validStatuses.includes(presenceStatus as any)
+ ? (presenceStatus as TeamMemberStatus['status'])
+ : 'offline';
+
const status: TeamMemberStatus = {
userId: member.id,
username: member.username,
displayName: member.displayName,
- status: presence?.status ? (presence.status as TeamMemberStatus['status']) : 'offline',
+ status,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const member of allMembers) { | |
| const presence = presences.get(member.id); | |
| const status: TeamMemberStatus = { | |
| userId: member.id, | |
| username: member.username, | |
| displayName: member.displayName, | |
| status: presence?.status ? (presence.status as TeamMemberStatus['status']) : 'offline', | |
| }; | |
| for (const member of allMembers) { | |
| const presence = presences.get(member.id); | |
| const validStatuses = ['online', 'idle', 'offline', 'dnd'] as const; | |
| const presenceStatus = presence?.status; | |
| const validatedStatus: TeamMemberStatus['status'] = | |
| validStatuses.includes(presenceStatus as any) | |
| ? (presenceStatus as TeamMemberStatus['status']) | |
| : 'offline'; | |
| const status: TeamMemberStatus = { | |
| userId: member.id, | |
| username: member.username, | |
| displayName: member.displayName, | |
| status: validatedStatus, | |
| }; |
🤖 Prompt for AI Agents
In @apps/server/src/integrations/slack.ts around lines 307 - 314, The code in
the for loop over allMembers uses a raw assertion (presence.status as
TeamMemberStatus['status']) when building the TeamMemberStatus, which can be
unsafe; modify the logic in the loop (around presences.get(member.id) and the
status assignment) to validate presence?.status against the allowed
TeamMemberStatus values (e.g., via a small helper isValidTeamStatus(status) or a
Set/array of allowed statuses) and only cast when valid, otherwise fall back to
'offline' — update the status assignment for TeamMemberStatus accordingly so no
unchecked type assertion is used.
| function formatDuration(seconds: number): string { | ||
| if (seconds < 60) return `${String(seconds)}s`; | ||
| if (seconds < 3600) return `${String(Math.floor(seconds / 60))}m`; | ||
| const hours = Math.floor(seconds / 3600); | ||
| const mins = Math.floor((seconds % 3600) / 60); | ||
| return `${String(hours)}h ${String(mins)}m`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication: formatDuration exists in multiple locations.
This function is duplicated in apps/extension/src/views/friendsProvider.ts (lines 143-151) with slightly different implementations. Consider extracting this utility to the shared package (packages/shared) to maintain consistency and reduce duplication.
🤖 Prompt for AI Agents
In @apps/server/src/integrations/slack.ts around lines 361 - 367, formatDuration
is duplicated across modules (e.g., the local function in slack.ts and the one
in friendsProvider) causing divergence; extract a single implementation into the
shared package (packages/shared) as an exported utility (e.g., export function
formatDuration(seconds: number): string) and replace the local definitions by
importing that shared function in both places; ensure you keep the canonical
behavior (seconds <60 -> "Xs", <3600 -> "Ym", otherwise "Hh Mm") and update any
imports/usages to the new shared symbol.
| // GET /me - List user's teams | ||
| app.get('/me', { onRequest: [app.authenticate] }, async (request: FastifyRequest, reply) => { | ||
| const { userId } = request.user as { userId: string }; | ||
|
|
||
| const paginationResult = PaginationQuerySchema.safeParse(request.query); | ||
| const { page, limit } = paginationResult.success | ||
| ? paginationResult.data | ||
| : { page: 1, limit: 20 }; | ||
|
|
||
| const skip = (page - 1) * limit; | ||
|
|
||
| // Get all teams where user is owner or member (fetch all to handle deduping/sorting correctly) | ||
| const [ownedTeams, memberships] = await Promise.all([ | ||
| db.team.findMany({ | ||
| where: { ownerId: userId }, | ||
| include: { | ||
| _count: { select: { members: true } }, | ||
| }, | ||
| orderBy: { createdAt: 'desc' }, | ||
| }), | ||
| db.teamMember.findMany({ | ||
| where: { userId }, | ||
| include: { | ||
| team: { | ||
| include: { | ||
| _count: { select: { members: true } }, | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| ]); | ||
|
|
||
| // Merge and dedupe | ||
| interface MergedTeam { | ||
| id: string; | ||
| name: string; | ||
| slug: string; | ||
| role: 'OWNER' | 'ADMIN' | 'MEMBER'; | ||
| memberCount: number; | ||
| createdAt: Date; | ||
| } | ||
| const teamMap = new Map<string, MergedTeam>(); | ||
|
|
||
| ownedTeams.forEach((t) => { | ||
| teamMap.set(t.id, { | ||
| id: t.id, | ||
| name: t.name, | ||
| slug: t.slug, | ||
| role: 'OWNER' as const, | ||
| memberCount: t._count.members + 1, | ||
| createdAt: new Date(t.createdAt), | ||
| }); | ||
| }); | ||
|
|
||
| memberships.forEach((m) => { | ||
| // Owner role takes precedence if present in both lists | ||
| if (!teamMap.has(m.team.id) || teamMap.get(m.team.id)?.role !== 'OWNER') { | ||
| teamMap.set(m.team.id, { | ||
| id: m.team.id, | ||
| name: m.team.name, | ||
| slug: m.team.slug, | ||
| role: m.role, | ||
| memberCount: m.team._count.members + 1, | ||
| createdAt: new Date(m.joinedAt), | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| const allTeams = Array.from(teamMap.values()); | ||
|
|
||
| // Sort by createdAt desc | ||
| allTeams.sort((a, b) => b.createdAt.getTime() - a.createdAt.getTime()); | ||
|
|
||
| // Apply pagination in memory | ||
| const total = allTeams.length; | ||
| const paginatedTeams = allTeams.slice(skip, skip + limit).map((t) => ({ | ||
| ...t, | ||
| createdAt: t.createdAt.toISOString(), | ||
| })); | ||
|
|
||
| return reply.send({ | ||
| data: paginatedTeams, | ||
| pagination: { | ||
| page, | ||
| limit, | ||
| total, | ||
| hasMore: skip + paginatedTeams.length < total, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
createdAt is overloaded (team.createdAt vs member.joinedAt) leading to confusing ordering/pagination.
Owners use team.createdAt; members use joinedAt stored in the createdAt field, then you sort by it. Either rename the field (e.g., associatedAt) or use team’s createdAt consistently.
| const bodyResult = CreateTeamSchema.safeParse(request.body); | ||
| if (!bodyResult.success) { | ||
| throw new ValidationError(bodyResult.error.errors[0]?.message ?? 'Invalid input'); | ||
| } | ||
|
|
||
| const { name, slug } = bodyResult.data; | ||
|
|
||
| // Check user's tier (only TEAM tier can create teams) | ||
| const user = await db.user.findUnique({ | ||
| where: { id: userId }, | ||
| select: { tier: true }, | ||
| }); | ||
|
|
||
| if (!user) { | ||
| throw new AuthenticationError('User not found'); | ||
| } | ||
|
|
||
| if (user.tier !== 'TEAM') { | ||
| throw new ForbiddenError('Team creation requires TEAM tier subscription'); | ||
| } | ||
|
|
||
| // Check if slug is already taken | ||
| const existingTeam = await db.team.findUnique({ | ||
| where: { slug }, | ||
| select: { id: true }, | ||
| }); | ||
|
|
||
| if (existingTeam) { | ||
| throw new ConflictError(`Team slug "${slug}" is already taken`); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the current code at the specified lines
head -n 300 apps/server/src/routes/teams.ts | tail -n 65Repository: senutpal/devRadar
Length of output: 1630
🏁 Script executed:
# Look for the schema definition to check unique constraints
find . -type f -name "schema.prisma" -o -name "*.prisma"Repository: senutpal/devRadar
Length of output: 95
🏁 Script executed:
# Search for Team model definition and unique constraints
rg "model Team" -A 20Repository: senutpal/devRadar
Length of output: 3801
🏁 Script executed:
# Check lines 270-286
sed -n '270,286p' apps/server/src/routes/teams.tsRepository: senutpal/devRadar
Length of output: 381
🏁 Script executed:
# Verify Prisma error code P2002 for unique constraint violations
rg "P2002" apps/server/src --type tsRepository: senutpal/devRadar
Length of output: 519
Replace check-then-create with create-and-catch pattern to prevent TOCTOU race condition on slug uniqueness.
The current code checks if the slug exists, then creates the team. Under concurrent requests, two threads can both pass the check and attempt creation, violating the unique constraint. Instead, attempt creation directly and catch Prisma's unique constraint violation error (code P2002), which maps to ConflictError.
Proposed fix
- // Check if slug is already taken
- const existingTeam = await db.team.findUnique({
- where: { slug },
- select: { id: true },
- });
-
- if (existingTeam) {
- throw new ConflictError(`Team slug "${slug}" is already taken`);
- }
-
- // Create team with owner
- const team = await db.team.create({
- data: {
- name,
- slug,
- ownerId: userId,
- },
- include: {
- owner: {
- select: {
- id: true,
- username: true,
- displayName: true,
- avatarUrl: true,
- },
- },
- },
- });
+ let team;
+ try {
+ team = await db.team.create({
+ data: { name, slug, ownerId: userId },
+ include: {
+ owner: {
+ select: { id: true, username: true, displayName: true, avatarUrl: true },
+ },
+ },
+ });
+ } catch (err: any) {
+ if (err?.code === 'P2002') {
+ throw new ConflictError(`Team slug "${slug}" is already taken`);
+ }
+ throw err;
+ }Also applies to: 270-286
🤖 Prompt for AI Agents
In @apps/server/src/routes/teams.ts around lines 238 - 268, The code does a
separate slug existence check (db.team.findUnique) before creating a team, which
risks a TOCTOU race; remove the pre-check and instead perform db.team.create
directly inside the createTeam handler (where CreateTeamSchema is used) and wrap
it in a try/catch that catches Prisma.PrismaClientKnownRequestError (import
Prisma) and when err.code === 'P2002' throws a ConflictError(`Team slug
"${slug}" is already taken`); reapply the same replace-and-catch pattern to the
other create path that currently does a findUnique (the block around the
alternate creation logic referenced in the review).
| const bodyResult = UpdateTeamSchema.safeParse(request.body); | ||
| if (!bodyResult.success) { | ||
| throw new ValidationError(bodyResult.error.errors[0]?.message ?? 'Invalid input'); | ||
| } | ||
|
|
||
| const { id: teamId } = paramsResult.data; | ||
| const updates = bodyResult.data; | ||
|
|
||
| // Only owner or admin can update team | ||
| const { isAdmin } = await checkTeamAdmin(db, teamId, userId); | ||
| if (!isAdmin) { | ||
| throw new ForbiddenError('Only team owner or admin can update team settings'); | ||
| } | ||
|
|
||
| const team = await db.team.update({ | ||
| where: { id: teamId }, | ||
| data: updates.name ? { name: updates.name } : {}, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the UpdateTeamSchema definition
find . -type f -name "*.ts" -o -name "*.js" | xargs grep -l "UpdateTeamSchema" | head -20Repository: senutpal/devRadar
Length of output: 94
🏁 Script executed:
# Read the teams.ts file to find UpdateTeamSchema definition and handler code
cat -n apps/server/src/routes/teams.ts | head -450Repository: senutpal/devRadar
Length of output: 15351
🏁 Script executed:
# Search for Prisma version and check if there's a prisma schema to understand the setup
find . -name "package.json" -type f | xargs grep -l prisma | head -5Repository: senutpal/devRadar
Length of output: 87
🏁 Script executed:
# Check Prisma version
cat apps/server/package.json | grep -A 5 -B 5 "prisma"Repository: senutpal/devRadar
Length of output: 1107
🏁 Script executed:
# Let's verify Prisma's behavior with empty data objects by checking if there's any documentation or usage patterns
# Also check if there are any other PATCH/update handlers to see how they handle this
rg "db\.(.*?)\.update\(" -A 3 apps/server/src/routes/ -t tsRepository: senutpal/devRadar
Length of output: 1667
🌐 Web query:
Prisma update empty data object {} behavior error
💡 Result:
Prisma will reject an update when the data object is empty. The client performs validation and throws a PrismaClientValidationError for missing/empty required arguments (e.g. an empty data: {} on create/update). Ensure you supply at least one updatable field or skip the call if there is nothing to change. Recommended options:
- Validate before calling: if Object.keys(data).length === 0, skip the update (or return 400 to client).
- Build the data object only with changed fields.
- Use find + conditional update (findUnique/findFirst then update only when needed).
References: Prisma CRUD docs and error reference describing validation for empty data. [1][2]
Sources
[1] Prisma CRUD (update) documentation.
[2] Prisma Errors / PrismaClientValidationError (empty data example).
Prevent empty PATCH body from reaching Prisma update (causes PrismaClientValidationError).
UpdateTeamSchema allows {} and the handler can call db.team.update({ data: {} }), which Prisma rejects with a validation error. Add validation to ensure at least one field is provided.
Proposed fix (schema-level or handler-level)
-const UpdateTeamSchema = z.object({
- name: z.string().min(2).max(50).optional(),
-});
+const UpdateTeamSchema = z
+ .object({
+ name: z.string().min(2).max(50).optional(),
+ })
+ .refine((v) => typeof v.name === 'string', { message: 'No updates provided' });Or, handler-side:
const updates = bodyResult.data;
+ if (!updates.name) {
+ throw new ValidationError('No updates provided');
+ }
const team = await db.team.update({
where: { id: teamId },
- data: updates.name ? { name: updates.name } : {},
+ data: { name: updates.name },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const bodyResult = UpdateTeamSchema.safeParse(request.body); | |
| if (!bodyResult.success) { | |
| throw new ValidationError(bodyResult.error.errors[0]?.message ?? 'Invalid input'); | |
| } | |
| const { id: teamId } = paramsResult.data; | |
| const updates = bodyResult.data; | |
| // Only owner or admin can update team | |
| const { isAdmin } = await checkTeamAdmin(db, teamId, userId); | |
| if (!isAdmin) { | |
| throw new ForbiddenError('Only team owner or admin can update team settings'); | |
| } | |
| const team = await db.team.update({ | |
| where: { id: teamId }, | |
| data: updates.name ? { name: updates.name } : {}, | |
| }); | |
| const bodyResult = UpdateTeamSchema.safeParse(request.body); | |
| if (!bodyResult.success) { | |
| throw new ValidationError(bodyResult.error.errors[0]?.message ?? 'Invalid input'); | |
| } | |
| const { id: teamId } = paramsResult.data; | |
| const updates = bodyResult.data; | |
| if (!updates.name) { | |
| throw new ValidationError('No updates provided'); | |
| } | |
| // Only owner or admin can update team | |
| const { isAdmin } = await checkTeamAdmin(db, teamId, userId); | |
| if (!isAdmin) { | |
| throw new ForbiddenError('Only team owner or admin can update team settings'); | |
| } | |
| const team = await db.team.update({ | |
| where: { id: teamId }, | |
| data: { name: updates.name }, | |
| }); |
| const invitation = await db.teamInvitation.findFirst({ | ||
| where: { | ||
| teamId, | ||
| tokenHash, | ||
| status: 'PENDING', | ||
| expiresAt: { gt: new Date() }, | ||
| }, | ||
| include: { | ||
| team: { select: { name: true } }, | ||
| }, | ||
| }); | ||
|
|
||
| if (!invitation) { | ||
| throw new NotFoundError('Valid invitation'); | ||
| } | ||
|
|
||
| // Verify email matches (or allow any authenticated user for now) | ||
| // Note: Email verification could be added here for stricter access control | ||
| const user = await db.user.findUnique({ | ||
| where: { id: userId }, | ||
| select: { email: true }, | ||
| }); | ||
|
|
||
| if (user?.email?.toLowerCase() !== invitation.email.toLowerCase()) { | ||
| throw new ForbiddenError('This invitation was sent to a different email address'); | ||
| } | ||
|
|
||
| // Check if already a member | ||
| const existingMember = await db.teamMember.findUnique({ | ||
| where: { userId_teamId: { userId, teamId } }, | ||
| }); | ||
|
|
||
| if (existingMember) { | ||
| throw new ConflictError('You are already a member of this team'); | ||
| } | ||
|
|
||
| // Accept invitation in a transaction | ||
| await db.$transaction([ | ||
| // Update invitation status | ||
| db.teamInvitation.update({ | ||
| where: { id: invitation.id }, | ||
| data: { status: 'ACCEPTED' }, | ||
| }), | ||
| // Add as team member | ||
| db.teamMember.create({ | ||
| data: { | ||
| userId, | ||
| teamId, | ||
| role: invitation.role, | ||
| }, | ||
| }), | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Make invitation acceptance atomic (concurrent accepts can double-process).
Two requests can both read PENDING then both run the transaction; the second will likely fail on TeamMember @@unique([userId, teamId]), but you’ll also mark the invite ACCEPTED twice and return a 500 unless you map the error.
Proposed fix (conditional update within an interactive transaction)
- // Accept invitation in a transaction
- await db.$transaction([
- // Update invitation status
- db.teamInvitation.update({
- where: { id: invitation.id },
- data: { status: 'ACCEPTED' },
- }),
- // Add as team member
- db.teamMember.create({
- data: {
- userId,
- teamId,
- role: invitation.role,
- },
- }),
- ]);
+ await db.$transaction(async (tx) => {
+ const updated = await tx.teamInvitation.updateMany({
+ where: {
+ id: invitation.id,
+ status: 'PENDING',
+ expiresAt: { gt: new Date() },
+ },
+ data: { status: 'ACCEPTED' },
+ });
+
+ if (updated.count !== 1) {
+ throw new ConflictError('Invitation is no longer valid');
+ }
+
+ await tx.teamMember.create({
+ data: { userId, teamId, role: invitation.role },
+ });
+ });🤖 Prompt for AI Agents
In @apps/server/src/routes/teams.ts around lines 573 - 625, The acceptance flow
is not atomic: two concurrent requests can both see a PENDING invite and race to
accept it, causing duplicate member creation or a 500. Fix by performing a
conditional update and create inside a single transaction using the
transactional client: replace the separate teamInvitation.update and
teamMember.create calls inside db.$transaction with an interactive/atomic
transaction that first does a conditional update (e.g.,
tx.teamInvitation.updateMany or tx.teamInvitation.update with a where that
includes status: 'PENDING' and expiresAt > new Date()) and check the affected
count/result; if the update did not affect exactly one row, throw a
ConflictError/NotFoundError; then create the team member with
tx.teamMember.create. Also catch/map unique-constraint errors from
tx.teamMember.create to a ConflictError to avoid returning 500. Ensure you still
use the original teamInvitation.findFirst / teamMember.findUnique checks only
for early validation or remove them to rely on the atomic transaction checks.
| function verifyHashing() { | ||
| console.log('Verifying Hashing...'); | ||
| const token = crypto.randomBytes(32).toString('hex'); | ||
| const hash1 = crypto.createHash('sha256').update(token).digest('hex'); | ||
| const hash2 = crypto.createHash('sha256').update(token).digest('hex'); | ||
|
|
||
| if (hash1 !== hash2) throw new Error('Hashing inconsistency'); | ||
| console.log(`✅ Hashing passed: ${hash1}`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hashing consistency test is redundant but acceptable.
This test verifies deterministic hashing behavior, which is already guaranteed by Node.js crypto. However, for a verification script, this redundancy provides additional confidence and is acceptable.
🤖 Prompt for AI Agents
In @apps/server/src/scripts/verify-security.ts around lines 23 - 31, The
verifyHashing function performs a redundant deterministic check by computing
hash1 and hash2 from the same token; simplify it by removing the duplicate
computation and equality check: compute a single SHA-256 hash of the random
token (keep the console messages) and, if you want to preserve the intent, add a
short comment noting Node's crypto is deterministic; update references to
hash2/remove the if (hash1 !== hash2) throw line and ensure only the single
variable (hash1 or renamed hash) is logged.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.