diff --git a/apps/web/src/app/(app)/gastown/onboarding/OnboardingStepRepo.tsx b/apps/web/src/app/(app)/gastown/onboarding/OnboardingStepRepo.tsx index e0f4b67fb6..9e2b22ec4c 100644 --- a/apps/web/src/app/(app)/gastown/onboarding/OnboardingStepRepo.tsx +++ b/apps/web/src/app/(app)/gastown/onboarding/OnboardingStepRepo.tsx @@ -1,7 +1,9 @@ 'use client'; -import { useState, useMemo, useCallback } from 'react'; +import { useState, useMemo, useCallback, useEffect } from 'react'; import { useQuery } from '@tanstack/react-query'; +import { useSearchParams } from 'next/navigation'; +import { toast } from 'sonner'; import { useTRPC } from '@/lib/trpc/utils'; import { useUser } from '@/hooks/useUser'; import { RepositoryCombobox, type RepositoryOption } from '@/components/shared/RepositoryCombobox'; @@ -63,11 +65,23 @@ export function OnboardingStepRepo() { const githubAppName = process.env.NEXT_PUBLIC_GITHUB_APP_NAME || 'KiloConnect'; const handleInstallGithub = useCallback(() => { - const installState = orgId ? `org_${orgId}` : `user_${user?.id}`; - const installUrl = `https://github.com/apps/${githubAppName}/installations/new?state=${installState}`; - window.open(installUrl, '_blank', 'noopener'); + const owner = orgId ? `org_${orgId}` : `user_${user?.id}`; + const returnPath = `/gastown/onboarding?step=repo${orgId ? `&orgId=${orgId}` : ''}`; + const state = `${owner}|return=${encodeURIComponent(returnPath)}`; + const installUrl = `https://github.com/apps/${githubAppName}/installations/new?state=${encodeURIComponent(state)}`; + window.location.href = installUrl; }, [orgId, user?.id, githubAppName]); + const githubInstallParam = useSearchParams().get('github_install'); + const { refetch: refetchGithubRepos } = githubReposQuery; + + useEffect(() => { + if (githubInstallParam === 'success') { + refetchGithubRepos(); + toast.success('GitHub app installed. Select a repo to continue.'); + } + }, [githubInstallParam, refetchGithubRepos]); + const handleRepoSelect = useCallback( (fullName: string) => { setSelectedRepoFullName(fullName); diff --git a/apps/web/src/app/(app)/gastown/onboarding/OnboardingWizardClient.tsx b/apps/web/src/app/(app)/gastown/onboarding/OnboardingWizardClient.tsx index 22cc538436..889c71a7bf 100644 --- a/apps/web/src/app/(app)/gastown/onboarding/OnboardingWizardClient.tsx +++ b/apps/web/src/app/(app)/gastown/onboarding/OnboardingWizardClient.tsx @@ -20,6 +20,8 @@ const STEPS = [ type StepKey = (typeof STEPS)[number]['key']; +const VALID_STEP_KEYS = new Set(STEPS.map(s => s.key)); + function StepIndicator({ currentIndex }: { currentIndex: number }) { return (
@@ -146,7 +148,16 @@ function CancelButton() { function WizardContent() { const searchParams = useSearchParams(); const orgId = searchParams.get('orgId'); - const [currentStepKey, setCurrentStepKey] = useState('name'); + + const initialStep: StepKey = (() => { + const stepParam = searchParams.get('step'); + if (stepParam && VALID_STEP_KEYS.has(stepParam)) { + return stepParam as StepKey; + } + return 'name'; + })(); + + const [currentStepKey, setCurrentStepKey] = useState(initialStep); const currentIndex = STEPS.findIndex(s => s.key === currentStepKey); diff --git a/apps/web/src/app/api/integrations/github/callback/route.ts b/apps/web/src/app/api/integrations/github/callback/route.ts index d5ff99be02..45598327c2 100644 --- a/apps/web/src/app/api/integrations/github/callback/route.ts +++ b/apps/web/src/app/api/integrations/github/callback/route.ts @@ -19,6 +19,7 @@ import type { IntegrationPermissions, Owner, } from '@/lib/integrations/core/types'; +import { parseStateReturn } from '@/lib/integrations/validate-return-path'; import { captureException, captureMessage } from '@sentry/nextjs'; /** @@ -40,23 +41,25 @@ export async function GET(request: NextRequest) { const searchParams = request.nextUrl.searchParams; const installationId = searchParams.get('installation_id') ?? ''; const setupAction = searchParams.get('setup_action'); - const state = searchParams.get('state'); // Contains owner info (org_ID or user_ID) + const rawState = searchParams.get('state'); + + // 3. Parse owner from state (with optional |return= suffix) + const { ownerToken, returnTo } = parseStateReturn(rawState); - // 3. Parse owner from state let owner: Owner; let ownerId: string; - if (state?.startsWith('org_')) { - ownerId = state.replace('org_', ''); + if (ownerToken.startsWith('org_')) { + ownerId = ownerToken.slice(4); owner = { type: 'org', id: ownerId }; - } else if (state?.startsWith('user_')) { - ownerId = state.replace('user_', ''); + } else if (ownerToken.startsWith('user_')) { + ownerId = ownerToken.slice(5); owner = { type: 'user', id: ownerId }; } else { captureMessage('GitHub callback missing or invalid owner in state', { level: 'warning', tags: { endpoint: 'github/callback', source: 'github_app_installation' }, - extra: { installationId, state, allParams: Object.fromEntries(searchParams.entries()) }, + extra: { installationId, rawState, allParams: Object.fromEntries(searchParams.entries()) }, }); return NextResponse.redirect(new URL('/', request.url)); } @@ -172,7 +175,7 @@ export async function GET(request: NextRequest) { captureMessage('GitHub callback missing installation_id', { level: 'warning', tags: { endpoint: 'github/callback', source: 'github_app_installation' }, - extra: { setupAction, state, allParams: Object.fromEntries(searchParams.entries()) }, + extra: { setupAction, rawState, allParams: Object.fromEntries(searchParams.entries()) }, }); const redirectPath = @@ -291,8 +294,9 @@ export async function GET(request: NextRequest) { } // 9. Redirect to success page - const successPath = - owner.type === 'org' + const successPath = returnTo + ? `${returnTo}${returnTo.includes('?') ? '&' : '?'}github_install=success` + : owner.type === 'org' ? `/organizations/${owner.id}/integrations/github?success=installed` : `/integrations/github?success=installed`; @@ -302,7 +306,7 @@ export async function GET(request: NextRequest) { // Capture error to Sentry with context for debugging const searchParams = request.nextUrl.searchParams; - const state = searchParams.get('state'); + const rawState = searchParams.get('state'); captureException(error, { tags: { @@ -312,17 +316,18 @@ export async function GET(request: NextRequest) { extra: { installationId: searchParams.get('installation_id'), setupAction: searchParams.get('setup_action'), - state, + rawState, }, }); - // Determine redirect path based on state parameter + const { ownerToken: errorOwnerToken } = parseStateReturn(rawState); + let redirectPath = '/?error=installation_failed'; - if (state?.startsWith('org_')) { - const orgId = state.replace('org_', ''); + if (errorOwnerToken.startsWith('org_')) { + const orgId = errorOwnerToken.slice(4); redirectPath = `/organizations/${orgId}/integrations/github?error=installation_failed`; - } else if (state?.startsWith('user_')) { + } else if (errorOwnerToken.startsWith('user_')) { redirectPath = `/integrations/github?error=installation_failed`; } diff --git a/apps/web/src/lib/integrations/validate-return-path.test.ts b/apps/web/src/lib/integrations/validate-return-path.test.ts new file mode 100644 index 0000000000..ffd238c210 --- /dev/null +++ b/apps/web/src/lib/integrations/validate-return-path.test.ts @@ -0,0 +1,110 @@ +import { validateReturnPath, parseStateReturn } from './validate-return-path'; + +describe('validateReturnPath', () => { + it('accepts a simple internal path', () => { + expect(validateReturnPath('/gastown/onboarding')).toBe('/gastown/onboarding'); + }); + + it('accepts a path with query params', () => { + expect(validateReturnPath('/gastown/onboarding?step=repo&orgId=123')).toBe( + '/gastown/onboarding?step=repo&orgId=123' + ); + }); + + it('rejects protocol-relative URLs', () => { + expect(validateReturnPath('//evil.com')).toBeNull(); + }); + + it('rejects absolute URLs', () => { + expect(validateReturnPath('https://evil.com')).toBeNull(); + }); + + it('rejects backslash-prefixed paths', () => { + expect(validateReturnPath('/\\evil.com')).toBeNull(); + }); + + it('rejects paths with carriage return', () => { + expect(validateReturnPath('/foo\rbar')).toBeNull(); + }); + + it('rejects paths with newline', () => { + expect(validateReturnPath('/foo\nbar')).toBeNull(); + }); + + it('rejects paths without leading slash', () => { + expect(validateReturnPath('foo/bar')).toBeNull(); + }); + + it('rejects empty string', () => { + expect(validateReturnPath('')).toBeNull(); + }); + + it('accepts root path', () => { + expect(validateReturnPath('/')).toBe('/'); + }); + + it('rejects triple-slash paths', () => { + expect(validateReturnPath('///foo')).toBeNull(); + }); +}); + +describe('parseStateReturn', () => { + it('parses state with return suffix', () => { + const encoded = encodeURIComponent('/gastown/onboarding?step=repo'); + const result = parseStateReturn(`user_abc|return=${encoded}`); + expect(result).toEqual({ + ownerToken: 'user_abc', + returnTo: '/gastown/onboarding?step=repo', + }); + }); + + it('parses org state with return suffix', () => { + const encoded = encodeURIComponent('/gastown/onboarding?step=repo&orgId=123'); + const result = parseStateReturn(`org_123|return=${encoded}`); + expect(result).toEqual({ + ownerToken: 'org_123', + returnTo: '/gastown/onboarding?step=repo&orgId=123', + }); + }); + + it('parses state without return suffix (backwards compat)', () => { + const result = parseStateReturn('user_abc'); + expect(result).toEqual({ + ownerToken: 'user_abc', + returnTo: null, + }); + }); + + it('parses org state without return suffix', () => { + const result = parseStateReturn('org_123'); + expect(result).toEqual({ + ownerToken: 'org_123', + returnTo: null, + }); + }); + + it('returns null returnTo when return path is invalid', () => { + const encoded = encodeURIComponent('//evil.com'); + const result = parseStateReturn(`user_abc|return=${encoded}`); + expect(result).toEqual({ + ownerToken: 'user_abc', + returnTo: null, + }); + }); + + it('handles null state', () => { + const result = parseStateReturn(null); + expect(result).toEqual({ + ownerToken: '', + returnTo: null, + }); + }); + + it('returns null returnTo when return suffix has malformed percent-encoding', () => { + const result = parseStateReturn('user_abc|return=%ZZ'); + expect(result).toEqual({ + ownerToken: 'user_abc', + returnTo: null, + }); + }); +}); diff --git a/apps/web/src/lib/integrations/validate-return-path.ts b/apps/web/src/lib/integrations/validate-return-path.ts new file mode 100644 index 0000000000..e0cc67008c --- /dev/null +++ b/apps/web/src/lib/integrations/validate-return-path.ts @@ -0,0 +1,31 @@ +const RETURN_PATH_RE = /^\/(?![\/\\])[^\r\n]*$/; + +export function validateReturnPath(candidate: string): string | null { + if (!RETURN_PATH_RE.test(candidate) || candidate.startsWith('//')) { + return null; + } + return candidate; +} + +export function parseStateReturn(rawState: string | null): { + ownerToken: string; + returnTo: string | null; +} { + let ownerToken = rawState ?? ''; + let returnTo: string | null = null; + + if (rawState) { + const sepIdx = rawState.indexOf('|return='); + if (sepIdx !== -1) { + ownerToken = rawState.slice(0, sepIdx); + try { + const candidate = decodeURIComponent(rawState.slice(sepIdx + '|return='.length)); + returnTo = validateReturnPath(candidate); + } catch { + returnTo = null; + } + } + } + + return { ownerToken, returnTo }; +}