Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion api/routes/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,12 @@
await _set_mail_hash(email, password_hash)

else:
logging.info("User already exists: %s", _sanitize_for_log(email))
# User already exists - return error instead of success
logging.info("Signup attempt for existing user: %s", _sanitize_for_log(email))
Comment thread Fixed
return JSONResponse(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[major] This new branch now treats any failure from ensure_user_in_organizations() as “user already exists” and forces a 409. When the helper returns success = False because the graph DB or callback handler failed, we’ll still execute this block, tell the user their email is taken, and hide the real 5xx error. That makes triage impossible and forces users to retry endlessly even though the backend is down. We need to distinguish between success is False (return a 500/"Registration failed") and the legit success=True/new_identity=False case before emitting this response.

{"success": False, "error": "An account with this email already exists"},
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[major] Returning "An account with this email already exists" along with a 409 leaks whether a particular email has ever registered. Attackers can now enumerate valid accounts just by hitting /signup/email. The earlier implementation always returned a generic success payload, so this is a new information disclosure. Please respond with a generic error (e.g. “Registration failed”/400) even when the identity already exists, and surface the specific reason only in logs.

status_code=status.HTTP_409_CONFLICT
)

logging.info("User registration successful: %s", _sanitize_for_log(email))

Expand Down
307 changes: 251 additions & 56 deletions app/src/components/modals/LoginModal.tsx

Large diffs are not rendered by default.

327 changes: 327 additions & 0 deletions app/src/components/modals/SignupModal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,327 @@
import { useState, useEffect } from 'react';
import { Dialog, DialogContent, DialogHeader, DialogTitle, DialogDescription } from "@/components/ui/dialog";
import { Button } from "@/components/ui/button";
import { Input } from "@/components/ui/input";
import { Label } from "@/components/ui/label";
import { useAuth } from "@/contexts/AuthContext";
import { buildApiUrl, API_CONFIG } from "@/config/api";

interface SignupModalProps {
open: boolean;
onOpenChange: (open: boolean) => void;
onSwitchToLogin?: () => void;
canClose?: boolean;
startWithEmailForm?: boolean; // Start with email form already open
}

