mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-22 10:33:27 +00:00
fix: resolve 2FA Codex review findings (Postgres bools, recovery code race, web login flow)
- Use dialect.BoolToInt() for totp_enabled updates instead of hardcoded 1/0 integers that fail on PostgreSQL BOOLEAN columns - Add optimistic locking to ConsumeRecoveryCode to prevent double-spend under concurrent requests - Add 2FA challenge step to web login and join pages so browser login works for accounts with TOTP enabled
This commit is contained in:
xarmian
+14
-6
@@ -194,9 +194,9 @@ func (s *Store) SetTOTPSecret(userID, secret string) error {
|
||||
// TOCTOU races (e.g., a concurrent setup call overwriting the secret).
|
||||
func (s *Store) EnableTOTP(userID, expectedSecret, hashedRecoveryCodes string) error {
|
||||
result, err := s.db.Exec(s.q(
|
||||
`UPDATE users SET totp_enabled = 1, recovery_codes = ?, updated_at = ?
|
||||
`UPDATE users SET totp_enabled = ?, recovery_codes = ?, updated_at = ?
|
||||
WHERE id = ? AND totp_secret = ?`),
|
||||
hashedRecoveryCodes, now(), userID, expectedSecret)
|
||||
s.dialect.BoolToInt(true), hashedRecoveryCodes, now(), userID, expectedSecret)
|
||||
if err != nil {
|
||||
return fmt.Errorf("enable totp: %w", err)
|
||||
}
|
||||
@@ -209,8 +209,8 @@ func (s *Store) EnableTOTP(userID, expectedSecret, hashedRecoveryCodes string) e
|
||||
|
||||
// DisableTOTP disables 2FA and clears the secret and recovery codes.
|
||||
func (s *Store) DisableTOTP(userID string) error {
|
||||
_, err := s.db.Exec(s.q(`UPDATE users SET totp_enabled = 0, totp_secret = '', recovery_codes = '', updated_at = ? WHERE id = ?`),
|
||||
now(), userID)
|
||||
_, err := s.db.Exec(s.q(`UPDATE users SET totp_enabled = ?, totp_secret = '', recovery_codes = '', updated_at = ? WHERE id = ?`),
|
||||
s.dialect.BoolToInt(false), now(), userID)
|
||||
if err != nil {
|
||||
return fmt.Errorf("disable totp: %w", err)
|
||||
}
|
||||
@@ -260,10 +260,18 @@ func (s *Store) ConsumeRecoveryCode(userID, code string) (bool, error) {
|
||||
return false, nil
|
||||
}
|
||||
|
||||
_, err = tx.Exec(s.q(`UPDATE users SET recovery_codes = ?, updated_at = ? WHERE id = ?`),
|
||||
strings.Join(remaining, "\n"), now(), userID)
|
||||
// Use optimistic locking: include the original recovery_codes in the WHERE
|
||||
// clause so a concurrent transaction that already consumed a code will cause
|
||||
// this UPDATE to match 0 rows, preventing double-spend.
|
||||
result, err := tx.Exec(s.q(`UPDATE users SET recovery_codes = ?, updated_at = ? WHERE id = ? AND recovery_codes = ?`),
|
||||
strings.Join(remaining, "\n"), now(), userID, recoveryCodes)
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("consume recovery code: %w", err)
|
||||
}
|
||||
n, _ := result.RowsAffected()
|
||||
if n == 0 {
|
||||
// Another request consumed or modified the codes concurrently
|
||||
return false, nil
|
||||
}
|
||||
return true, tx.Commit()
|
||||
}
|
||||
|
||||
@@ -107,6 +107,13 @@ export interface AuthSession {
|
||||
user?: { id: string; email: string; name: string; role: string };
|
||||
}
|
||||
|
||||
export interface LoginResponse {
|
||||
user?: { id: string; email: string; name: string; role: string };
|
||||
token?: string;
|
||||
requires_2fa?: boolean;
|
||||
challenge_token?: string;
|
||||
}
|
||||
|
||||
export const api = {
|
||||
// ── Health / Version ──────────────────────────────────────────────────────
|
||||
|
||||
@@ -503,10 +510,15 @@ export const api = {
|
||||
auth: {
|
||||
session: (): Promise<AuthSession> => fetch(BASE + '/auth/session', { credentials: 'same-origin' }).then((r) => r.json()),
|
||||
login: (email: string, password: string) =>
|
||||
request<{ user: { id: string; email: string; name: string; role: string }; token: string }>('/auth/login', {
|
||||
request<LoginResponse>('/auth/login', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({ email, password })
|
||||
}),
|
||||
verify2FA: (challengeToken: string, code?: string, recoveryCode?: string) =>
|
||||
request<{ user: { id: string; email: string; name: string; role: string }; token: string }>('/auth/2fa/login-verify', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({ challenge_token: challengeToken, code: code || undefined, recovery_code: recoveryCode || undefined })
|
||||
}),
|
||||
register: (email: string, name: string, password: string, invitation_code?: string) =>
|
||||
request<{ user: { id: string; email: string; name: string; role: string }; token: string }>('/auth/register', {
|
||||
method: 'POST',
|
||||
|
||||
@@ -6,7 +6,7 @@
|
||||
import SetupRequiredNotice from '$lib/components/auth/SetupRequiredNotice.svelte';
|
||||
|
||||
let code = $derived(page.params.code ?? '');
|
||||
let status = $state<'loading' | 'login' | 'register' | 'accepting' | 'error' | 'setup'>('loading');
|
||||
let status = $state<'loading' | 'login' | 'register' | 'accepting' | 'error' | 'setup' | '2fa'>('loading');
|
||||
let errorMsg = $state('');
|
||||
let setupMethod = $state<'local_cli' | 'docker_exec' | 'cloud' | undefined>(undefined);
|
||||
|
||||
@@ -18,6 +18,8 @@
|
||||
let confirmPassword = $state('');
|
||||
let formError = $state('');
|
||||
let submitting = $state(false);
|
||||
let challengeToken = $state('');
|
||||
let totpCode = $state('');
|
||||
|
||||
onMount(async () => {
|
||||
try {
|
||||
@@ -72,7 +74,14 @@
|
||||
} else {
|
||||
if (!email.trim()) { formError = 'Email is required'; submitting = false; return; }
|
||||
if (!password) { formError = 'Password is required'; submitting = false; return; }
|
||||
await api.auth.login(email.trim(), password);
|
||||
const response = await api.auth.login(email.trim(), password);
|
||||
|
||||
if (response.requires_2fa && response.challenge_token) {
|
||||
challengeToken = response.challenge_token;
|
||||
status = '2fa';
|
||||
submitting = false;
|
||||
return;
|
||||
}
|
||||
}
|
||||
// Logged in via login — now accept the invitation
|
||||
await acceptInvitation();
|
||||
@@ -82,8 +91,49 @@
|
||||
}
|
||||
}
|
||||
|
||||
async function handleVerify2FA() {
|
||||
formError = '';
|
||||
const code = totpCode.trim();
|
||||
|
||||
if (!code) {
|
||||
formError = 'Please enter your authentication code.';
|
||||
return;
|
||||
}
|
||||
|
||||
submitting = true;
|
||||
try {
|
||||
const isTotp = /^\d{6}$/.test(code);
|
||||
|
||||
if (isTotp) {
|
||||
await api.auth.verify2FA(challengeToken, code, undefined);
|
||||
} else {
|
||||
await api.auth.verify2FA(challengeToken, undefined, code);
|
||||
}
|
||||
|
||||
// 2FA verified — now accept the invitation
|
||||
await acceptInvitation();
|
||||
} catch (err: unknown) {
|
||||
formError = err instanceof Error ? err.message : 'Invalid code. Please try again.';
|
||||
submitting = false;
|
||||
}
|
||||
}
|
||||
|
||||
function handleBack2FA() {
|
||||
status = 'login';
|
||||
challengeToken = '';
|
||||
totpCode = '';
|
||||
formError = '';
|
||||
submitting = false;
|
||||
}
|
||||
|
||||
function handleKeydown(event: KeyboardEvent) {
|
||||
if (event.key === 'Enter') handleSubmit();
|
||||
if (event.key === 'Enter') {
|
||||
if (status === '2fa') {
|
||||
handleVerify2FA();
|
||||
} else {
|
||||
handleSubmit();
|
||||
}
|
||||
}
|
||||
}
|
||||
</script>
|
||||
|
||||
@@ -105,6 +155,38 @@
|
||||
{:else if status === 'error'}
|
||||
<p class="subtitle error-text">{errorMsg}</p>
|
||||
<a href="/login" class="link">Go to login</a>
|
||||
{:else if status === '2fa'}
|
||||
<p class="subtitle">Two-factor authentication</p>
|
||||
|
||||
<div class="form">
|
||||
<p class="hint">Enter the 6-digit code from your authenticator app, or a recovery code.</p>
|
||||
|
||||
<input
|
||||
type="text"
|
||||
placeholder="Authentication code"
|
||||
bind:value={totpCode}
|
||||
onkeydown={handleKeydown}
|
||||
disabled={submitting}
|
||||
autocomplete="one-time-code"
|
||||
inputmode="numeric"
|
||||
/>
|
||||
|
||||
{#if formError}
|
||||
<p class="error">{formError}</p>
|
||||
{/if}
|
||||
|
||||
<button onclick={handleVerify2FA} disabled={submitting}>
|
||||
{#if submitting}
|
||||
Verifying...
|
||||
{:else}
|
||||
Verify & join
|
||||
{/if}
|
||||
</button>
|
||||
|
||||
<button class="back-button" onclick={handleBack2FA} disabled={submitting} type="button">
|
||||
Back to sign in
|
||||
</button>
|
||||
</div>
|
||||
{:else}
|
||||
<p class="subtitle">You've been invited to a workspace</p>
|
||||
<p class="hint">{mode === 'register' ? 'Create an account' : 'Sign in'} to accept</p>
|
||||
@@ -209,6 +291,7 @@
|
||||
color: var(--text-muted);
|
||||
font-size: 0.85rem;
|
||||
margin-bottom: var(--space-6);
|
||||
line-height: 1.4;
|
||||
}
|
||||
|
||||
.error-text {
|
||||
@@ -259,6 +342,18 @@
|
||||
button:hover:not(:disabled) { opacity: 0.9; }
|
||||
button:disabled { opacity: 0.6; cursor: not-allowed; }
|
||||
|
||||
.back-button {
|
||||
background: transparent;
|
||||
color: var(--text-muted);
|
||||
font-size: 0.85rem;
|
||||
font-weight: 400;
|
||||
padding: var(--space-2) var(--space-4);
|
||||
}
|
||||
.back-button:hover:not(:disabled) {
|
||||
color: var(--text-primary);
|
||||
opacity: 1;
|
||||
}
|
||||
|
||||
.switch-mode {
|
||||
margin-top: var(--space-6);
|
||||
font-size: 0.85rem;
|
||||
|
||||
@@ -12,6 +12,10 @@
|
||||
let setupMethod = $state<'local_cli' | 'docker_exec' | 'cloud' | undefined>(undefined);
|
||||
let loading = $state(false);
|
||||
|
||||
let step = $state<'credentials' | '2fa'>('credentials');
|
||||
let challengeToken = $state('');
|
||||
let totpCode = $state('');
|
||||
|
||||
onMount(async () => {
|
||||
try {
|
||||
const session = await api.auth.session();
|
||||
@@ -40,8 +44,16 @@
|
||||
|
||||
loading = true;
|
||||
try {
|
||||
await api.auth.login(email, password);
|
||||
await authStore.load(); // Refresh global auth state before navigating.
|
||||
const response = await api.auth.login(email, password);
|
||||
|
||||
if (response.requires_2fa && response.challenge_token) {
|
||||
challengeToken = response.challenge_token;
|
||||
step = '2fa';
|
||||
error = '';
|
||||
return;
|
||||
}
|
||||
|
||||
await authStore.load();
|
||||
await goto('/', { replaceState: true });
|
||||
} catch (err: unknown) {
|
||||
if (err instanceof Error) {
|
||||
@@ -54,9 +66,52 @@
|
||||
}
|
||||
}
|
||||
|
||||
async function handleVerify2FA() {
|
||||
error = '';
|
||||
const code = totpCode.trim();
|
||||
|
||||
if (!code) {
|
||||
error = 'Please enter your authentication code.';
|
||||
return;
|
||||
}
|
||||
|
||||
loading = true;
|
||||
try {
|
||||
const isTotp = /^\d{6}$/.test(code);
|
||||
|
||||
if (isTotp) {
|
||||
await api.auth.verify2FA(challengeToken, code, undefined);
|
||||
} else {
|
||||
await api.auth.verify2FA(challengeToken, undefined, code);
|
||||
}
|
||||
|
||||
await authStore.load();
|
||||
await goto('/', { replaceState: true });
|
||||
} catch (err: unknown) {
|
||||
if (err instanceof Error) {
|
||||
error = err.message || 'Invalid code. Please try again.';
|
||||
} else {
|
||||
error = 'Invalid code. Please try again.';
|
||||
}
|
||||
} finally {
|
||||
loading = false;
|
||||
}
|
||||
}
|
||||
|
||||
function handleBack() {
|
||||
step = 'credentials';
|
||||
challengeToken = '';
|
||||
totpCode = '';
|
||||
error = '';
|
||||
}
|
||||
|
||||
function handleKeydown(event: KeyboardEvent) {
|
||||
if (event.key === 'Enter') {
|
||||
handleSubmit();
|
||||
if (step === 'credentials') {
|
||||
handleSubmit();
|
||||
} else {
|
||||
handleVerify2FA();
|
||||
}
|
||||
}
|
||||
}
|
||||
</script>
|
||||
@@ -69,6 +124,38 @@
|
||||
{setupMethod}
|
||||
nextStep="Once setup is complete, return here to sign in."
|
||||
/>
|
||||
{:else if step === '2fa'}
|
||||
<p class="subtitle">Two-factor authentication</p>
|
||||
|
||||
<div class="form">
|
||||
<p class="hint">Enter the 6-digit code from your authenticator app, or a recovery code.</p>
|
||||
|
||||
<input
|
||||
type="text"
|
||||
placeholder="Authentication code"
|
||||
bind:value={totpCode}
|
||||
onkeydown={handleKeydown}
|
||||
disabled={loading}
|
||||
autocomplete="one-time-code"
|
||||
inputmode="numeric"
|
||||
/>
|
||||
|
||||
{#if error}
|
||||
<p class="error">{error}</p>
|
||||
{/if}
|
||||
|
||||
<button onclick={handleVerify2FA} disabled={loading}>
|
||||
{#if loading}
|
||||
Verifying...
|
||||
{:else}
|
||||
Verify
|
||||
{/if}
|
||||
</button>
|
||||
|
||||
<button class="back-button" onclick={handleBack} disabled={loading} type="button">
|
||||
Back to sign in
|
||||
</button>
|
||||
</div>
|
||||
{:else}
|
||||
<p class="subtitle">Sign in to continue</p>
|
||||
|
||||
@@ -154,6 +241,13 @@
|
||||
gap: var(--space-4);
|
||||
}
|
||||
|
||||
.hint {
|
||||
color: var(--text-muted);
|
||||
font-size: 0.85rem;
|
||||
text-align: left;
|
||||
line-height: 1.4;
|
||||
}
|
||||
|
||||
input {
|
||||
width: 100%;
|
||||
padding: var(--space-3) var(--space-4);
|
||||
@@ -208,6 +302,19 @@
|
||||
cursor: not-allowed;
|
||||
}
|
||||
|
||||
.back-button {
|
||||
background: transparent;
|
||||
color: var(--text-muted);
|
||||
font-size: 0.85rem;
|
||||
font-weight: 400;
|
||||
padding: var(--space-2) var(--space-4);
|
||||
}
|
||||
|
||||
.back-button:hover:not(:disabled) {
|
||||
color: var(--text-primary);
|
||||
opacity: 1;
|
||||
}
|
||||
|
||||
.register-link {
|
||||
margin-top: var(--space-6);
|
||||
color: var(--text-muted);
|
||||
|
||||
Reference in New Issue
Block a user