Skip to content

Commit f07f39f

Browse files
fix(auth): pr #194 review remediation (p0-p2)
P0: Extract shared `validateNext` to `src/lib/auth-redirect.ts` so the OAuth callback route and auth server actions cannot drift — normalizes `next` via `URL.pathname` (defeats `/aluno/../admin` traversal), enforces path-segment boundaries (`/aluno$` ok, `/alunox` rejected), and blocks protocol-relative `//` open redirects. Add Sentry.captureException on every auth failure path (magic-link + 3 OAuth providers + callback exchange). Add Zod email validation in `signInWithMagicLink`. P1: Wrap magic-link submit in try/finally so `setMagicLinkLoading(false)` always runs even if the server action throws (button no longer stuck disabled). Stack admin social buttons (drop `sm:flex-row`) — `max-w-sm` card (384px) cannot fit 3 long-label buttons in a row without overflow. P2: Dedup 3 OAuth handlers per login page into `handleOAuthSignIn` / `handleOAuthLogin` helpers; admin variant clears stale password error via `setError('')` before OAuth attempt. Dedup 3 `signInWith{Google,GitHub,Apple}` server actions into shared `signInWithOAuth(provider, next)`. Rename stale "Aegis Fitness OS" template brand to "Five Star Gym" in DESIGN.md. Fix plan doc P01 radius contradiction (spec standardizes `rounded-md`, not `rounded-full`) and add `npm run e2e` to the gate command. Tests: 1151 pass, 0 fail. Typecheck clean. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent fcb1081 commit f07f39f

7 files changed

Lines changed: 104 additions & 98 deletions

File tree

DESIGN.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
# Aegis Fitness OS - Design System
1+
# Five Star Gym - Design System
22

3-
This document outlines the master visual identity for **Aegis Fitness OS**, a next-generation gym and personal training platform.
3+
This document outlines the master visual identity for **Five Star Gym**, a next-generation gym and personal training platform.
44

55
## Design Philosophy
66

docs/superpowers/plans/2026-07-07-aluno-ui-10-fixes.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,12 @@
3636

3737
| ID | File | Change |
3838
|---|---|---|
39-
| P01 | `button.tsx:8` | `rounded-md``rounded-full` |
39+
| P01 | `button.tsx:8` | ~~`rounded-md``rounded-full`~~ Reverted: DESIGN.md standardizes buttons on `rounded-md`; `rounded-full` contradicted spec. Kept `rounded-md`. |
4040
| P02 | `DESIGN.md` | document Inter+Outfit split |
4141

4242
---
4343

4444
## Gates
4545
```
46-
npm run lint && npm run typecheck && npm test
46+
npm run lint && npm run typecheck && npm test && npm run e2e
4747
```

src/app/aluno/login/page.tsx

Lines changed: 14 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -108,35 +108,19 @@ export default function AlunoLoginPage() {
108108
}
109109
};
110110

