-
Notifications
You must be signed in to change notification settings - Fork 460
fix(ui): remove ticket parameters on sign up continue #9255
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@clerk/ui': patch | ||
| --- | ||
|
|
||
| Fix an issue where Clerk's ticket query parameters were not removed from the URL when completing a sign-up that was missing requirements. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import { CardStateProvider } from '@/ui/elements/contexts'; | |
|
|
||
| import { OptionsProvider } from '../../../contexts'; | ||
| import { AppearanceProvider } from '../../../customizables'; | ||
| import { SignUpContinue } from '../SignUpContinue'; | ||
| import { SignUpStart } from '../SignUpStart'; | ||
|
|
||
| const { createFixtures } = bindCreateFixtures('SignUp'); | ||
|
|
@@ -497,6 +498,77 @@ describe('SignUpStart', () => { | |
| ); | ||
| }); | ||
|
|
||
| it('removes the ticket before setting the session active after continuing the sign up', async () => { | ||
| const { wrapper, fixtures } = await createFixtures(f => { | ||
| f.withEmailAddress({ required: true }); | ||
| f.withPassword({ required: true }); | ||
| f.startSignUpWithEmailAddress({ emailVerificationStatus: 'verified' }); | ||
| }); | ||
| let currentUrl = new URL('http://localhost/sign-up?__clerk_ticket=test_ticket'); | ||
| let ticketAtSetActive: string | null | undefined; | ||
|
|
||
| Object.defineProperty(window, 'location', { | ||
| configurable: true, | ||
| value: { | ||
| get href() { | ||
| return currentUrl.href; | ||
| }, | ||
| get search() { | ||
| return currentUrl.search; | ||
| }, | ||
| }, | ||
| }); | ||
| Object.defineProperty(window, 'history', { | ||
| configurable: true, | ||
| value: { | ||
| state: undefined, | ||
| replaceState: vi.fn((_state, _title, url) => { | ||
| currentUrl = new URL(url, currentUrl); | ||
| }), | ||
| }, | ||
| }); | ||
|
Comment on lines
+510
to
+529
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check for cleanup/restoration of window.location or window.history in this test file
rg -n "afterEach|afterAll|beforeEach" -A5 packages/ui/src/components/SignUp/__tests__/SignUpStart.test.tsx
rg -n "window.location|window.history" -B2 -A2 packages/ui/src/components/SignUp/__tests__/SignUpStart.test.tsxRepository: clerk/javascript Length of output: 1862 🏁 Script executed: #!/bin/bash
sed -n '500,640p' packages/ui/src/components/SignUp/__tests__/SignUpStart.test.tsxRepository: clerk/javascript Length of output: 5422 🏁 Script executed: #!/bin/bash
rg -n "afterEach|afterAll|beforeEach|window\.history|window\.location" packages/ui/src/components/SignUp/__tests__/SignUpStart.test.tsxRepository: clerk/javascript Length of output: 652 Restore the 🤖 Prompt for AI Agents |
||
|
|
||
| fixtures.signUp.create.mockResolvedValueOnce(fixtures.signUp as SignUpResource); | ||
| fixtures.router.navigate.mockImplementation((to, options) => { | ||
| currentUrl = new URL(`/sign-up/${to}`, currentUrl); | ||
| currentUrl.search = options?.searchParams?.toString() || ''; | ||
| return Promise.resolve(true); | ||
| }); | ||
| fixtures.signUp.update.mockImplementationOnce(() => { | ||
| fixtures.signUp.status = 'complete'; | ||
| fixtures.signUp.createdSessionId = 'sess_ticket'; | ||
| return Promise.resolve(fixtures.signUp); | ||
| }); | ||
| fixtures.clerk.setActive.mockImplementationOnce(() => { | ||
| ticketAtSetActive = new URL(window.location.href).searchParams.get('__clerk_ticket'); | ||
| return Promise.resolve(); | ||
| }); | ||
|
|
||
| const { rerender, userEvent } = render(<SignUpStart />, { wrapper }); | ||
|
|
||
| await waitFor(() => | ||
| expect(fixtures.signUp.create).toHaveBeenCalledWith({ | ||
| strategy: 'ticket', | ||
| ticket: 'test_ticket', | ||
| unsafeMetadata: undefined, | ||
| }), | ||
| ); | ||
| await waitFor(() => | ||
| expect(fixtures.router.navigate).toHaveBeenCalledWith('continue', { | ||
| searchParams: new URLSearchParams('__clerk_ticket=test_ticket'), | ||
| }), | ||
| ); | ||
| expect(currentUrl.href).toBe('http://localhost/sign-up/continue?__clerk_ticket=test_ticket'); | ||
|
|
||
| rerender(<SignUpContinue />); | ||
| await userEvent.type(screen.getByLabelText('Password'), 'a-secure-password'); | ||
| await userEvent.click(screen.getByText('Continue')); | ||
|
|
||
| await waitFor(() => expect(fixtures.signUp.update).toHaveBeenCalled()); | ||
| await waitFor(() => expect(fixtures.clerk.setActive).toHaveBeenCalled()); | ||
| expect(ticketAtSetActive).toBeNull(); | ||
| }); | ||
|
|
||
| it('should show the sign up form when ticket detected and mode is restricted', async () => { | ||
| const { wrapper, fixtures } = await createFixtures(f => { | ||
| f.withEmailAddress(); | ||
|
|
||
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.
This cleanup only runs when
signUp.update()completes immediately.completeSignUpFlowforwards the ticket into email/phone verification and protect-check routes, whose terminal handlers callsetActivewithout removing it, so those flows can still remount and attempt to consume an already-used ticket. Please apply the cleanup to every terminal sign-up completion path (preferably via a shared helper) and cover a continue → verification → activation case.~ 🤖
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.
I was a bit nervous to impact ~all the sign up routes, but I feel like removing the tickets universally should be safe. So I updated to move the removal logic into
completeSignUpinstead of forcing each caller to individually remove the parameters.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.
We don't HAVE to, I just wanted to call it out. Feel free to ignore for now.