Userauth part3 backend routes#36
Open
parthdude07 wants to merge 6 commits into
Open
Conversation
|
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.
17 issues found across 12 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/src/services/authService.js">
<violation number="1" location="backend/src/services/authService.js:23">
P2: Registration has a TOCTOU (time-of-check-time-of-use) race condition that can produce a 500 error under concurrent requests for the same email. The check-then-insert pattern checks `SELECT ... WHERE email = $1`, then inserts. When two registrations arrive near-simultaneously for the same email, both checks can pass, the second INSERT hits the UNIQUE constraint and throws a PostgreSQL error (code 23505) that bubbles up as an unhandled 500 to the caller. The controller's `catch` block checks for `err.statusCode === 409`, but the raw pg error has no `statusCode` — so it falls through and becomes a 500 instead of a clean 409.</violation>
<violation number="2" location="backend/src/services/authService.js:36">
P1: Passwords longer than 72 UTF-8 bytes are accepted but bcryptjs silently ignores the suffix, so different passwords sharing the first 72 bytes authenticate as the same credential. Reject `bcrypt.truncates(password)` during both registration and login, or use a password-hashing scheme without this limit.</violation>
<violation number="3" location="backend/src/services/authService.js:43">
P2: Malformed or unsanitized display names can cause registration to fail with a 500 or store forbidden input because optional chaining only handles nullish values, not non-strings. Validate the field as a bounded string and apply the repository's sanitization before calling the service.</violation>
<violation number="4" location="backend/src/services/authService.js:49">
P2: Registration and login email addresses are written verbatim to application logs, exposing user identifiers in every configured transport. Log a non-identifying event or pass a redacted value instead.</violation>
<violation number="5" location="backend/src/services/authService.js:70">
P2: Unknown-email login attempts return without bcrypt work, while known-email attempts do not, allowing attackers to infer registered accounts from latency. Compare against a fixed dummy bcrypt hash when no user is found so both paths perform equivalent password work.</violation>
</file>
<file name="backend/src/middleware/auth.js">
<violation number="1" location="backend/src/middleware/auth.js:25">
P3: JWT parsing, verification, and user mapping are duplicated across `requireAuth` and `optionalAuth`, so authentication-policy fixes can drift between required and optional routes. A shared verifier that returns a normalized user, with each middleware applying its own failure policy, would centralize this logic.</violation>
<violation number="2" location="backend/src/middleware/auth.js:41">
P3: This optional-authentication path is currently dead and has no exercised behavior. Removing it until a route needs guest/user handling, or adding a consumer and tests, would avoid maintaining an unverified authentication path.</violation>
</file>
<file name="backend/.env.example">
<violation number="1" location="backend/.env.example:30">
P1: Production can run with a publicly known JWT signing key when this example is copied unchanged, making authentication tokens forgeable. Leaving the sample empty would make production fail the existing required-variable check until a real random secret is supplied.</violation>
</file>
<file name="backend/src/config/index.js">
<violation number="1" location="backend/src/config/index.js:43">
P1: Production accepts the publicly known `.env.example` placeholder as a valid JWT secret, allowing anyone who follows the example without replacing it to forge authentication tokens. Secret validation should reject placeholders and enforce a sufficiently random secret before startup.</violation>
<violation number="2" location="backend/src/config/index.js:69">
P1: A reachable staging or other non-production deployment without `JWT_SECRET` will accept forged Bearer tokens because all tokens use the known `dev-secret-change-in-production` key. Removing the fixed fallback and requiring an injected secret for every deployed environment would prevent authentication bypass.</violation>
</file>
<file name="backend/supabase/migrations/002_auth_users.sql">
<violation number="1" location="backend/supabase/migrations/002_auth_users.sql:8">
P1: This creates a public-schema table containing password hashes without a database access boundary. If `public` is exposed through Supabase APIs, an anon/authenticated client can read the hashes; keeping `users` server-only with RLS and no public policies (or moving it to a private schema) prevents that exposure.</violation>
<violation number="2" location="backend/supabase/migrations/002_auth_users.sql:10">
P2: Email identity uniqueness is case-sensitive in this schema even though `authService` treats emails case-insensitively. A non-normalizing writer or future auth provider can create duplicate logical accounts; enforcing a unique index on `LOWER(email)` would preserve one account per email.</violation>
<violation number="3" location="backend/supabase/migrations/002_auth_users.sql:21">
P3: This index duplicates the index automatically created by the `UNIQUE` constraint on `email`, adding storage and write overhead without changing lookup behavior. Removing the redundant index keeps the schema lean.</violation>
<violation number="4" location="backend/supabase/migrations/002_auth_users.sql:22">
P2: OAuth identities are not constrained to one row per `(provider, provider_id)`; this index only speeds up duplicate lookups. Making it a unique partial index for non-null provider IDs would prevent duplicate external accounts while still allowing local users without a provider ID.</violation>
<violation number="5" location="backend/supabase/migrations/002_auth_users.sql:33">
P3: Reapplying this migration fails at the trigger even though the surrounding DDL is written to tolerate existing objects. Dropping this named trigger before recreating it, or guarding creation in a `DO` block, would make repair/redeploy runs consistent.</violation>
</file>
<file name="backend/src/controllers/authController.js">
<violation number="1" location="backend/src/controllers/authController.js:18">
P2: Auth controller validates inputs inline (regex, length checks) instead of using the express-validator middleware pattern that every other endpoint follows.
The project has a dedicated validation layer (`src/middleware/validation.js`) with reusable rulesets like `validationRules.search` that are applied as route middleware. The auth controller's inline validation duplicates this concern inside the handler, making the validation invisible from the route definition and harder to audit, reuse, or update.
Consider extracting auth validation rules into `validationRules.register` and `validationRules.login` in `validation.js`, then applying them as middleware on the auth routes — consistent with the rest of the codebase.</violation>
<violation number="2" location="backend/src/controllers/authController.js:39">
P2: Auth controller constructs `{ success: true, data: ... }` response manually instead of using the `sendSuccess`/`sendCreated` helper consistently used by every other controller in this codebase.
All three handlers (`register`, `login`, `me`) call `res.json({ success: true, data: ... })` or `res.status(201).json(...)` directly. The rest of the codebase (transcriptController, aiController, healthController) uses `sendSuccess(res, data)` and `sendCreated(res, data)` from `src/utils/responseHelper.js`, which guarantees a uniform response shape and reduces duplication.
Recommendation: Replace inline response construction with the shared helpers — `sendCreated(res, result)` for register (201), `sendSuccess(res, result)` for login, and `sendSuccess(res, { user })` for me.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| # Authentication (JWT) | ||
| # Generate a strong secret: node -e "console.log(require('crypto').randomBytes(64).toString('hex'))" | ||
| JWT_SECRET=your-jwt-secret-min-32-chars |
There was a problem hiding this comment.
P1: Production can run with a publicly known JWT signing key when this example is copied unchanged, making authentication tokens forgeable. Leaving the sample empty would make production fail the existing required-variable check until a real random secret is supplied.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/.env.example, line 30:
<comment>Production can run with a publicly known JWT signing key when this example is copied unchanged, making authentication tokens forgeable. Leaving the sample empty would make production fail the existing required-variable check until a real random secret is supplied.</comment>
<file context>
@@ -24,3 +24,8 @@ RATE_LIMIT_MAX_REQUESTS=100
+
+# Authentication (JWT)
+# Generate a strong secret: node -e "console.log(require('crypto').randomBytes(64).toString('hex'))"
+JWT_SECRET=your-jwt-secret-min-32-chars
+JWT_EXPIRES_IN=7d
</file context>
Suggested change
| JWT_SECRET=your-jwt-secret-min-32-chars | |
| JWT_SECRET= |
|
|
||
| // Validate critical environment variables | ||
| const requiredVars = ['DATABASE_URL']; | ||
| const requiredVars = ['DATABASE_URL', 'JWT_SECRET']; |
There was a problem hiding this comment.
P1: Production accepts the publicly known .env.example placeholder as a valid JWT secret, allowing anyone who follows the example without replacing it to forge authentication tokens. Secret validation should reject placeholders and enforce a sufficiently random secret before startup.
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 43:
<comment>Production accepts the publicly known `.env.example` placeholder as a valid JWT secret, allowing anyone who follows the example without replacing it to forge authentication tokens. Secret validation should reject placeholders and enforce a sufficiently random secret before startup.</comment>
<file context>
@@ -40,7 +40,7 @@ const validateEnvVars = (requiredVars) => {
// Validate critical environment variables
-const requiredVars = ['DATABASE_URL'];
+const requiredVars = ['DATABASE_URL', 'JWT_SECRET'];
// Only validate in production, allow fallbacks in development
</file context>
|
|
||
| // Authentication (JWT) | ||
| auth: { | ||
| jwtSecret: process.env.JWT_SECRET || 'dev-secret-change-in-production', |
There was a problem hiding this comment.
P1: A reachable staging or other non-production deployment without JWT_SECRET will accept forged Bearer tokens because all tokens use the known dev-secret-change-in-production key. Removing the fixed fallback and requiring an injected secret for every deployed environment would prevent authentication bypass.
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>A reachable staging or other non-production deployment without `JWT_SECRET` will accept forged Bearer tokens because all tokens use the known `dev-secret-change-in-production` key. Removing the fixed fallback and requiring an injected secret for every deployed environment would prevent authentication bypass.</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>
| } | ||
|
|
||
| // Hash password | ||
| const hashedPassword = await bcrypt.hash(password, config.auth.bcryptRounds); |
There was a problem hiding this comment.
P1: Passwords longer than 72 UTF-8 bytes are accepted but bcryptjs silently ignores the suffix, so different passwords sharing the first 72 bytes authenticate as the same credential. Reject bcrypt.truncates(password) during both registration and login, or use a password-hashing scheme without this limit.
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 36:
<comment>Passwords longer than 72 UTF-8 bytes are accepted but bcryptjs silently ignores the suffix, so different passwords sharing the first 72 bytes authenticate as the same credential. Reject `bcrypt.truncates(password)` during both registration and login, or use a password-hashing scheme without this limit.</comment>
<file context>
@@ -0,0 +1,139 @@
+ }
+
+ // Hash password
+ const hashedPassword = await bcrypt.hash(password, config.auth.bcryptRounds);
+
+ // Insert user
</file context>
| -- ============================================================ | ||
|
|
||
| -- Users table | ||
| CREATE TABLE IF NOT EXISTS users ( |
There was a problem hiding this comment.
P1: This creates a public-schema table containing password hashes without a database access boundary. If public is exposed through Supabase APIs, an anon/authenticated client can read the hashes; keeping users server-only with RLS and no public policies (or moving it to a private schema) prevents that exposure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/supabase/migrations/002_auth_users.sql, line 8:
<comment>This creates a public-schema table containing password hashes without a database access boundary. If `public` is exposed through Supabase APIs, an anon/authenticated client can read the hashes; keeping `users` server-only with RLS and no public policies (or moving it to a private schema) prevents that exposure.</comment>
<file context>
@@ -0,0 +1,36 @@
+-- ============================================================
+
+-- Users table
+CREATE TABLE IF NOT EXISTS users (
+ id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
+ email TEXT UNIQUE NOT NULL,
</file context>
| */ | ||
| export const registerUser = async (email, password, name) => { | ||
| // Check if user already exists | ||
| const existing = await query( |
There was a problem hiding this comment.
P2: Registration has a TOCTOU (time-of-check-time-of-use) race condition that can produce a 500 error under concurrent requests for the same email. The check-then-insert pattern checks SELECT ... WHERE email = $1, then inserts. When two registrations arrive near-simultaneously for the same email, both checks can pass, the second INSERT hits the UNIQUE constraint and throws a PostgreSQL error (code 23505) that bubbles up as an unhandled 500 to the caller. The controller's catch block checks for err.statusCode === 409, but the raw pg error has no statusCode — so it falls through and becomes a 500 instead of a clean 409.
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 23:
<comment>Registration has a TOCTOU (time-of-check-time-of-use) race condition that can produce a 500 error under concurrent requests for the same email. The check-then-insert pattern checks `SELECT ... WHERE email = $1`, then inserts. When two registrations arrive near-simultaneously for the same email, both checks can pass, the second INSERT hits the UNIQUE constraint and throws a PostgreSQL error (code 23505) that bubbles up as an unhandled 500 to the caller. The controller's `catch` block checks for `err.statusCode === 409`, but the raw pg error has no `statusCode` — so it falls through and becomes a 500 instead of a clean 409.</comment>
<file context>
@@ -0,0 +1,139 @@
+ */
+export const registerUser = async (email, password, name) => {
+ // Check if user already exists
+ const existing = await query(
+ 'SELECT id FROM users WHERE email = $1',
+ [email.toLowerCase().trim()]
</file context>
| * Useful for routes that work for both guests and logged-in users. | ||
| * Sets req.user = null when no token or invalid token. | ||
| */ | ||
| export const optionalAuth = (req, _res, next) => { |
There was a problem hiding this comment.
P3: This optional-authentication path is currently dead and has no exercised behavior. Removing it until a route needs guest/user handling, or adding a consumer and tests, would avoid maintaining an unverified authentication path.
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 41:
<comment>This optional-authentication path is currently dead and has no exercised behavior. Removing it until a route needs guest/user handling, or adding a consumer and tests, would avoid maintaining an unverified authentication path.</comment>
<file context>
@@ -0,0 +1,59 @@
+ * Useful for routes that work for both guests and logged-in users.
+ * Sets req.user = null when no token or invalid token.
+ */
+export const optionalAuth = (req, _res, next) => {
+ const authHeader = req.headers.authorization;
+
</file context>
| const token = authHeader.split(' ')[1]; | ||
|
|
||
| try { | ||
| const decoded = jwt.verify(token, config.auth.jwtSecret); |
There was a problem hiding this comment.
P3: JWT parsing, verification, and user mapping are duplicated across requireAuth and optionalAuth, so authentication-policy fixes can drift between required and optional routes. A shared verifier that returns a normalized user, with each middleware applying its own failure policy, would centralize this logic.
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 25:
<comment>JWT parsing, verification, and user mapping are duplicated across `requireAuth` and `optionalAuth`, so authentication-policy fixes can drift between required and optional routes. A shared verifier that returns a normalized user, with each middleware applying its own failure policy, would centralize this logic.</comment>
<file context>
@@ -0,0 +1,59 @@
+ const token = authHeader.split(' ')[1];
+
+ try {
+ const decoded = jwt.verify(token, config.auth.jwtSecret);
+ req.user = { id: decoded.sub, email: decoded.email };
+ next();
</file context>
| ); | ||
|
|
||
| -- Indexes | ||
| CREATE INDEX IF NOT EXISTS idx_users_email ON users(email); |
There was a problem hiding this comment.
P3: This index duplicates the index automatically created by the UNIQUE constraint on email, adding storage and write overhead without changing lookup behavior. Removing the redundant index keeps the schema lean.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/supabase/migrations/002_auth_users.sql, line 21:
<comment>This index duplicates the index automatically created by the `UNIQUE` constraint on `email`, adding storage and write overhead without changing lookup behavior. Removing the redundant index keeps the schema lean.</comment>
<file context>
@@ -0,0 +1,36 @@
+);
+
+-- Indexes
+CREATE INDEX IF NOT EXISTS idx_users_email ON users(email);
+CREATE INDEX IF NOT EXISTS idx_users_provider ON users(provider, provider_id);
+
</file context>
| END; | ||
| $$ LANGUAGE plpgsql; | ||
|
|
||
| CREATE TRIGGER trg_users_updated_at |
There was a problem hiding this comment.
P3: Reapplying this migration fails at the trigger even though the surrounding DDL is written to tolerate existing objects. Dropping this named trigger before recreating it, or guarding creation in a DO block, would make repair/redeploy runs consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/supabase/migrations/002_auth_users.sql, line 33:
<comment>Reapplying this migration fails at the trigger even though the surrounding DDL is written to tolerate existing objects. Dropping this named trigger before recreating it, or guarding creation in a `DO` block, would make repair/redeploy runs consistent.</comment>
<file context>
@@ -0,0 +1,36 @@
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE TRIGGER trg_users_updated_at
+ BEFORE UPDATE ON users
+ FOR EACH ROW
</file context>
- 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
parthdude07
force-pushed
the
userauth-part3-backend-routes
branch
from
July 21, 2026 09:53
ed26223 to
e938151
Compare
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
Adds JWT-based email/password auth to the backend and introduces local note‑taking with a transcript whiteboard. Updates the UI to create, tag, and pin notes, and wires notes into highlights and transcript pages.
New Features
/api/v1/auth:POST /register,POST /login,GET /me(protected)requireAuth,optionalAuthregisterUser,loginUser,getUserByIdwith bcrypt hashing and JWTsJWT_SECRET,JWT_EXPIRES_IN,bcryptRoundsadded to.env.exampleand prod validationuseNotes,NoteCard,NoteEditor,NotesSearchBar,TranscriptNotes) with tags/colors/pin, search/filter/sort, storage + migration; interactive whiteboard (TranscriptWhiteboard,ConceptNode,useWhiteboard) for concept mappingTranscriptDetailadds Notes/Whiteboard tabs;Libraryadds Notes tab;HighlightToolbarcan add notes; highlights now supportcolorandisUnderline;TranscriptChatcan consume a pending promptjsonwebtoken,bcryptjs,@xyflow/reactMigration
backend/supabase/migrations/002_auth_users.sqlto create theuserstable and triggerJWT_SECRET(32+ chars) and optionallyJWT_EXPIRES_INin your environment, then restart the serverWritten for commit e938151. Summary will update on new commits.