const SignupModal = ({ open, onOpenChange, onSwitchToLogin, canClose = true, startWithEmailForm = false }: SignupModalProps) => {
Comment thread
Naseem77 marked this conversation as resolved.
Outdated
const { signup, refreshAuth } = useAuth();
const [firstName, setFirstName] = useState('');
const [lastName, setLastName] = useState('');
const [email, setEmail] = useState('');
const [password, setPassword] = useState('');
const [confirmPassword, setConfirmPassword] = useState('');
const [error, setError] = useState('');
const [isLoading, setIsLoading] = useState(false);
const [showEmailForm, setShowEmailForm] = useState(startWithEmailForm);

// Reset form when modal opens/closes or startWithEmailForm changes
useEffect(() => {
if (open) {
setShowEmailForm(startWithEmailForm);
setError('');
setFirstName('');
setLastName('');
setEmail('');
setPassword('');
setConfirmPassword('');
} else {
// Reset when closing
setShowEmailForm(false);
setError('');
setFirstName('');
setLastName('');
setEmail('');
setPassword('');
setConfirmPassword('');
}
}, [open, startWithEmailForm]);

const handleGoogleSignup = () => {
Comment thread
Naseem77 marked this conversation as resolved.
Outdated
window.location.href = buildApiUrl(API_CONFIG.ENDPOINTS.LOGIN_GOOGLE);
};

const handleGithubSignup = () => {
window.location.href = buildApiUrl(API_CONFIG.ENDPOINTS.LOGIN_GITHUB);
};

const handleEmailSignup = async (e: React.FormEvent) => {
e.preventDefault();
setError('');

// Validation
if (!firstName.trim() || !lastName.trim() || !email.trim() || !password) {
setError('All fields are required');
return;
}

if (password.length < 8) {
setError('Password must be at least 8 characters long');
return;
}

if (password !== confirmPassword) {
setError('Passwords do not match');
return;
}

setIsLoading(true);
try {
const result = await signup.email(firstName, lastName, email, password);

if (result.success) {
// Refresh auth to get user data
await refreshAuth();
onOpenChange(false);
// Reset form
setFirstName('');
setLastName('');
setEmail('');
setPassword('');
setConfirmPassword('');
setShowEmailForm(false);
} else {
setError(result.error || 'Signup failed');
}
} catch (err) {
setError('An unexpected error occurred');
} finally {
setIsLoading(false);
}
};

return (
<Dialog
open={open}
onOpenChange={canClose ? onOpenChange : undefined}
>
<DialogContent
className="sm:max-w-[425px] bg-card border-border"
onInteractOutside={(e) => {
if (!canClose) {
e.preventDefault();
}
}}
onEscapeKeyDown={(e) => {
if (!canClose) {
e.preventDefault();
}
}}
>
<DialogHeader>
<DialogTitle className="text-2xl font-semibold text-center text-card-foreground">
Create Your Account
</DialogTitle>
<DialogDescription className="text-center text-muted-foreground pt-2">
Sign up to start using QueryWeaver
</DialogDescription>
</DialogHeader>

{!showEmailForm ? (
<div className="space-y-4 py-6">
<Button
onClick={handleGoogleSignup}
className="w-full bg-white hover:bg-gray-50 text-gray-900 hover:text-gray-900 border-2 border-gray-300 hover:border-gray-400 font-medium py-6 text-base flex items-center justify-center gap-3 shadow-sm hover:shadow transition-all"
variant="outline"
>
<svg className="w-5 h-5" viewBox="0 0 24 24">
<path
fill="currentColor"
d="M22.56 12.25c0-.78-.07-1.53-.2-2.25H12v4.26h5.92c-.26 1.37-1.04 2.53-2.21 3.31v2.77h3.57c2.08-1.92 3.28-4.74 3.28-8.09z"
/>
<path
fill="currentColor"
d="M12 23c2.97 0 5.46-.98 7.28-2.66l-3.57-2.77c-.98.66-2.23 1.06-3.71 1.06-2.86 0-5.29-1.93-6.16-4.53H2.18v2.84C3.99 20.53 7.7 23 12 23z"
/>
<path
fill="currentColor"
d="M5.84 14.09c-.22-.66-.35-1.36-.35-2.09s.13-1.43.35-2.09V7.07H2.18C1.43 8.55 1 10.22 1 12s.43 3.45 1.18 4.93l2.85-2.22.81-.62z"
/>
<path
fill="currentColor"
d="M12 5.38c1.62 0 3.06.56 4.21 1.64l3.15-3.15C17.45 2.09 14.97 1 12 1 7.7 1 3.99 3.47 2.18 7.07l3.66 2.84c.87-2.6 3.3-4.53 6.16-4.53z"
/>
</svg>
Sign up with Google
</Button>

<Button
onClick={handleGithubSignup}
className="w-full bg-gradient-to-r from-gray-200 to-gray-300 hover:from-gray-300 hover:to-gray-400 dark:from-[#24292e] dark:to-[#1a1e22] dark:hover:from-[#1b1f23] dark:hover:to-[#161a1d] text-gray-900 dark:text-white font-medium py-6 text-base flex items-center justify-center gap-3 shadow-md hover:shadow-lg transition-all border-2 border-gray-400 hover:border-gray-500 dark:border-gray-600"
>
<svg className="w-5 h-5" fill="currentColor" viewBox="0 0 24 24">
<path d="M12 0c-6.626 0-12 5.373-12 12 0 5.302 3.438 9.8 8.207 11.387.599.111.793-.261.793-.577v-2.234c-3.338.726-4.033-1.416-4.033-1.416-.546-1.387-1.333-1.756-1.333-1.756-1.089-.745.083-.729.083-.729 1.205.084 1.839 1.237 1.839 1.237 1.07 1.834 2.807 1.304 3.492.997.107-.775.418-1.305.762-1.604-2.665-.305-5.467-1.334-5.467-5.931 0-1.311.469-2.381 1.236-3.221-.124-.303-.535-1.524.117-3.176 0 0 1.008-.322 3.301 1.23.957-.266 1.983-.399 3.003-.404 1.02.005 2.047.138 3.006.404 2.291-1.552 3.297-1.23 3.297-1.23.653 1.653.242 2.874.118 3.176.77.84 1.235 1.911 1.235 3.221 0 4.609-2.807 5.624-5.479 5.921.43.372.823 1.102.823 2.222v3.293c0 .319.192.694.801.576 4.765-1.589 8.199-6.086 8.199-11.386 0-6.627-5.373-12-12-12z"/>
</svg>
Sign up with GitHub
</Button>

<div className="relative">
<div className="absolute inset-0 flex items-center">
<span className="w-full border-t border-border" />
</div>
<div className="relative flex justify-center text-xs uppercase">
<span className="bg-card px-2 text-muted-foreground">Or</span>
</div>
</div>

<Button
onClick={() => setShowEmailForm(true)}
className="w-full"
variant="outline"
>
Sign up with Email
</Button>

{onSwitchToLogin && (
<div className="text-center text-sm text-muted-foreground pt-4">
Already have an account?{' '}
<button
onClick={onSwitchToLogin}
className="text-primary hover:underline font-medium"
>
Sign in
</button>
</div>
)}
</div>
) : (
<form onSubmit={handleEmailSignup} className="space-y-4 py-6">
<div className="grid grid-cols-2 gap-4">
<div className="space-y-2">
<Label htmlFor="firstName">First Name</Label>
<Input
id="firstName"
type="text"
placeholder="John"
value={firstName}
onChange={(e) => setFirstName(e.target.value)}
required
disabled={isLoading}
/>
</div>
<div className="space-y-2">
<Label htmlFor="lastName">Last Name</Label>
<Input
id="lastName"
type="text"
placeholder="Doe"
value={lastName}
onChange={(e) => setLastName(e.target.value)}
required
disabled={isLoading}
/>
</div>
</div>

<div className="space-y-2">
<Label htmlFor="email">Email</Label>
<Input
id="email"
type="email"
placeholder="john@example.com"
value={email}
onChange={(e) => setEmail(e.target.value)}
required
disabled={isLoading}
/>
</div>

<div className="space-y-2">
<Label htmlFor="password">Password</Label>
<Input
id="password"
type="password"
placeholder="••••••••"
value={password}
onChange={(e) => setPassword(e.target.value)}
required
disabled={isLoading}
minLength={8}
/>
<p className="text-xs text-muted-foreground">
Must be at least 8 characters long
</p>
</div>

<div className="space-y-2">
<Label htmlFor="confirmPassword">Confirm Password</Label>
<Input
id="confirmPassword"
type="password"
placeholder="••••••••"
value={confirmPassword}
onChange={(e) => setConfirmPassword(e.target.value)}
required
disabled={isLoading}
/>
</div>

{error && (
<div className="text-sm text-destructive bg-destructive/10 p-3 rounded-md">
{error}
</div>
)}

<div className="flex gap-3">
<Button
type="button"
variant="outline"
className="flex-1"
onClick={() => {
setShowEmailForm(false);
setError('');
}}
disabled={isLoading}
>
Back
</Button>
<Button
type="submit"
className="flex-1"
disabled={isLoading}
>
{isLoading ? 'Creating Account...' : 'Create Account'}
</Button>
</div>

{onSwitchToLogin && (
<div className="text-center text-sm text-muted-foreground pt-2">
Already have an account?{' '}
<button
type="button"
onClick={() => {
if (onSwitchToLogin) {
onSwitchToLogin();
}
}}
className="text-primary hover:underline font-medium"
disabled={isLoading}
>
Sign in
</button>
</div>
)}
</form>
)}

{canClose && !showEmailForm && (
<div className="text-center text-sm text-muted-foreground pt-2">
<p>By signing up, you agree to our Terms of Service and Privacy Policy</p>
</div>
)}
</DialogContent>
</Dialog>
);
};

