fix: use higher entropy invite tokens #2558
Conversation
We currently use CUIDs for invite tokens, which are generated using a pattern and are not cryptographically secure. This PR switches to a higher entropy string generated with `nanoid`.
|
WalkthroughReplaces inline invite construction in apps/webapp/app/models/member.server.ts with a pre-built invites array that includes a generated token per invite. Adds a module-level token length and token generator using nanoid, and imports Prisma types. Uses Prisma's createMany to insert the invites (removing the previous skipDuplicates option). Keeps the subsequent query flow to return the created invites. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/webapp/app/models/member.server.ts (1)
102-111: Hash tokens at rest; uniqueness is already enforced
Prisma schema definestoken String @unique @default(cuid()). To avoid plaintext exposure in DB/backups, store only a hash (e.g., SHA-256) and match incoming tokens by hash. Ensure tokens aren’t serialized to clients or logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/models/member.server.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/models/member.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/models/member.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/models/member.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/models/member.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/models/member.server.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/webapp/app/models/member.server.ts (1)
1-1: Good: typed Prisma import used forsatisfiesImporting
type Prismaavoids runtime bloat and keeps checks at compile-time. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webapp/app/models/member.server.ts (1)
102-111: Normalize and dedupe emails (trim; optional case-insensitive) before creating invitesPrevent surprises from whitespace in inputs and reduce duplicate rows from variant spellings. Minimal change is to trim before Set-deduping.
Apply within this hunk:
- const invites = [...new Set(emails)].map( + const normalizedEmails = [...new Set(emails.map((e) => e.trim()))]; + const invites = normalizedEmails.map( (email) => ({ email, token: tokenGenerator(), organizationId: org.id, inviterId: userId, role: "MEMBER", } satisfies Prisma.OrgMemberInviteCreateManyInput) );If you want case-insensitive dedupe, build a Map keyed by
email.toLowerCase()and store the first occurrence.Also update the subsequent query to use
normalizedEmailsfor consistency (outside this hunk).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/models/member.server.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/models/member.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/models/member.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/models/member.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/models/member.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/models/member.server.ts
🧬 Code graph analysis (1)
apps/webapp/app/models/member.server.ts (1)
apps/webapp/app/db.server.ts (1)
Prisma(99-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/webapp/app/models/member.server.ts (3)
1-1: Type-only Prisma import looks goodUsing
import type Prismakeeps runtime clean while enabling thesatisfiescheck below.
113-115: Idempotency across requests: consider re‑enabling skipDuplicates with a composite uniqueAs previously noted, deduping the input array avoids intra-request dupes, but repeated requests can still create multiple invites for the same (organizationId, email) unless the DB enforces a composite unique. After adding
@@unique([organizationId, email]), re-enableskipDuplicates:await prisma.orgMemberInvite.createMany({ - data: invites, + data: invites, + skipDuplicates: true, // requires @@unique([organizationId, email]) in the Prisma model });This makes the batch write safe to retry.
3-6: Secure, high-entropy token: ensure DB uniqueness and prefer findUnique for lookupsGood switch to
nanoidwith a fixed integer length. Please ensureOrgMemberInvite.tokenhas a DB unique index; otherwise,getInviteFromToken’sfindFirstcan be ambiguous if a duplicate ever occurs (however unlikely), and it weakens guarantees around token-based access.Follow-ups:
- Add a unique constraint on
tokenin Prisma (and ensure the column length supports 40 chars).- After that, change
getInviteFromTokento usefindUnique({ where: { token } }).Example (outside this diff):
- Prisma schema:
model OrgMemberInvite { token String @unique ... }- Lookup:
export async function getInviteFromToken({ token }: { token: string }) {
return prisma.orgMemberInvite.findUnique({ where: { token }, include: { organization: true, inviter: true } });
}Note: The nanoid version audit across the repo was called out in a previous review; not repeating details here.
We currently use CUIDs for invite tokens, which are generated using
a pattern and are not cryptographically secure. This PR switches to
a higher entropy string generated with
nanoid.Will remove the default value in the prisma schema in a separate PR.