Userauth part5 frontend guards#38
Open
parthdude07 wants to merge 5 commits into
Open
Conversation
- Create 002_auth_users.sql: users table with email/password, provider columns for future GitHub OAuth, auto-update trigger - Add auth config block (JWT_SECRET, JWT_EXPIRES_IN, bcryptRounds) - Add JWT_SECRET to production required env vars - Install jsonwebtoken and bcryptjs dependencies - Update .env.example with auth variables
- auth.js middleware: requireAuth (hard 401) and optionalAuth (soft attach) for JWT verification from Authorization: Bearer header - authService.js: registerUser, loginUser, getUserById with bcrypt hashing, email normalization, and sanitized user output - authController.js: HTTP handlers with input validation (email format, password length 8–128), proper error code propagation - Export query() from supabaseService.js for use by authService
- Create authRoutes.js: POST /register, POST /login, GET /me - Mount auth routes at /api/v1/auth in route index - Add auth endpoints to API documentation endpoint - Export requireAuth and optionalAuth from middleware index
- Add auth types and API service - Add AuthContext for managing user state and modal visibility - Add useAuth hook - Add LoginModal component with tabs for login and registration
- Create ProtectedRoute component for guarding routes - Create LoginPrompt component for inline auth gating - Guard the /audio route in App.tsx - Guard the notes and canvas panels in TranscriptDetail.tsx - Update Layout.tsx to show user avatar and logout button
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
19 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/supabase/migrations/002_auth_users.sql">
<violation number="1" location="backend/supabase/migrations/002_auth_users.sql:21">
P3: Every user write will maintain two identical email indexes, with no additional query capability. Omit `idx_users_email`; the `UNIQUE` constraint already supplies the lookup index.</violation>
</file>
<file name="backend/src/config/index.js">
<violation number="1" location="backend/src/config/index.js:69">
P1: Instances started without `NODE_ENV=production` authenticate JWTs signed with this public fallback, so anyone can forge a token for an arbitrary `sub`. Require a configured secret whenever auth is enabled (and fail at startup) rather than using a shared default.</violation>
<violation number="2" location="backend/src/config/index.js:70">
P2: The default JWT expiration is 7 days, which is long for an access token. If a token is compromised (XSS, client-side leakage, network interception), the attacker has a week-long access window. Consider reducing the default to 1 hour or less, or documenting that this should be lowered in production environments and paired with a refresh-token mechanism for longer sessions.</violation>
</file>
<file name="src/components/Layout.tsx">
<violation number="1" location="src/components/Layout.tsx:254">
P2: A returning user can be shown—and open—the login dialog during token restoration, then remain authenticated behind that dialog after `/auth/me` succeeds. Gate this auth control on `isLoading` (or close the modal when restoration resolves) so the unauthenticated action is not exposed during validation.</violation>
<violation number="2" location="src/components/Layout.tsx:276">
P3: The "Profile" dropdown menu item has no `onClick` handler — clicking it does nothing. Consider either wiring it to a profile page (if one exists or is planned) or removing it to avoid a dead UI element.</violation>
<violation number="3" location="src/components/Layout.tsx:288">
P2: Mobile visitors cannot initiate sign-in from the header because this button is hidden and the mobile menu has no replacement. Include a Sign In action in the mobile menu or make an equivalent trigger visible there.</violation>
</file>
<file name="backend/src/routes/authRoutes.js">
<violation number="1" location="backend/src/routes/authRoutes.js:17">
P2: Malformed JSON bodies can reach bcrypt and produce a 500 instead of `VALIDATION_ERROR`: neither public auth route validates field types before its controller. Add auth-specific validators requiring bounded strings for email, password, and optional name, then apply them to both routes.</violation>
</file>
<file name="src/components/LoginModal.tsx">
<violation number="1" location="src/components/LoginModal.tsx:104">
P2: On short viewports, the registration form is clipped with no way to reach its lower fields or submit button. Give the dialog a viewport-bounded height and vertical scrolling instead of hiding overflow.</violation>
<violation number="2" location="src/components/LoginModal.tsx:137">
P3: Failed authentication feedback may not be announced to screen-reader users because the dynamically inserted banner is not a live region. Mark it as an alert.</violation>
<violation number="3" location="src/components/LoginModal.tsx:189">
P3: Keyboard and screen-reader users cannot operate the login password visibility control: `tabIndex={-1}` removes it from tab order and the icon-only button has no name. Restore focusability and provide its current action as a label.</violation>
</file>
<file name="backend/src/middleware/auth.js">
<violation number="1" location="backend/src/middleware/auth.js:22">
P3: JWT extraction/verification is duplicated across `requireAuth` and `optionalAuth`, which makes future auth changes easy to update in one path and miss in the other. A shared helper for parse+verify+user mapping would keep behavior consistent.</violation>
</file>
<file name="backend/src/services/authService.js">
<violation number="1" location="backend/src/services/authService.js:36">
P2: Passwords beyond bcrypt's 72-byte limit are accepted and silently truncated, so different long passwords can authenticate as the same credential. Reject `bcrypt.truncates(password)` during registration (and make the byte limit explicit to clients).</violation>
<violation number="2" location="backend/src/services/authService.js:36">
P2: The `registerUser` service function does not validate password length or format before hashing. While the current controller catches this (8-128 char enforcement), exposing the service directly to unvalidated input is a defense-in-depth gap. Add a password-length guard (e.g., if password.length < 8 or > 128, throw a validation error) at the top of registerUser so the validation is enforced regardless of caller.</violation>
<violation number="3" location="backend/src/services/authService.js:40">
P1: Concurrent sign-ups for one email return a 500/database error instead of `EMAIL_EXISTS`, since the pre-check is not atomic with this insert. Handle the unique-constraint result atomically (for example `ON CONFLICT`) and map it to the 409 response.</violation>
<violation number="4" location="backend/src/services/authService.js:70">
P2: Invalid logins leak account existence through timing: unknown emails skip bcrypt while known emails perform a password comparison. Compare against a fixed dummy bcrypt hash before returning the generic 401 for missing users.</violation>
</file>
<file name="backend/src/controllers/authController.js">
<violation number="1" location="backend/src/controllers/authController.js:16">
P2: The `register` and `login` handlers accept `email`, `password`, and `name` from `req.body` without verifying they are strings. This allows non-string types (numbers, arrays, objects) to bypass the simple existence and length checks. For example, a numeric password passes `password.length < 8` because `(12345678).length` is `undefined` (falsy comparison), and an object `name` would crash `authService` at `name?.trim()`. Recommend adding `typeof email === 'string' && typeof password === 'string'` guards or using express-validator schemas (as the rest of the codebase does for other endpoints) to ensure type safety before business logic runs.</violation>
<violation number="2" location="backend/src/controllers/authController.js:39">
P3: Auth endpoints return raw JSON instead of the shared response helper pattern used elsewhere, which can cause response-shape drift over time. Aligning these handlers with `sendSuccess`/`sendCreated` would keep API formatting consistent.</violation>
</file>
<file name="src/components/ProtectedRoute.tsx">
<violation number="1" location="src/components/ProtectedRoute.tsx:22">
P1: The two `useEffect` hooks in `ProtectedRoute` have identical conditions and race against each other. When a user visits a protected route without being logged in, one effect opens the login modal while the other simultaneously redirects them away (to `redirectTo`). This means the user gets pushed off the protected page at the same moment the modal appears, defeating the purpose of showing the modal on the protected path.
Fix: The redirect effect should only trigger after the modal has been explicitly shown and then dismissed without a successful login, not fire on the initial auth-complete render. One approach: track whether the modal was opened by this component using a ref (`const modalWasOpened = useRef(false)`), set it to `true` in the first effect, and only allow the redirect in the second effect when `modalWasOpened.current` is `true`.</violation>
</file>
<file name="src/contexts/AuthContext.tsx">
<violation number="1" location="src/contexts/AuthContext.tsx:66">
P0: The `authApi.me()` call in the token-validation effect sends no Authorization header, so the stored JWT token is never transmitted to the backend. The backend will reject the request with 401, the catch block clears the valid token from localStorage, and the user is logged out on every page refresh.
The comment on line 57 says "Temporarily inject the token header for this request" — acknowledging the requirement — but the code doesn't implement it. `api.ts` uses native `fetch()` with no auth interceptor, so the header must be passed explicitly.
**Fix**: Pass the stored token as an Authorization header. In `services/authService.ts`, update `me()` to accept a token parameter:
```typescript
me: (token: string) =>
api.get<{ user: User }>('/api/v1/auth/me', {
headers: { 'Authorization': `Bearer ${token}` },
}),
Then in src/contexts/AuthContext.tsx, change the call to:
const data = await authApi.me(token);This is the central validation path — without this fix, the entire auth-on-mount flow is broken.
</details>
<sub>Reply with feedback, questions, or to request a fix.<br /><br />[Re-trigger cubic](https://www.cubic.dev/action/re-review/pr/genesis-kb/genesis-kb.github.io/38/ai_pr_review_1784019714554_b9f65bb8-58e4-459b-b4d1-0ed337657c94?returnTo=https%3A%2F%2Fgithub.com%2Fgenesis-kb%2Fgenesis-kb.github.io%2Fpull%2F38)</sub>
<!-- cubic:review-post:ai_pr_review_1784019714554_b9f65bb8-58e4-459b-b4d1-0ed337657c94:e2827f14defc03ce6c0b4075edf33f9f5b26d7b1:0d1c3843-1ab8-43b2-ae48-d2dd824e584e -->
|
|
||
| try { | ||
| // Temporarily inject the token header for this request | ||
| const data = await authApi.me(); |
There was a problem hiding this comment.
P0: The authApi.me() call in the token-validation effect sends no Authorization header, so the stored JWT token is never transmitted to the backend. The backend will reject the request with 401, the catch block clears the valid token from localStorage, and the user is logged out on every page refresh.
The comment on line 57 says "Temporarily inject the token header for this request" — acknowledging the requirement — but the code doesn't implement it. api.ts uses native fetch() with no auth interceptor, so the header must be passed explicitly.
Fix: Pass the stored token as an Authorization header. In services/authService.ts, update me() to accept a token parameter:
me: (token: string) =>
api.get<{ user: User }>('/api/v1/auth/me', {
headers: { 'Authorization': `Bearer ${token}` },
}),Then in src/contexts/AuthContext.tsx, change the call to:
const data = await authApi.me(token);This is the central validation path — without this fix, the entire auth-on-mount flow is broken.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/contexts/AuthContext.tsx, line 66:
<comment>The `authApi.me()` call in the token-validation effect sends no Authorization header, so the stored JWT token is never transmitted to the backend. The backend will reject the request with 401, the catch block clears the valid token from localStorage, and the user is logged out on every page refresh.
The comment on line 57 says "Temporarily inject the token header for this request" — acknowledging the requirement — but the code doesn't implement it. `api.ts` uses native `fetch()` with no auth interceptor, so the header must be passed explicitly.
**Fix**: Pass the stored token as an Authorization header. In `services/authService.ts`, update `me()` to accept a token parameter:
```typescript
me: (token: string) =>
api.get<{ user: User }>('/api/v1/auth/me', {
headers: { 'Authorization': `Bearer ${token}` },
}),
Then in src/contexts/AuthContext.tsx, change the call to:
const data = await authApi.me(token);This is the central validation path — without this fix, the entire auth-on-mount flow is broken.
@@ -0,0 +1,137 @@ + + try { + // Temporarily inject the token header for this request + const data = await authApi.me(); + setUser(data.user); + } catch { ```|
|
||
| // Authentication (JWT) | ||
| auth: { | ||
| jwtSecret: process.env.JWT_SECRET || 'dev-secret-change-in-production', |
There was a problem hiding this comment.
P1: Instances started without NODE_ENV=production authenticate JWTs signed with this public fallback, so anyone can forge a token for an arbitrary sub. Require a configured secret whenever auth is enabled (and fail at startup) rather than using a shared default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/src/config/index.js, line 69:
<comment>Instances started without `NODE_ENV=production` authenticate JWTs signed with this public fallback, so anyone can forge a token for an arbitrary `sub`. Require a configured secret whenever auth is enabled (and fail at startup) rather than using a shared default.</comment>
<file context>
@@ -64,6 +64,13 @@ const config = {
+ // Authentication (JWT)
+ auth: {
+ jwtSecret: process.env.JWT_SECRET || 'dev-secret-change-in-production',
+ jwtExpiresIn: process.env.JWT_EXPIRES_IN || '7d',
+ bcryptRounds: 12,
</file context>
|
|
||
| // Insert user | ||
| const result = await query( | ||
| `INSERT INTO users (email, password, name) |
There was a problem hiding this comment.
P1: Concurrent sign-ups for one email return a 500/database error instead of EMAIL_EXISTS, since the pre-check is not atomic with this insert. Handle the unique-constraint result atomically (for example ON CONFLICT) and map it to the 409 response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/src/services/authService.js, line 40:
<comment>Concurrent sign-ups for one email return a 500/database error instead of `EMAIL_EXISTS`, since the pre-check is not atomic with this insert. Handle the unique-constraint result atomically (for example `ON CONFLICT`) and map it to the 409 response.</comment>
<file context>
@@ -0,0 +1,139 @@
+
+ // Insert user
+ const result = await query(
+ `INSERT INTO users (email, password, name)
+ VALUES ($1, $2, $3)
+ RETURNING id, email, name, avatar_url, created_at`,
</file context>
| const { user, isLoading, openLoginModal, isLoginModalOpen } = useAuth(); | ||
| const navigate = useNavigate(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
P1: The two useEffect hooks in ProtectedRoute have identical conditions and race against each other. When a user visits a protected route without being logged in, one effect opens the login modal while the other simultaneously redirects them away (to redirectTo). This means the user gets pushed off the protected page at the same moment the modal appears, defeating the purpose of showing the modal on the protected path.
Fix: The redirect effect should only trigger after the modal has been explicitly shown and then dismissed without a successful login, not fire on the initial auth-complete render. One approach: track whether the modal was opened by this component using a ref (const modalWasOpened = useRef(false)), set it to true in the first effect, and only allow the redirect in the second effect when modalWasOpened.current is true.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/ProtectedRoute.tsx, line 22:
<comment>The two `useEffect` hooks in `ProtectedRoute` have identical conditions and race against each other. When a user visits a protected route without being logged in, one effect opens the login modal while the other simultaneously redirects them away (to `redirectTo`). This means the user gets pushed off the protected page at the same moment the modal appears, defeating the purpose of showing the modal on the protected path.
Fix: The redirect effect should only trigger after the modal has been explicitly shown and then dismissed without a successful login, not fire on the initial auth-complete render. One approach: track whether the modal was opened by this component using a ref (`const modalWasOpened = useRef(false)`), set it to `true` in the first effect, and only allow the redirect in the second effect when `modalWasOpened.current` is `true`.</comment>
<file context>
@@ -0,0 +1,50 @@
+ const { user, isLoading, openLoginModal, isLoginModalOpen } = useAuth();
+ const navigate = useNavigate();
+
+ useEffect(() => {
+ // If we've finished loading and there is no user, trigger the login modal
+ if (!isLoading && !user && !isLoginModalOpen) {
</file context>
| <ThemeToggle /> | ||
|
|
||
| {/* Auth button/avatar */} | ||
| {user ? ( |
There was a problem hiding this comment.
P2: A returning user can be shown—and open—the login dialog during token restoration, then remain authenticated behind that dialog after /auth/me succeeds. Gate this auth control on isLoading (or close the modal when restoration resolves) so the unauthenticated action is not exposed during validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/Layout.tsx, line 254:
<comment>A returning user can be shown—and open—the login dialog during token restoration, then remain authenticated behind that dialog after `/auth/me` succeeds. Gate this auth control on `isLoading` (or close the modal when restoration resolves) so the unauthenticated action is not exposed during validation.</comment>
<file context>
@@ -240,11 +250,52 @@ export const Layout = ({ children }: { children: React.ReactNode }) => {
<ThemeToggle />
+ {/* Auth button/avatar */}
+ {user ? (
+ <DropdownMenu>
+ <DropdownMenuTrigger asChild>
</file context>
| type="button" | ||
| onClick={() => setShowLoginPassword(!showLoginPassword)} | ||
| className="absolute right-3 top-1/2 -translate-y-1/2 text-muted-foreground hover:text-foreground transition-colors" | ||
| tabIndex={-1} |
There was a problem hiding this comment.
P3: Keyboard and screen-reader users cannot operate the login password visibility control: tabIndex={-1} removes it from tab order and the icon-only button has no name. Restore focusability and provide its current action as a label.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/LoginModal.tsx, line 189:
<comment>Keyboard and screen-reader users cannot operate the login password visibility control: `tabIndex={-1}` removes it from tab order and the icon-only button has no name. Restore focusability and provide its current action as a label.</comment>
<file context>
@@ -0,0 +1,324 @@
+ type="button"
+ onClick={() => setShowLoginPassword(!showLoginPassword)}
+ className="absolute right-3 top-1/2 -translate-y-1/2 text-muted-foreground hover:text-foreground transition-colors"
+ tabIndex={-1}
+ >
+ {showLoginPassword ? <EyeOff className="w-4 h-4" /> : <Eye className="w-4 h-4" />}
</file context>
Suggested change
| tabIndex={-1} | |
| aria-label={showLoginPassword ? 'Hide password' : 'Show password'} |
| {/* Error banner */} | ||
| <AnimatePresence mode="wait"> | ||
| {error && ( | ||
| <motion.div |
There was a problem hiding this comment.
P3: Failed authentication feedback may not be announced to screen-reader users because the dynamically inserted banner is not a live region. Mark it as an alert.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/LoginModal.tsx, line 137:
<comment>Failed authentication feedback may not be announced to screen-reader users because the dynamically inserted banner is not a live region. Mark it as an alert.</comment>
<file context>
@@ -0,0 +1,324 @@
+ {/* Error banner */}
+ <AnimatePresence mode="wait">
+ {error && (
+ <motion.div
+ initial={{ opacity: 0, y: -8 }}
+ animate={{ opacity: 1, y: 0 }}
</file context>
Suggested change
| <motion.div | |
| <motion.div | |
| role="alert" |
| throw new APIError('Authentication required', 401, 'UNAUTHORIZED'); | ||
| } | ||
|
|
||
| const token = authHeader.split(' ')[1]; |
There was a problem hiding this comment.
P3: JWT extraction/verification is duplicated across requireAuth and optionalAuth, which makes future auth changes easy to update in one path and miss in the other. A shared helper for parse+verify+user mapping would keep behavior consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/src/middleware/auth.js, line 22:
<comment>JWT extraction/verification is duplicated across `requireAuth` and `optionalAuth`, which makes future auth changes easy to update in one path and miss in the other. A shared helper for parse+verify+user mapping would keep behavior consistent.</comment>
<file context>
@@ -0,0 +1,59 @@
+ throw new APIError('Authentication required', 401, 'UNAUTHORIZED');
+ }
+
+ const token = authHeader.split(' ')[1];
+
+ try {
</file context>
|
|
||
| try { | ||
| const result = await authService.registerUser(email, password, name); | ||
| res.status(201).json({ success: true, data: result }); |
There was a problem hiding this comment.
P3: Auth endpoints return raw JSON instead of the shared response helper pattern used elsewhere, which can cause response-shape drift over time. Aligning these handlers with sendSuccess/sendCreated would keep API formatting consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/src/controllers/authController.js, line 39:
<comment>Auth endpoints return raw JSON instead of the shared response helper pattern used elsewhere, which can cause response-shape drift over time. Aligning these handlers with `sendSuccess`/`sendCreated` would keep API formatting consistent.</comment>
<file context>
@@ -0,0 +1,84 @@
+
+ try {
+ const result = await authService.registerUser(email, password, name);
+ res.status(201).json({ success: true, data: result });
+ } catch (err) {
+ if (err.statusCode === 409) {
</file context>
| <span className="text-xs truncate max-w-full">{user.email}</span> | ||
| </DropdownMenuItem> | ||
| <DropdownMenuSeparator /> | ||
| <DropdownMenuItem className="cursor-pointer"> |
There was a problem hiding this comment.
P3: The "Profile" dropdown menu item has no onClick handler — clicking it does nothing. Consider either wiring it to a profile page (if one exists or is planned) or removing it to avoid a dead UI element.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/Layout.tsx, line 276:
<comment>The "Profile" dropdown menu item has no `onClick` handler — clicking it does nothing. Consider either wiring it to a profile page (if one exists or is planned) or removing it to avoid a dead UI element.</comment>
<file context>
@@ -240,11 +250,52 @@ export const Layout = ({ children }: { children: React.ReactNode }) => {
+ <span className="text-xs truncate max-w-full">{user.email}</span>
+ </DropdownMenuItem>
+ <DropdownMenuSeparator />
+ <DropdownMenuItem className="cursor-pointer">
+ Profile
+ </DropdownMenuItem>
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Add JWT-based authentication with email/password and introduce frontend auth guards, enabling registration, login, and protected UI/routes with a modal sign-in flow.
New Features
POST /api/v1/auth/register,POST /api/v1/auth/login,GET /api/v1/auth/mewith JWT responses.requireAuth(hard 401) andoptionalAuth(soft attach).id,email,password,name,avatar_url, timestamps) via SQL migration.JWT_SECRET,JWT_EXPIRES_IN(default 7d),bcryptRounds=12; addedjsonwebtokenandbcryptjs.queryfromsupabaseServicefor use in auth service.AuthProvider+useAuthfor user/token state and modal control.LoginModalwith tabs (Sign In / Create Account).ProtectedRoutefor guarded pages;/audionow protected.LoginPromptfor Notes/Canvas in TranscriptDetail.services/authService.tsforregister,login, andmecalls.Migration
.env:JWT_SECRET=<strong-random-secret>JWT_EXPIRES_IN=7dbackend/supabase/migrations/002_auth_users.sqlto createuserstable.Written for commit e2827f1. Summary will update on new commits.