-
Notifications
You must be signed in to change notification settings - Fork 53.3k
feat: Change resolveSignupToken to use new JWT for invite urls #23995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/cli/src/controllers/__tests__/auth.controller.test.ts">
<violation number="1" location="packages/cli/src/controllers/__tests__/auth.controller.test.ts:576">
P2: Test title doesn't match what's being tested. The title says "neither token nor inviterId/inviteeId are provided" but the payload actually contains `inviterId`. Consider either:
1. Renaming to "throws BadRequestError if only inviterId is provided without inviteeId", or
2. Changing the payload to `{} as ResolveSignupTokenQueryDto` to match the current title</violation>
</file>
<file name="packages/cli/src/services/user.service.ts">
<violation number="1" location="packages/cli/src/services/user.service.ts:454">
P1: `getInvitationIdsFromPayload()` gates JWT vs legacy parsing using the **instance owner’s** feature flags, but `sendEmails()` chooses JWT vs legacy based on the **inviter’s** feature flags. This mismatch can cause valid invite URLs to be rejected (and invalid formats accepted) depending on who created the invite link. Consider determining the flag based on the inviter (token.inviterId after verification, or payload.inviterId for legacy) rather than the global owner.</violation>
<violation number="2" location="packages/cli/src/services/user.service.ts:464">
P2: `getInvitationIdsFromPayload()` calls `postHog.getFeatureFlags()` without a timeout, even though this file already documents PostHog can block ~10s (plus retries). Consider applying the same `Promise.race` timeout pattern (or other bounded timeout) here to avoid slow invite resolution when PostHog is degraded.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
E2E Tests: n8n tests passed after 4m 17.7s Run Details
This message was posted automatically by
currents.dev | Integration Settings
|
…ignupToken-to-use-jwt
guillaumejacquart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 comment, otherwise LGTM
Summary
This PR updates resolveSignupToken to use the new JWT for validating invite urls IF the feature is enabled
Related Linear tickets, Github issues, and Community forum posts
PAY-4392
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)