111-
const handleGoogleSignIn = async () => {
111+
const handleOAuthSignIn = async (action: () => Promise<{ error?: string }>) => {
112112
setAuthError(null);
113-
const result = await signInWithGoogle('/aluno/dashboard');
113+
const result = await action();
114114
if (result.error?.startsWith('http')) {
115115
window.location.href = result.error;
116116
} else if (result.error) {
117117
setAuthError(result.error);
118118
}
119119
};
120120

121-
const handleGitHubSignIn = async () => {
122-
setAuthError(null);
123-
const result = await signInWithGitHub('/aluno/dashboard');
124-
if (result.error?.startsWith('http')) {
125-
window.location.href = result.error;
126-
} else if (result.error) {
127-
setAuthError(result.error);
128-
}
129-
};
130-
131-
const handleAppleSignIn = async () => {
132-
setAuthError(null);
133-
const result = await signInWithApple('/aluno/dashboard');
134-
if (result.error?.startsWith('http')) {
135-
window.location.href = result.error;
136-
} else if (result.error) {
137-
setAuthError(result.error);
138-
}
139-
};
121+
const handleGoogleSignIn = () => handleOAuthSignIn(() => signInWithGoogle('/aluno/dashboard'));
122+
const handleGitHubSignIn = () => handleOAuthSignIn(() => signInWithGitHub('/aluno/dashboard'));
123+
const handleAppleSignIn = () => handleOAuthSignIn(() => signInWithApple('/aluno/dashboard'));
140124

141125
const handleMagicLinkSubmit = async () => {
142126
if (!magicLinkEmail) return;
@@ -147,14 +131,16 @@ export default function AlunoLoginPage() {
147131
const formData = new FormData();
148132
formData.append('email', magicLinkEmail);
149133

150-
const result = await signInWithMagicLink(formData);
151-
152-
if (result.error) {
153-
setMagicLinkError(result.error);
154-
} else {
155-
setMagicLinkSent(true);
134+
try {
135+
const result = await signInWithMagicLink(formData);
136+
if (result.error) {
137+
setMagicLinkError(result.error);
138+
} else {
139+
setMagicLinkSent(true);
140+
}
141+
} finally {
142+
setMagicLinkLoading(false);
156143
}
157-
setMagicLinkLoading(false);
158144
};
159145

160146
return (

src/app/auth/callback/route.ts

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1+
import * as Sentry from '@sentry/nextjs';
12
import { createServerClient } from '@supabase/ssr';
23
import { type NextRequest, NextResponse } from 'next/server';
4+
import { validateNext } from '@/lib/auth-redirect';
35

46
export async function GET(request: NextRequest) {
57
const supabaseUrl = process.env.NEXT_PUBLIC_SUPABASE_URL;
@@ -16,19 +18,20 @@ export async function GET(request: NextRequest) {
1618
const code = searchParams.get('code');
1719
const next = searchParams.get('next') ?? '/login';
1820

19-
// Open redirect prevention: only allow paths under /dashboard or /aluno
20-
const validatedNext =
21-
next.startsWith('/dashboard') || next.startsWith('/aluno') ? next : '/login';
21+
// Open-redirect prevention: single shared helper so this route and the auth
22+
// server actions cannot drift. validateNext rejects protocol-relative
23+
// (`//`), path traversal (`/aluno/../admin`), and sibling-prefix spoofing
24+
// (`/alunox`). Falls back to '/login' for OAuth callback context.
25+
const validatedRoot = validateNext(next);
26+
const validatedNext = validatedRoot === '/' ? '/login' : validatedRoot;
2227

23-
// Missing code — cannot proceed with exchange
2428
if (!code) {
2529
const errorUrl = request.nextUrl.clone();
2630
errorUrl.pathname = '/login';
2731
errorUrl.searchParams.set('error', 'auth_callback_failed');
2832
return NextResponse.redirect(errorUrl);
2933
}
3034

31-
// Build the success redirect early so cookies.setAll can attach to it
3235
const redirectUrl = request.nextUrl.clone();
3336
redirectUrl.pathname = validatedNext;
3437
redirectUrl.search = '';
@@ -54,6 +57,11 @@ export async function GET(request: NextRequest) {
5457
const { error } = await supabase.auth.exchangeCodeForSession(code);
5558

5659
if (error) {
60+
// Capture before redirect so auth failures are observable — a bare
61+
// redirect here silently drops the Supabase exchange error.
62+
Sentry.captureException(error, {
63+
extra: { callbackUrl: request.url, next },
64+
});
5765
const errorUrl = request.nextUrl.clone();
5866
errorUrl.pathname = '/login';
5967
errorUrl.searchParams.set('error', 'auth_callback_failed');

src/app/login/page.tsx

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -57,32 +57,19 @@ export default function LoginPage() {
5757
window.location.href = '/dashboard';
5858
};
5959

60-
const handleGoogleLogin = async () => {
61-
const result = await signInWithGoogle('/dashboard');
62-
if (result.error?.startsWith('http')) {
63-
window.location.href = result.error;
64-
} else if (result.error) {
65-
setError(result.error);
66-
}
67-
};
68-
69-
const handleGitHubLogin = async () => {
70-
const result = await signInWithGitHub('/dashboard');
60+
const handleOAuthLogin = async (action: () => Promise<{ error?: string }>) => {
61+
setError('');
62+
const result = await action();
7163
if (result.error?.startsWith('http')) {
7264
window.location.href = result.error;
7365
} else if (result.error) {
7466
setError(result.error);
7567
}
7668
};
7769

78-
const handleAppleLogin = async () => {
79-
const result = await signInWithApple('/dashboard');
80-
if (result.error?.startsWith('http')) {
81-
window.location.href = result.error;
82-
} else if (result.error) {
83-
setError(result.error);
84-
}
85-
};
70+
const handleGoogleLogin = () => handleOAuthLogin(() => signInWithGoogle('/dashboard'));
71+
const handleGitHubLogin = () => handleOAuthLogin(() => signInWithGitHub('/dashboard'));
72+
const handleAppleLogin = () => handleOAuthLogin(() => signInWithApple('/dashboard'));
8673

8774
return (
8875
<div className="relative flex min-h-dvh flex-col items-center justify-center overflow-hidden bg-background px-4">
@@ -172,7 +159,7 @@ export default function LoginPage() {
172159
</div>
173160

174161
{/* Social login buttons */}
175-
<div className="flex flex-col sm:flex-row gap-2">
162+
<div className="flex flex-col gap-2">
176163
<Button
177164
type="button"
178165
variant="outline"

src/lib/actions/auth.ts

Lines changed: 37 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,78 +1,74 @@
11
'use server';
22

3+
import * as Sentry from '@sentry/nextjs';
4+
import { z } from 'zod/v4';
35
import { createClient } from '@/utils/supabase/server';
46
import { AUTH_CALLBACK_PATH } from '@/lib/constants';
7+
import { validateNext } from '@/lib/auth-redirect';
58

69
const appUrl = process.env.NEXT_PUBLIC_APP_URL || 'http://localhost:3001';
710

8-
function validateNext(next: string | null): string {
9-
const allowedPrefixes = ['/dashboard', '/aluno'] as const;
10-
if (next && allowedPrefixes.some((p) => next.startsWith(p)) && !next.startsWith('//')) {
11-
return next;
12-
}
13-
return '/login';
14-
}
15-
1611
function callbackUrl(next?: string | null): string {
17-
const safeNext = validateNext(next ?? null);
12+
const validated = validateNext(next);
13+
// validateNext's inert fallback is '/'; for the OAuth callback we need a
14+
// concrete post-login destination, so map the inert '/' → '/login' the same
15+
// way callback/route.ts does.
16+
const safeNext = validated === '/' ? '/login' : validated;
1817
return `${appUrl}${AUTH_CALLBACK_PATH}?next=${encodeURIComponent(safeNext)}`;
1918
}
2019

20+
const MagicLinkSchema = z.object({
21+
email: z.email({ error: 'Email is required' }),
22+
});
23+
2124
export async function signInWithMagicLink(formData: FormData): Promise<{ error?: string }> {
22-
const email = formData.get('email') as string;
23-
if (!email) return { error: 'Email is required' };
25+
const parsed = MagicLinkSchema.safeParse({ email: formData.get('email') });
26+
if (!parsed.success) return { error: parsed.error.issues[0]?.message ?? 'Email is required' };
2427

2528
const supabase = await createClient();
2629
const { error } = await supabase.auth.signInWithOtp({
27-
email,
30+
email: parsed.data.email,
2831
options: {
2932
emailRedirectTo: callbackUrl('/aluno/dashboard'),
3033
shouldCreateUser: true,
3134
},
3235
});
3336

34-
if (error) return { error: error.message };
37+
if (error) {
38+
Sentry.captureException(error, { tags: { auth: 'magic-link' } });
39+
return { error: error.message };
40+
}
3541
return {};
3642
}
3743

38-
export async function signInWithGoogle(next?: string): Promise<{ error?: string }> {
44+
/**
45+
* Shared OAuth sign-in. `data.url` is the provider authorize URL; tunnel it
46+
* through `error` (existing public API contract) so the client can
47+
* `window.location.href = result.error` when it starts with `http`.
48+
*/
49+
async function signInWithOAuth(provider: 'google' | 'github' | 'apple', next?: string) {
3950
const supabase = await createClient();
4051
const { data, error } = await supabase.auth.signInWithOAuth({
41-
provider: 'google',
42-
options: {
43-
redirectTo: callbackUrl(next),
44-
},
52+
provider,
53+
options: { redirectTo: callbackUrl(next) },
4554
});
4655

47-
if (error) return { error: error.message };
56+
if (error) {
57+
Sentry.captureException(error, { tags: { auth: provider } });
58+
return { error: error.message };
59+
}
4860
if (data.url) return { error: data.url };
4961
return { error: 'No redirect URL returned' };
5062
}
5163

52-
export async function signInWithGitHub(next?: string): Promise<{ error?: string }> {
53-
const supabase = await createClient();
54-
const { data, error } = await supabase.auth.signInWithOAuth({
55-
provider: 'github',
56-
options: {
57-
redirectTo: callbackUrl(next),
58-
},
59-
});
64+
export async function signInWithGoogle(next?: string): Promise<{ error?: string }> {
65+
return signInWithOAuth('google', next);
66+
}
6067

61-
if (error) return { error: error.message };
62-
if (data.url) return { error: data.url };
63-
return { error: 'No redirect URL returned' };
68+
export async function signInWithGitHub(next?: string): Promise<{ error?: string }> {
69+
return signInWithOAuth('github', next);
6470
}
6571

6672
export async function signInWithApple(next?: string): Promise<{ error?: string }> {
67-
const supabase = await createClient();
68-
const { data, error } = await supabase.auth.signInWithOAuth({
69-
provider: 'apple',
70-
options: {
71-
redirectTo: callbackUrl(next),
72-
},
73-
});
74-
75-
if (error) return { error: error.message };
76-
if (data.url) return { error: data.url };
77-
return { error: 'No redirect URL returned' };
73+
return signInWithOAuth('apple', next);
7874
}

src/lib/auth-redirect.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/**
2+
* Shared open-redirect prevention for post-login `next` params.
3+
*
4+
* Used by both the OAuth callback route (src/app/auth/callback/route.ts) and
5+
* the auth server actions (src/lib/actions/auth.ts) so the two paths cannot
6+
* drift — every redirect target is validated by the same logic.
7+
*
8+
* Guards:
9+
* - Only allowlisted prefixes (`/dashboard`, `/aluno`).
10+
* - Path-segment boundary: `/aluno$` or `/aluno/...` pass, `/alunox` fails.
11+
* - No protocol-relative (`//evil.com` → `evil.com`) open redirects.
12+
* - Normalizes `next` via URL.pathname so `/aluno/../admin` collapses to
13+
* `/admin` *before* the prefix check, defeating traversal.
14+
*
15+
* Returns the safe target or `'/'` as the inert fallback. Callers that need a
16+
* login-redirect default can map `'/' → '/login'` at the boundary.
17+
*/
18+
const ALLOWED_PREFIXES = ['/dashboard', '/aluno'] as const;
19+
20+
export function validateNext(next: string | null | undefined): string {
21+
if (!next) return '/';
22+
if (next.startsWith('//')) return '/';
23+
24+
// Normalize so `/aluno/../admin` collapses to `/admin` before the prefix
25+
// check, defeating path traversal bypasses.
26+
const normalized = new URL(next, 'http://localhost').pathname;
27+
const safe = ALLOWED_PREFIXES.some((p) => normalized === p || normalized.startsWith(`${p}/`));
28+
return safe ? normalized : '/';
29+
}

0 commit comments

Comments
 (0)