export default SignupModal;
Comment thread
Naseem77 marked this conversation as resolved.
Outdated
2 changes: 2 additions & 0 deletions app/src/config/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ export const API_CONFIG = {
AUTH_STATUS: '/auth-status',
LOGIN_GOOGLE: '/login/google',
LOGIN_GITHUB: '/login/github',
LOGIN_EMAIL: '/login/email',
SIGNUP_EMAIL: '/signup/email',
LOGOUT: '/logout',

// Graph/Database management
Expand Down
8 changes: 8 additions & 0 deletions app/src/contexts/AuthContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ interface AuthContextType {
login: {
google: () => Promise<void>;
github: () => Promise<void>;
email: (email: string, password: string) => Promise<{ success: boolean; error?: string }>;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[major] Now that the context exposes login.email/signup.email, refreshAuth() will eventually populate user.provider with the literal 'email'. Our shared User type (imported from @/types/api) still restricts provider to 'google' | 'github', so this assignment will be rejected by TypeScript (or worse, forced to any) and anything narrowing on provider will never match the email case. Please extend the User interface to include 'email' (and update the client code accordingly) so the new auth path is representable.

};
signup: {
email: (firstName: string, lastName: string, email: string, password: string) => Promise<{ success: boolean; error?: string }>;
};
logout: () => Promise<void>;
refreshAuth: () => Promise<void>;
Expand Down Expand Up @@ -55,6 +59,10 @@ export const AuthProvider: React.FC<{ children: React.ReactNode }> = ({ children
login: {
google: AuthService.loginWithGoogle,
github: AuthService.loginWithGithub,
email: AuthService.loginWithEmail,
},
signup: {
email: AuthService.signupWithEmail,
},
logout: handleLogout,
refreshAuth: checkAuth,
Expand Down
Loading