feat: Transcribe auth#634
Conversation
Co-authored-by: aider (gemini/gemini-3.1-pro-preview) <aider@aider.chat>
Co-authored-by: aider (gemini/gemini-3.1-pro-preview) <aider@aider.chat>
…od methods Co-authored-by: aider (gemini/gemini-3.1-pro-preview) <aider@aider.chat>
Co-authored-by: aider (gemini/gemini-3.1-pro-preview) <aider@aider.chat>
Co-authored-by: aider (gemini/gemini-3.1-pro-preview) <aider@aider.chat>
Co-authored-by: aider (gemini/gemini-3.1-pro-preview) <aider@aider.chat>
Co-authored-by: aider (gemini/gemini-3.1-pro-preview) <aider@aider.chat>
Co-authored-by: aider (gemini/gemini-3.1-pro-preview) <aider@aider.chat>
Co-authored-by: aider (gemini/gemini-3.1-pro-preview) <aider@aider.chat>
inperegelion
left a comment
inperegelion
left a comment
There was a problem hiding this comment.
🔍 Code Review — PR #634 "feat: Transcribe auth"
Reviewed by three independent Claude agents covering Security, Logic/Functionality, and Clean Code.
🔴 Critical
by Claude [Security/Logic]: Missing auth middleware on GET /api/auth/me
src/server/src/app.ts — The route app.get('/api/auth/me', handleTranscribeAuthMe) has no transcribeAuthMiddleware, but the handler reads c.var.userId (set only by that middleware). This means userId is always undefined, making the endpoint non-functional and potentially exposing unexpected behavior. The DELETE route correctly includes the middleware.
Fix: app.get('/api/auth/me', transcribeAuthMiddleware, handleTranscribeAuthMe)
by Claude [Logic]: Project create fetch missing credentials: 'include'
src/app/transcribe/create/page.tsx (onSubmit) — The POST /api/transcribe/projects fetch does not include credentials: 'include'. The browser won't send the auth_session cookie, so the backend will always return 401. Project creation is broken.
Fix: Add credentials: 'include' to the fetch options.
🟠 High
by Claude [Logic]: field.value.join(',') crashes when value is undefined
src/app/transcribe/create/page.tsx (location Controller render) — field.value can be undefined during initialization, causing TypeError: Cannot read properties of undefined (reading 'join') — matches the crash in spec 041.
Fix: (field.value ?? []).join(',') or field.value?.join(',') ?? ''
by Claude [Logic]: 'use client' removed from location-picker.tsx
src/app/components/contribute/location-picker.tsx — The 'use client' directive was removed, but the component uses client-only hooks (usePostHog, useMemo, useRef). This is a regression that will break existing usage if no parent client boundary exists.
Fix: Restore 'use client'.
by Claude [Logic]: year_end uses || instead of ??
src/server/src/database/create-project.ts — yearsRange[1] || yearsRange[0] — if yearsRange[1] is 0, the falsy check incorrectly falls back to yearsRange[0].
Fix: Use yearsRange[1] ?? yearsRange[0].
by Claude [Logic]: import { SubmitEvent } from 'react' is incorrect
src/app/transcribe/create/page.tsx — SubmitEvent is a DOM global type, not a React export. This will fail TypeScript compilation.
Fix: Remove the import; use global SubmitEvent or React.FormEvent<HTMLFormElement>.
🟡 Medium
by Claude [Security]: No CSRF protection on state-changing endpoints
Auth relies solely on HttpOnly cookies. State-changing endpoints (POST /api/transcribe/projects, DELETE /api/auth/me) have no CSRF token mechanism. SameSite=Lax helps but isn't complete protection.
by Claude [Security]: Logout only deletes client cookie, JWT remains valid
src/server/src/handlers/handle-transcribe-auth-delete.ts — The JWT remains valid until expiration. If captured, it can still be used after "logout." Consider short-lived tokens or a server-side blocklist.
by Claude [Logic]: projects-list.tsx and user.tsx don't check response.ok
Both components call response.json() without checking response.ok. Error responses (401, 500) will be parsed as data and fail at Zod validation with misleading errors.
by Claude [Logic]: layout.tsx missing 'use client' directive
src/app/transcribe/layout.tsx — Uses GoogleOAuthProvider (a client component with React context) but lacks 'use client'. This will error in Next.js App Router.
by Claude [Code]: process.env.NODE_ENV used directly in handle-transcribe-auth-delete.ts
Uses process.env.NODE_ENV instead of the centralized environment.NODE_ENV module. Inconsistent with handle-transcribe-auth-google.ts and violates CONVENTIONS.md.
by Claude [Code]: Unnecessary async with eslint-disable
src/server/src/handlers/handle-transcribe-auth-delete.ts — Function is async but has no await. Remove async instead of suppressing the lint rule.
by Claude [Code]: Heavy inline styles in create/page.tsx
10+ inline style={{...}} occurrences while sibling components use CSS modules. Inconsistent approach within the same PR.
by Claude [Code]: Broad try/catch masks errors in handle-transcribe-auth-me.ts
All errors (including DB connection failures) are silently caught and returned as 401. Should distinguish "not found" from unexpected errors.
🔵 Low
by Claude [Code]: console.log(data) left in projects-list.tsx — Remove debug logging.
by Claude [Code]: console.log('Not Found') in 404 handler (src/server/src/app.ts) — Remove or use structured logging.
by Claude [Code]: Redundant null check after Zod parse — if (!oauthToken) in handle-transcribe-auth-google.ts is dead code since authSchema.parse() already validates.
by Claude [Code]: Relative import in user.tsx — Uses '../../environment' while other files use '@/app/environment'.
by Claude [Code]: Unnecessary return; in .then() callbacks — In projects-list.tsx, user.tsx, logout-button.tsx.
by Claude [Code]: Dead file get-projects.ts — src/server/src/database/get-projects.ts is unused; no handler imports it.
by Claude [Code]: <div> instead of <header> in transcribe-header.tsx — Use semantic <header> element.
by Claude [Code]: Inconsistent zod import — import z from 'zod' vs import { z } from 'zod' across files.
by Claude [Code]: Inconsistent export styles — Mix of arrow functions with default export vs named function declarations across handlers. Mix of named vs default exports in database files.
by Claude [Code]: Overly verbose comments in logout-button.tsx — "Sever the local Google Identity SDK state" → could be simplified.
Summary
| Severity | Count |
|---|---|
| Critical | 2 |
| High | 4 |
| Medium | 8 |
| Low | 11 |
Blocking issues: The missing auth middleware on /api/auth/me and missing credentials: 'include' on project create should be fixed before merge. The location-picker.tsx 'use client' removal is a regression for existing functionality.
inperegelion
left a comment
inperegelion
left a comment
There was a problem hiding this comment.
Follow-up review with inline code suggestions for key findings identified by Claude [Security], [Logic], and [Code] reviewers.
Co-authored-by: aider (gemini/gemini-flash-latest) <aider@aider.chat>
Co-authored-by: aider (gemini/gemini-flash-latest) <aider@aider.chat>
|
🤖 Hi @undead404, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
@gemini-cli /review |
|
🤖 Hi @undead404, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @undead404, but I was unable to process your request. Please see the logs for more details. |
|
@gemini-cli /review |
|
🤖 Hi @undead404, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @undead404, but I was unable to process your request. Please see the logs for more details. |
|
@gemini-cli /review |
|
🤖 Hi @undead404, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
@gemini-cli /review |
|
🤖 Hi @undead404, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @undead404, but I was unable to process your request. Please see the logs for more details. |
|
|
||
| // 2. Execute backend cookie destruction | ||
| requestApi('/api/auth/me', { | ||
| method: 'DELETE', |
There was a problem hiding this comment.
unclear usage of REST standard. Use one of options:
- DELETE:/api/auth/session - still tricky to understand, and not best practice, but at least readable
- GET:/api/auth/signout - I'd mark it as most used option
There was a problem hiding this comment.
Changing to DELETE api/auth/session/current. GET is a no-go.
| return; | ||
| }) | ||
| .catch(() => { | ||
| toast.error('Failed to sign out'); |
There was a problem hiding this comment.
follow one semantic naming for routes and texts, don't mix them:
- signin/signout
- login/logout
| <section> | ||
| <h1>Projects</h1> | ||
| {projects.map((project) => ( | ||
| <p key={project.id}>{project.title}</p> |
There was a problem hiding this comment.
that's expected to render
not ? just not sure about logic, but looks like it's a place where you render list of projects, and potentially can open one of them
There was a problem hiding this comment.
It's just a placeholder for now; the implementation is the next stage – see #694
| export default function TranscribeHeader() { | ||
| return ( | ||
| <div className={styles.root}> | ||
| <p>Transcribe</p> |
There was a problem hiding this comment.
| <p>Transcribe</p> | |
| <h1>Transcribe</h1> |
looks like a header, so use sematic attribute
There was a problem hiding this comment.
I think it's an over-title, so there would be different pages under it... So not changing
|
|
||
| const handleFormSubmit = (event: SubmitEvent) => { | ||
| void handleSubmit(onSubmit)(event); | ||
| }; |
There was a problem hiding this comment.
useless function, that just add reading complexity with no affect on flow. handleSubmit(onSubmit) can be used directly:
<form onSubmit={handleSubmit(onSubmit)} className={styles.form}>
it's a standard practice: https://www.react-hook-form.com/get-started/
| import { v7 as uuidv7 } from 'uuid'; | ||
|
|
||
| export default function generateId() { | ||
| return uuidv7(); |
There was a problem hiding this comment.
use native DB feature instead: https://www.postgresql.org/docs/current/functions-uuid.html
| export type ImportPayload = z.infer<typeof importPayloadSchema>; | ||
|
|
||
| export const projectCreatePayloadSchema = z.object({ | ||
| id: nonEmptyString.regex(/^[a-z0-9-]+$/i), |
There was a problem hiding this comment.
in most classic scenarios you should NOT allow to pass id from business logic, and leave it to DB to decide
There was a problem hiding this comment.
This one is not classic... This is what'd become a slug later
4 participants
No description provided.