feat: implement major auth system upgrade with dynamic forms and enha…#36
Merged
yash-pouranik merged 10 commits intomainfrom Mar 6, 2026
Merged
feat: implement major auth system upgrade with dynamic forms and enha…#36yash-pouranik merged 10 commits intomainfrom
yash-pouranik merged 10 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant overhaul to the authentication system, transforming it into a dynamic, schema-driven experience. The changes enhance security by hardening data privacy, improving resource management for both internal and external databases, and streamlining the developer experience with guided setup and dynamic forms for user management. The update aims to make the authentication service robust, user-friendly, and release-ready. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request aims to upgrade the authentication system with dynamic forms, email verification, password reset, and robust admin controls, focusing on security hardening and improved data security. However, it introduces critical security vulnerabilities, including Broken Access Control (IDOR) in admin user management, a Mass Assignment vulnerability during signup leading to potential privilege escalation, and Stored XSS due to a lack of sanitization in email templates. Immediate remediation of these issues is required.
Comment on lines
+392
to
+407
| module.exports.updateAdminUser = async (req, res) => { | ||
| try { | ||
| const { projectId, userId } = req.params; | ||
| const updateData = req.body; | ||
|
|
||
| // Prevent admin from inadvertently changing password hash directly from this endpoint | ||
| delete updateData.password; | ||
| delete updateData._id; | ||
|
|
||
| const collectionName = `${projectId}_users`; | ||
| const collection = mongoose.connection.db.collection(collectionName); | ||
|
|
||
| const result = await collection.updateOne( | ||
| { _id: new mongoose.Types.ObjectId(userId) }, | ||
| { $set: updateData } | ||
| ); |
Contributor
There was a problem hiding this comment.
The updateAdminUser endpoint does not verify that the authenticated developer owns the project associated with the provided projectId. An attacker with a valid developer account can modify user data for any project on the platform by simply providing the target projectId in the URL.
Comment on lines
+372
to
+380
| module.exports.getUserDetails = async (req, res) => { | ||
| try { | ||
| const { projectId, userId } = req.params; | ||
|
|
||
| const collectionName = `${projectId}_users`; | ||
| const collection = mongoose.connection.db.collection(collectionName); | ||
|
|
||
| const user = await collection.findOne({ _id: new mongoose.Types.ObjectId(userId) }); | ||
| if (!user) return res.status(404).json({ error: "User not found" }); |
Contributor
There was a problem hiding this comment.
The getUserDetails endpoint does not verify that the authenticated developer owns the project associated with the provided projectId. An attacker with a valid developer account can access sensitive user information (excluding passwords) for any project on the platform by simply providing the target projectId in the URL.
Comment on lines
+505
to
+510
| if (collectionName === 'users') { | ||
| delete req.body.password; | ||
| // Also ensure it's not and nested or sneaky | ||
| Object.keys(req.body).forEach(key => { | ||
| if (key.toLowerCase().includes('password')) delete req.body[key]; | ||
| }); |
Contributor
There was a problem hiding this comment.
This is a critical security improvement. Explicitly deleting password and any case-insensitive password related keys from the request body before an update prevents an attacker from attempting to overwrite or inject unhashed passwords into the users collection via the general editRow endpoint.
Comment on lines
+538
to
+541
| const responseData = updatedDoc.toObject(); | ||
| if (collectionName === 'users') { | ||
| delete responseData.password; | ||
| } |
Contributor
There was a problem hiding this comment.
Removing the password field from the responseData before sending it back to the client is a good practice. Even if the input was sanitized, ensuring the output is also clean adds another layer of protection against accidental data exposure.
frontend/src/pages/Auth.jsx
Outdated
Comment on lines
+212
to
+214
| // Convert everything except core fields to form data | ||
| const { _id, email, password, emailVerified, createdAt, updatedAt, ...customFields } = res.data; | ||
| setEditFormData(customFields); |
Contributor
There was a problem hiding this comment.
Destructuring password from res.data before setting editFormData is a good security practice. It ensures that the password hash is never loaded into the frontend form state, even if the backend accidentally sends it.
backend/utils/emailService.js
Outdated
| </head> | ||
| <body> | ||
| <div class="container"> | ||
| <div class="logo">${pname}</div> |
Contributor
There was a problem hiding this comment.
The sendAuthOtpEmail function embeds the project name (pname) directly into the HTML content of the email without sanitization. Since project names are developer-controlled, an attacker can inject malicious scripts that will execute when the email is viewed in a vulnerable client.
Comment on lines
+314
to
+339
| {project?.isAuthEnabled && !hasUserCollection && ( | ||
| <div style={{ | ||
| background: 'rgba(255, 189, 46, 0.1)', | ||
| border: '1px solid rgba(255, 189, 46, 0.2)', | ||
| borderRadius: '12px', | ||
| padding: '1.5rem', | ||
| marginBottom: '2rem', | ||
| display: 'flex', | ||
| gap: '1rem', | ||
| alignItems: 'center' | ||
| }}> | ||
| <AlertCircle color="#FFBD2E" size={24} style={{ flexShrink: 0 }} /> | ||
| <div style={{ flex: 1 }}> | ||
| <h4 style={{ color: '#FFBD2E', margin: '0 0 5px 0', fontSize: '1rem', fontWeight: 600 }}>User Schema Required</h4> | ||
| <p style={{ color: 'var(--color-text-muted)', fontSize: '0.9rem', margin: '0 0 12px 0', lineHeight: '1.5' }}> | ||
| Auth is enabled, but you haven't defined a <strong>"users"</strong> collection. Your Auth API will return errors until you create this collection in the Dashboard to define your user fields. | ||
| </p> | ||
| <button | ||
| className="btn btn-primary" | ||
| style={{ fontSize: '0.8rem', padding: '6px 12px' }} | ||
| onClick={() => navigate(`/project/${projectId}/create-collection?name=users`)} | ||
| > | ||
| Create "users" collection now | ||
| </button> | ||
| </div> | ||
| ) : ( | ||
| <div className="table-responsive"> | ||
| <table style={{ width: '100%', borderCollapse: 'collapse' }}> | ||
| <thead> | ||
| <tr style={{ backgroundColor: 'var(--color-bg-input)', borderBottom: '1px solid var(--color-border)' }}> | ||
| <th style={{ padding: '16px', textAlign: 'center', width: '60px' }}></th> | ||
| <th style={{ padding: '16px', textAlign: 'left', fontSize: '0.8rem', textTransform: 'uppercase', letterSpacing: '0.05em', color: 'var(--color-text-muted)', fontWeight: 600 }}>Email</th> | ||
| <th style={{ padding: '16px', textAlign: 'left', fontSize: '0.8rem', textTransform: 'uppercase', letterSpacing: '0.05em', color: 'var(--color-text-muted)', fontWeight: 600 }}>User ID</th> | ||
| <th style={{ padding: '16px', textAlign: 'left', fontSize: '0.8rem', textTransform: 'uppercase', letterSpacing: '0.05em', color: 'var(--color-text-muted)', fontWeight: 600 }}>Created At</th> | ||
| <th style={{ padding: '16px', textAlign: 'right' }}>Actions</th> | ||
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| {filteredUsers.map((user) => ( | ||
| <tr key={user._id} className="user-row" style={{ borderBottom: '1px solid var(--color-border)', transition: 'background 0.2s' }}> | ||
| <td style={{ padding: '16px', textAlign: 'center' }}> | ||
| <div style={{ width: '36px', height: '36px', borderRadius: '50%', background: 'linear-gradient(135deg, #333, #111)', display: 'flex', alignItems: 'center', justifyContent: 'center', margin: '0 auto', border: '1px solid var(--color-border)' }}> | ||
| <User size={16} color="#aaa" /> | ||
| </div> | ||
| </td> | ||
| <td style={{ padding: '16px', fontWeight: 500, color: 'var(--color-text-main)' }}> | ||
| <div style={{ display: 'flex', alignItems: 'center', gap: '8px' }}> | ||
| <Mail size={14} color="var(--color-text-muted)" /> | ||
| {user.email} | ||
| </div> | ||
| </td> | ||
| <td style={{ padding: '16px' }}> | ||
| <span style={{ fontFamily: 'monospace', fontSize: '0.8rem', color: 'var(--color-text-muted)', background: 'var(--color-bg-input)', padding: '4px 8px', borderRadius: '4px', border: '1px solid var(--color-border)' }}> | ||
| {user._id} | ||
| </span> | ||
| </td> | ||
| <td style={{ padding: '16px', fontSize: '0.9rem', color: 'var(--color-text-muted)' }}> | ||
| {user.createdAt ? new Date(user.createdAt).toLocaleDateString(undefined, { year: 'numeric', month: 'short', day: 'numeric' }) : 'N/A'} | ||
| </td> | ||
| <td style={{ padding: '16px', textAlign: 'right' }}> | ||
| <button | ||
| onClick={() => handleDelete(user._id)} | ||
| className="btn btn-ghost" | ||
| style={{ color: '#ef4444', padding: '8px', borderRadius: '6px' }} | ||
| title="Delete User" | ||
| > | ||
| <Trash2 size={18} /> | ||
| </button> | ||
| </td> | ||
| </div> |
Contributor
There was a problem hiding this comment.
This warning banner is an excellent addition for guiding users through the setup process. It clearly communicates the next steps when authentication is enabled but the 'users' collection schema is missing, improving the developer experience significantly.
Comment on lines
+329
to
+336
| const getInitialFields = () => { | ||
| if (initialName === 'users') { | ||
| return [ | ||
| { ...createEmptyField(), key: 'email', type: 'String', required: true }, | ||
| { ...createEmptyField(), key: 'password', type: 'String', required: true }, | ||
| { ...createEmptyField(), key: 'username', type: 'String', required: false }, | ||
| { ...createEmptyField(), key: 'emailVerified', type: 'Boolean', required: false }, | ||
| ]; |
Contributor
There was a problem hiding this comment.
This conditional logic to pre-fill essential authentication fields when creating a 'users' collection is a great feature. It streamlines the setup process for the authentication system and ensures that critical fields are included by default.
|
|
||
| <div className="card" style={{ padding: 0, overflow: 'hidden', display: 'flex', flexDirection: 'column', minHeight: '200px' }}> | ||
| {project.collections.length === 0 ? ( | ||
| {project.collections.filter(c => c.name !== 'users').length === 0 ? ( |
Contributor
There was a problem hiding this comment.
Filtering out the 'users' collection from the project overview table is a consistent approach with the DatabaseSidebar.jsx changes. It ensures that the 'users' collection is not prominently displayed in general project views, aligning with the security goal of centralizing user management on the Auth page.
| </thead> | ||
| <tbody> | ||
| {project.collections.map(c => ( | ||
| {project.collections.filter(c => c.name !== 'users').map(c => ( |
Contributor
There was a problem hiding this comment.
Applying the filter c => c.name !== 'users' here ensures that the 'users' collection is excluded from the list of collections displayed in the project details, maintaining consistency across the UI regarding the visibility of this sensitive collection.
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds per-project authentication: toggleable auth flag, users collection schema enforcement, Redis-backed OTP flows with a BullMQ auth-email queue, new admin user management endpoints and middleware, and frontend UI for managing users and toggling auth. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant Server as Express
participant DB as Database
participant Redis as Redis
participant Queue as BullMQ
participant Mailer as EmailService
User->>Frontend: Submit signup (email, password)
Frontend->>Server: POST /api/auth/signup
Server->>DB: Create user (emailVerified: false)
Server->>Redis: SET otp (project:otp:<email>)
Server->>Queue: Enqueue job (auth-email-queue) with {email, otp, type, pname}
Queue->>Mailer: Worker processes job -> sendAuthOtpEmail
Mailer->>User: Deliver OTP email
User->>Frontend: Submit OTP
Frontend->>Server: POST /api/auth/verify-email
Server->>Redis: GET/DEL otp
alt OTP valid
Server->>DB: Mark user.emailVerified = true
Server->>Frontend: 200 OK (sanitized user)
else invalid
Server->>Frontend: 400 Invalid OTP
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/src/pages/ProjectDetails.jsx (1)
221-240:⚠️ Potential issue | 🟡 MinorAvoid showing a false empty state when only
usersis hidden.If auth is configured and
usersis the only collection, this now renders “No Data Yet” even though the project already has a collection. That makes the dashboard look uninitialized and nudges users toward creating duplicate data collections.💡 Proposed fix
+ const visibleCollections = project.collections.filter(c => c.name !== 'users'); + const hasHiddenUsersCollection = project.collections.some(c => c.name === 'users'); + ... - {project.collections.filter(c => c.name !== 'users').length === 0 ? ( + {visibleCollections.length === 0 ? ( <div style={{ padding: '4rem 2rem', textAlign: 'center', color: 'var(--color-text-muted)', display: 'flex', flexDirection: 'column', alignItems: 'center', justifyContent: 'center', flex: 1 }}> <div style={{ width: '60px', height: '60px', borderRadius: '50%', background: 'var(--color-bg-input)', display: 'flex', alignItems: 'center', justifyContent: 'center', marginBottom: '1rem', border: '1px solid var(--color-border)' }}> <Layers size={24} color="#555" /> </div> - <h4 style={{ color: 'var(--color-text-main)', marginBottom: '5px' }}>No Data Yet</h4> - <p style={{ fontSize: '0.9rem', maxWidth: '200px' }}>Create a collection to start storing JSON documents.</p> + <h4 style={{ color: 'var(--color-text-main)', marginBottom: '5px' }}> + {hasHiddenUsersCollection ? 'No public collections yet' : 'No Data Yet'} + </h4> + <p style={{ fontSize: '0.9rem', maxWidth: '260px' }}> + {hasHiddenUsersCollection + ? 'The auth users collection is intentionally hidden here. Create another collection to store application data.' + : 'Create a collection to start storing JSON documents.'} + </p> </div> ) : ( ... - {project.collections.filter(c => c.name !== 'users').map(c => ( + {visibleCollections.map(c => (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/ProjectDetails.jsx` around lines 221 - 240, The empty-state check currently uses project.collections.filter(c => c.name !== 'users').length === 0 which shows “No Data Yet” when the only collection is the hidden 'users' collection; change the empty-state condition to check the raw collection count (project.collections.length === 0) so the UI only shows the empty state when the project truly has no collections, while keeping the mapping/rendering code that filters out 'users' (the map/filter expressions referencing c => c.name !== 'users') intact.backend/middleware/verifyApiKey.js (1)
19-46:⚠️ Potential issue | 🟠 MajorAdd API-key cache eviction to
toggleAuth()anddeleteCollection()mutations.The
toggleAuth()anddeleteCollection()endpoints modifyisAuthEnabledandcollectionsrespectively but only clear the ID-based cache. Requests authenticated with API keys will continue serving stale data from the API-key cache (2-hour TTL) until natural expiration. Both functions should calldeleteProjectByApiKeyCache()for both publishable and secret keys, consistent with other mutation endpoints likeupdateProject()andcreateCollection().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/middleware/verifyApiKey.js` around lines 19 - 46, The API-key cache eviction is missing for mutations that change project fields; update toggleAuth() and deleteCollection() to call deleteProjectByApiKeyCache() for both the publishable and secret API key variants after making database changes (same pattern used in updateProject() and createCollection()); locate toggleAuth() and deleteCollection() and, after successful update/delete and after any ID-based cache clears, invoke deleteProjectByApiKeyCache(publishableKeyHash) and deleteProjectByApiKeyCache(secretKeyHash) so API-key cache entries are invalidated along with the existing ID-based cache clears.backend/controllers/userAuth.controller.js (1)
84-95:⚠️ Potential issue | 🔴 CriticalReject unverified users before issuing a JWT.
Line 84 loads the user, but
emailVerifiedis never checked before Line 95 returns a token. Combined with the JWT already returned fromsignup, the new verification flow is effectively optional right now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/userAuth.controller.js` around lines 84 - 95, The login flow issues JWTs without verifying emailVerified: after retrieving the user via collection.findOne({ email }) and before calling jwt.sign, check user.emailVerified (or the appropriate verified flag on the user document) and if it is falsy return a 403/400 JSON error (e.g., { error: "Email not verified" }) instead of issuing a token; update the early-return logic around validPass/jwt.sign to reject unverified users so jwt.sign is only called when user.emailVerified is true.backend/controllers/project.controller.js (1)
779-785:⚠️ Potential issue | 🔴 CriticalPurge Redis when a project is deleted.
After Line 829 removes the row from MongoDB, stale
project:id:*andproject:apikey:*entries can still be served bygetSingleProjectandverifyApiKeyuntil TTL expiry. That keeps deleted projects visible and their API keys usable for a while.Suggested fix
- }).select( - "+resources.storage.config.encrypted " + - "+resources.storage.config.iv " + - "+resources.storage.config.tag" - ); + }).select( + "publishableKey secretKey " + + "+resources.storage.config.encrypted " + + "+resources.storage.config.iv " + + "+resources.storage.config.tag" + ); @@ await Project.deleteOne({ _id: projectId }); + await deleteProjectById(projectId.toString()); + await deleteProjectByApiKeyCache(project.publishableKey); + await deleteProjectByApiKeyCache(project.secretKey); storageRegistry.delete(projectId.toString());Also applies to: 829-832
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/project.controller.js` around lines 779 - 785, When a project is deleted the Redis cache keys for that project aren't purged, so getSingleProject and verifyApiKey can still return stale data; after the code that deletes the project (the block performing Project.deleteOne / project removal) add logic to delete Redis keys matching the project's patterns (e.g., "project:id:<projectId>*" and "project:apikey:*" for any API keys belonging to that project). Implement this using a safe SCAN loop (or Redis KEYS only in dev) to collect matching keys and then DEL/UNLINK them, and call this cleanup from the same delete handler that removes the Mongo row so cache and auth entries are invalidated immediately (refer to getSingleProject and verifyApiKey where those key patterns are read).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/project.controller.js`:
- Around line 882-891: The toggleAuth handler is returning a raw project and
only deleting cache keys matching project:id:*, causing API-key-authenticated
requests to still read stale isAuthEnabled and exposing fields normally
stripped; update the cache invalidation in toggleAuth to also delete API-key
related cache keys (e.g. patterns that include project:apikey:* or whatever key
deleteProjectById misses) after saving, and change the response to return the
sanitized shape used by getSingleProject (call or reuse the same sanitize/format
function that getSingleProject uses) so you return the same filtered project
object and up-to-date isAuthEnabled to clients.
In `@backend/controllers/userAuth.controller.js`:
- Around line 42-53: The code inserts the user with
collection.insertOne(newUser) before performing redis.set and
authEmailQueue.add, which can leave an unverified user if those side effects
fail; change the flow to either perform side effects first and then insert, or
(simpler) wrap the post-insert side effects in a try/catch and on any failure
remove the created record using the insertedId (call collection.deleteOne({_id:
result.insertedId})) before rethrowing the error so retries get a clean state;
reference collection.insertOne, result.insertedId, redis.set,
authEmailQueue.add, newUser, otp and ensure cleanup runs before returning an
error response.
- Around line 372-417: Both getUserDetails and updateAdminUser are missing
ownership checks allowing any authenticated user to access other projects; add
the same project membership/ownership guard used by
createAdminUser/resetPassword: verify req.user exists and that req.user's
project list (e.g., req.user.projectIds or req.user.projects) includes the
projectId from req.params before any DB access, return a 403 Forbidden JSON
response if the check fails, and place this check at the top of the functions
(getUserDetails and updateAdminUser) so the collection lookup/update is only
performed for authorized callers.
In `@backend/queues/authEmailQueue.js`:
- Around line 9-16: The Worker callback in auth-email-queue currently logs raw
recipient emails (console.log/console.error) using job.data.email; replace those
raw email usages with a redacted or non-PII identifier (e.g., maskLocalPart or
show only domain, or use job.id/hashedEmail) before logging, and update both the
success log (`console.log(`[Queue] Processing ...`)`) and error log
(`console.error(`[Queue] Failed to send ...`)`) to reference the redacted
identifier; ensure sendAuthOtpEmail is still called with the real email but no
raw email is written to logs anywhere in this function.
In `@backend/routes/projects.js`:
- Around line 107-111: The admin user routes
(router.post('/:projectId/admin/users',
router.patch('/:projectId/admin/users/:userId/password',
router.get('/:projectId/admin/users/:userId',
router.put('/:projectId/admin/users/:userId')) are currently protected only by
authMiddleware and thus remain reachable when authentication is globally
disabled; update these route registrations to also require the auth-enabled
guard from backend/middleware/checkAuthEnabled.js (i.e., apply the
checkAuthEnabled middleware before authMiddleware on createAdminUser,
resetPassword, getUserDetails, and updateAdminUser) so the routes are
inaccessible when auth is not enabled or the users schema is not set up.
In `@backend/utils/emailService.js`:
- Around line 156-180: pname (used in htmlContent, the From header and replyTo)
is developer-controlled and must be sanitized/validated: HTML-escape or strip
any tags/entities before embedding into the email body (htmlContent) and build a
safe display name; derive safeEmailHandle by removing non-alphanumerics,
truncate to a reasonable length, and if the result is empty use a fixed fallback
local-part (e.g. "no-reply-urbackend") to avoid creating invalid addresses;
ensure the From/replyTo passed to resend.emails.send use the validated display
name and the guaranteed-valid local-part@apps.bitbros.in, and also guard against
header-injection characters in pname before concatenation.
In `@frontend/src/components/DatabaseSidebar.jsx`:
- Line 20: The sidebar currently builds visibleCollections via
collections.filter(c => c.name !== 'users' || activeCollection?.name ===
'users'), which lets a URL-driven or default activeCollection expose the
protected "users" entry; instead, change the gating so the 'users' row is shown
only when rendering that row or when an explicit mode flag is set (e.g., use a
showUsers flag or props.mode === 'admin') — update the visibleCollections filter
to check that flag (not activeCollection) before including the 'users'
collection, and also modify the initial-selection logic (where activeCollection
is set) so it does not default to 'users' unless the request explicitly asked
for it.
In `@frontend/src/pages/Auth.jsx`:
- Around line 92-93: The Add User modal uses usersCollection.model from Auth.jsx
but the backend (project.controller.js) strips out password from the model so
createAdminUser fails; update the form generation in Auth.jsx (where
usersCollection is used) to ensure the auth fields are present by either: (a)
augment the model before passing to the form with explicit fields email,
username, and password (marking password as required), or (b) stop using
usersCollection.model directly and build a minimal form schema containing email,
username, and password, and submit those to createAdminUser; reference
usersCollection and createAdminUser when making the change.
In `@frontend/src/pages/CreateCollection.jsx`:
- Around line 327-343: When creating a "users" collection we must enforce the
auth schema at submit, not only prefill it. In CreateCollection.jsx update the
submit handler that uses the fields state (the function that creates the
collection / handles form submit) to check if name === 'users' and then ensure
fields contains keys "email" and "password" with type "String" and required:
true (either reject the submit with a clear validation error or automatically
insert those field objects into fields via setFields). Also mark those objects
as locked/immutable (e.g., add a locked: true flag) and update the UI field
renderer to prevent editing/removal of fields where locked === true; reference
getInitialFields, createEmptyField, fields, setFields and the submit handler
(e.g., handleCreate/onSubmit) when making these changes.
---
Outside diff comments:
In `@backend/controllers/project.controller.js`:
- Around line 779-785: When a project is deleted the Redis cache keys for that
project aren't purged, so getSingleProject and verifyApiKey can still return
stale data; after the code that deletes the project (the block performing
Project.deleteOne / project removal) add logic to delete Redis keys matching the
project's patterns (e.g., "project:id:<projectId>*" and "project:apikey:*" for
any API keys belonging to that project). Implement this using a safe SCAN loop
(or Redis KEYS only in dev) to collect matching keys and then DEL/UNLINK them,
and call this cleanup from the same delete handler that removes the Mongo row so
cache and auth entries are invalidated immediately (refer to getSingleProject
and verifyApiKey where those key patterns are read).
In `@backend/controllers/userAuth.controller.js`:
- Around line 84-95: The login flow issues JWTs without verifying emailVerified:
after retrieving the user via collection.findOne({ email }) and before calling
jwt.sign, check user.emailVerified (or the appropriate verified flag on the user
document) and if it is falsy return a 403/400 JSON error (e.g., { error: "Email
not verified" }) instead of issuing a token; update the early-return logic
around validPass/jwt.sign to reject unverified users so jwt.sign is only called
when user.emailVerified is true.
In `@backend/middleware/verifyApiKey.js`:
- Around line 19-46: The API-key cache eviction is missing for mutations that
change project fields; update toggleAuth() and deleteCollection() to call
deleteProjectByApiKeyCache() for both the publishable and secret API key
variants after making database changes (same pattern used in updateProject() and
createCollection()); locate toggleAuth() and deleteCollection() and, after
successful update/delete and after any ID-based cache clears, invoke
deleteProjectByApiKeyCache(publishableKeyHash) and
deleteProjectByApiKeyCache(secretKeyHash) so API-key cache entries are
invalidated along with the existing ID-based cache clears.
In `@frontend/src/pages/ProjectDetails.jsx`:
- Around line 221-240: The empty-state check currently uses
project.collections.filter(c => c.name !== 'users').length === 0 which shows “No
Data Yet” when the only collection is the hidden 'users' collection; change the
empty-state condition to check the raw collection count
(project.collections.length === 0) so the UI only shows the empty state when the
project truly has no collections, while keeping the mapping/rendering code that
filters out 'users' (the map/filter expressions referencing c => c.name !==
'users') intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb8977cd-efc1-4883-96e6-14925e65feb5
📒 Files selected for processing (16)
backend/app.jsbackend/controllers/project.controller.jsbackend/controllers/userAuth.controller.jsbackend/middleware/checkAuthEnabled.jsbackend/middleware/verifyApiKey.jsbackend/models/Project.jsbackend/queues/authEmailQueue.jsbackend/queues/emailQueue.jsbackend/routes/projects.jsbackend/routes/userAuth.jsbackend/utils/emailService.jsbackend/utils/network.jsfrontend/src/components/DatabaseSidebar.jsxfrontend/src/pages/Auth.jsxfrontend/src/pages/CreateCollection.jsxfrontend/src/pages/ProjectDetails.jsx
💤 Files with no reviewable changes (1)
- backend/utils/network.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/CreateCollection.jsx (1)
396-410:⚠️ Potential issue | 🟠 MajorMirror the
usersschema validation on the backend too.This guard only protects the Create Collection page. The backend can still enable auth with an invalid
usersschema:backend/controllers/project.controller.jsauto-createsuserswithmodel: [], andbackend/middleware/checkAuthEnabled.jsonly checks that the collection exists. That means this PR still leaves a path where auth is "enabled" but signup/login run against a schema this UI would reject.Please enforce the same
passwordrequired-Stringcontract in the collection creation/auth-toggle API path, not just here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/CreateCollection.jsx` around lines 396 - 410, The backend must enforce the same users-schema rule: add validation in the collection-creation handler (the controller method that handles creating collections, e.g., the createCollection / project controller function) and in the auth toggle/enabling endpoint (the function that enables auth) to reject any request that would create or enable a 'users' collection unless the schema contains a required String field 'email' and a required String field 'password'; also update the auto-create-users logic (the code path that currently seeds users with model: []) to initialize the users model with those required fields instead of an empty model, and update the checkAuthEnabled middleware to verify the users collection schema meets the same contract and return a 400/422 error if it does not.
♻️ Duplicate comments (3)
frontend/src/pages/Database.jsx (1)
285-285:⚠️ Potential issue | 🟠 MajorSelection-driven
showUsersstill re-exposes the protected collection.Because this prop turns true whenever
activeCollectionbecomesusers, a?collection=usersURL or ausers-first response from the initial selection logic will still surface the hidden collection in the general Database view. Make this an explicit mode/permission flag instead, and also ensure the initial collection resolver skipsusershere.Suggested fix
- showUsers={activeCollection?.name === 'users'} + showUsers={false}You should also update the upstream selection logic in
fetchProject()soqueryCollectionand the default fallback do not pickuserson this page.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Database.jsx` at line 285, Replace the selection-driven showUsers prop (currently set via activeCollection?.name === 'users') with an explicit mode or permission flag so the hidden users collection is only shown when the component is in an allowed/users mode (update the prop name used by Database.jsx and the parent that sets it), and change fetchProject() selection logic (the queryCollection and default fallback resolver) so it never returns 'users' for the Database view initial collection; update references to activeCollection, showUsers, queryCollection, and fetchProject to use the new explicit flag and ensure the initial resolver skips 'users'.backend/controllers/userAuth.controller.js (1)
48-59:⚠️ Potential issue | 🟠 MajorOrphaned unverified accounts possible on side-effect failures.
The user is inserted (line 48) before Redis/queue operations (lines 51, 54-59). If either fails, an unverified user record persists, and retries hit "User already exists" without receiving a fresh OTP.
🔧 Proposed fix: wrap side effects and clean up on failure
const result = await collection.insertOne(newUser); - // Save OTP to Redis (5 mins expiry) - await redis.set(`project:${project._id}:otp:verification:${email}`, otp, 'EX', 300); - - // Queue Email - await authEmailQueue.add('send-verification-email', { - email, - otp, - type: 'verification', - pname: project.name - }); + try { + // Save OTP to Redis (5 mins expiry) + await redis.set(`project:${project._id}:otp:verification:${email}`, otp, 'EX', 300); + + // Queue Email + await authEmailQueue.add('send-verification-email', { + email, + otp, + type: 'verification', + pname: project.name + }); + } catch (sideEffectErr) { + // Rollback: remove the partially created user + await collection.deleteOne({ _id: result.insertedId }); + throw sideEffectErr; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/userAuth.controller.js` around lines 48 - 59, The insertOne of newUser (result) happens before side-effects (redis.set and authEmailQueue.add), which can leave orphaned unverified users if those fail; modify the signup flow in this controller so that either (a) perform side-effects first and only call collection.insertOne(newUser) after they succeed, or (b) keep the insert but wrap redis.set and authEmailQueue.add in a try/catch and on any failure clean up by deleting the created user (use collection.deleteOne({_id: result.insertedId})) and re-throw the error so callers see the failure; ensure you reference result, newUser, redis.set, and authEmailQueue.add and that errors are logged/propagated.frontend/src/pages/CreateCollection.jsx (1)
126-179:⚠️ Potential issue | 🟡 MinorMake locked auth fields fully immutable.
Line 130 and Line 171 block rename/delete, but Line 144 and Line 160 still let a locked field change
typeandrequired. That leaves thepasswordrows editable in ways the lock contract is supposed to prevent.Suggested fix
<select value={field.type} + disabled={field.locked} onChange={(e) => handleChange('type', e.target.value)} className="input-field" style={{ flex: 1, border: 'none', background: 'transparent', - padding: '4px 0', cursor: 'pointer', fontSize: '0.9rem' + padding: '4px 0', + cursor: field.locked ? 'not-allowed' : 'pointer', + fontSize: '0.9rem', + opacity: field.locked ? 0.6 : 1 }} > @@ <input type="checkbox" checked={field.required} + disabled={field.locked} onChange={(e) => handleChange('required', e.target.checked)} style={{ accentColor: 'var(--color-primary)', - transform: 'scale(1.1)', cursor: 'pointer', flexShrink: 0 + transform: 'scale(1.1)', + cursor: field.locked ? 'not-allowed' : 'pointer', + flexShrink: 0, + opacity: field.locked ? 0.6 : 1 }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/CreateCollection.jsx` around lines 126 - 179, The auth-locked fields are still editable via the type selector and required checkbox; update the field UI logic so when field.locked is true both the <select> (value={field.type}) and the required <input type="checkbox"> ignore changes and are visually/semantically disabled: prevent their onChange handlers from mutating state (use the same field.locked guard as the text input and delete button), set disabled/aria-disabled on the <select> and checkbox, and adjust styles/cursor/opactiy accordingly; ensure handleChange and onRemove respect field.locked (e.g., in the onChange callbacks for 'type' and 'required' and in onRemove) so locked auth rows (like email/password) cannot change type or required state.
🧹 Nitpick comments (3)
frontend/src/pages/Auth.jsx (2)
222-225:forEachcallback should not return a value.Static analysis correctly flags that
deletereturns a boolean, andforEachcallbacks should not return values. While this doesn't cause a bug (forEach ignores return values), it's cleaner to use explicit block syntax.Proposed fix
const customFields = { ...res.data }; - ['_id', 'email', 'password', 'emailVerified', 'createdAt', 'updatedAt'].forEach(key => delete customFields[key]); + ['_id', 'email', 'password', 'emailVerified', 'createdAt', 'updatedAt'].forEach(key => { + delete customFields[key]; + }); setEditFormData(customFields);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Auth.jsx` around lines 222 - 225, The forEach callback on the customFields deletion is returning a value due to using a concise arrow with delete (delete returns a boolean); change the callback to an explicit block or use a simple for..of loop to perform deletions without returning a value. Update the code around customFields and the loop that deletes '_id', 'email', 'password', 'emailVerified', 'createdAt', 'updatedAt' (the forEach currently used) so the loop body only performs delete operations and then call setEditFormData(customFields).
194-196: Password reset usesprompt()which exposes the new password on screen.The browser's
prompt()dialog displays text in plaintext. Consider using a modal with a password input field for better security hygiene, especially in shared or screen-recording scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Auth.jsx` around lines 194 - 196, The code exposes plaintext passwords via prompt() — replace the prompt-based flow in Auth.jsx (the code creating newPassword via prompt and the subsequent length check) with a secure modal dialog that contains a password-type input field and explicit confirm/cancel controls; validate the entered value in the modal (non-empty and min length 6), show toast.error for validation failures, and submit the password to the existing reset handler only after the user confirms, ensuring no plaintext password is shown via window.prompt().backend/controllers/userAuth.controller.js (1)
316-317: Consider adding an upper bound for username length.The validation only enforces a minimum length of 3 characters. Adding a maximum (e.g., 50-100 chars) would prevent excessively long usernames that could cause display issues or storage concerns.
💡 Suggested validation
const username = req.body.username; - if (!username || username.length < 3) return res.status(400).json({ error: "Username must be at least 3 characters." }); + if (!username || username.length < 3 || username.length > 50) { + return res.status(400).json({ error: "Username must be between 3 and 50 characters." }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/userAuth.controller.js` around lines 316 - 317, The username validation only enforces a minimum length; update the check around the username variable (where const username = req.body.username is read and the if that currently checks username.length < 3) to also enforce an upper bound (e.g., username.length > 50) and return a 400 with a clear message like "Username must be between 3 and 50 characters." Ensure the same validation is applied wherever username is accepted (e.g., registration or profile update handlers) and adjust any related tests or error messages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/routes/projects.js`:
- Around line 108-112: The admin routes call checkAuthEnabled but req.project is
never set because authMiddleware authenticates the user but does not populate
req.project (verifyApiKey does); fix by loading the project before
checkAuthEnabled: add a middleware (e.g., loadProjectForAdmin) that finds the
Project by req.params.projectId and validates ownership against req.user._id,
sets req.project, and returns 404 on failure, then apply loadProjectForAdmin
before checkAuthEnabled in the route chain for createAdminUser, resetPassword,
getUserDetails, and updateAdminUser; alternatively, if you prefer
controller-level changes, perform the same auth-enabled check after fetching the
project inside the controller handlers (createAdminUser, resetPassword,
getUserDetails, updateAdminUser).
In `@backend/utils/emailService.js`:
- Around line 129-145: In sendAuthOtpEmail: the sanitization order is wrong and
you're reusing HTML-escaped pname for different contexts; derive safeEmailHandle
from the raw pname (strip non-alphanumeric, toLowerCase, fallback/truncate)
before any HTML-escaping, create safeProjectNameHtml = escapeHtml(pname ||
'urBackend') for use inside the HTML logo, and create safeDisplayName by
stripping CR and LF/newlines from the raw pname, then escaping or quoting it
appropriately for email headers (From/ReplyTo) so header injection is prevented;
replace usages of sPname/safeEmailHandle in the logo and headers with
safeProjectNameHtml and safeDisplayName respectively.
In `@frontend/src/pages/Auth.jsx`:
- Line 182: The reset call for the Add User form only clears email and password
so username can persist; update the setAddFormData invocation (the state setter
used for the Add User modal form) to also reset username to its initial empty
value so the form state matches the original initial state (the variable managed
by setAddFormData).
- Around line 51-52: The number input shows empty when formData[field.key] is 0
because the code uses the || operator; change the value expression to use
nullish coalescing so legitimate zeros render: replace
value={formData[field.key] || ''} with value={formData[field.key] ?? ''} (keep
the existing onChange handler that converts e.target.value to Number); this
targets the value binding for the input in Auth.jsx where formData and field.key
are used.
---
Outside diff comments:
In `@frontend/src/pages/CreateCollection.jsx`:
- Around line 396-410: The backend must enforce the same users-schema rule: add
validation in the collection-creation handler (the controller method that
handles creating collections, e.g., the createCollection / project controller
function) and in the auth toggle/enabling endpoint (the function that enables
auth) to reject any request that would create or enable a 'users' collection
unless the schema contains a required String field 'email' and a required String
field 'password'; also update the auto-create-users logic (the code path that
currently seeds users with model: []) to initialize the users model with those
required fields instead of an empty model, and update the checkAuthEnabled
middleware to verify the users collection schema meets the same contract and
return a 400/422 error if it does not.
---
Duplicate comments:
In `@backend/controllers/userAuth.controller.js`:
- Around line 48-59: The insertOne of newUser (result) happens before
side-effects (redis.set and authEmailQueue.add), which can leave orphaned
unverified users if those fail; modify the signup flow in this controller so
that either (a) perform side-effects first and only call
collection.insertOne(newUser) after they succeed, or (b) keep the insert but
wrap redis.set and authEmailQueue.add in a try/catch and on any failure clean up
by deleting the created user (use collection.deleteOne({_id:
result.insertedId})) and re-throw the error so callers see the failure; ensure
you reference result, newUser, redis.set, and authEmailQueue.add and that errors
are logged/propagated.
In `@frontend/src/pages/CreateCollection.jsx`:
- Around line 126-179: The auth-locked fields are still editable via the type
selector and required checkbox; update the field UI logic so when field.locked
is true both the <select> (value={field.type}) and the required <input
type="checkbox"> ignore changes and are visually/semantically disabled: prevent
their onChange handlers from mutating state (use the same field.locked guard as
the text input and delete button), set disabled/aria-disabled on the <select>
and checkbox, and adjust styles/cursor/opactiy accordingly; ensure handleChange
and onRemove respect field.locked (e.g., in the onChange callbacks for 'type'
and 'required' and in onRemove) so locked auth rows (like email/password) cannot
change type or required state.
In `@frontend/src/pages/Database.jsx`:
- Line 285: Replace the selection-driven showUsers prop (currently set via
activeCollection?.name === 'users') with an explicit mode or permission flag so
the hidden users collection is only shown when the component is in an
allowed/users mode (update the prop name used by Database.jsx and the parent
that sets it), and change fetchProject() selection logic (the queryCollection
and default fallback resolver) so it never returns 'users' for the Database view
initial collection; update references to activeCollection, showUsers,
queryCollection, and fetchProject to use the new explicit flag and ensure the
initial resolver skips 'users'.
---
Nitpick comments:
In `@backend/controllers/userAuth.controller.js`:
- Around line 316-317: The username validation only enforces a minimum length;
update the check around the username variable (where const username =
req.body.username is read and the if that currently checks username.length < 3)
to also enforce an upper bound (e.g., username.length > 50) and return a 400
with a clear message like "Username must be between 3 and 50 characters." Ensure
the same validation is applied wherever username is accepted (e.g., registration
or profile update handlers) and adjust any related tests or error messages
accordingly.
In `@frontend/src/pages/Auth.jsx`:
- Around line 222-225: The forEach callback on the customFields deletion is
returning a value due to using a concise arrow with delete (delete returns a
boolean); change the callback to an explicit block or use a simple for..of loop
to perform deletions without returning a value. Update the code around
customFields and the loop that deletes '_id', 'email', 'password',
'emailVerified', 'createdAt', 'updatedAt' (the forEach currently used) so the
loop body only performs delete operations and then call
setEditFormData(customFields).
- Around line 194-196: The code exposes plaintext passwords via prompt() —
replace the prompt-based flow in Auth.jsx (the code creating newPassword via
prompt and the subsequent length check) with a secure modal dialog that contains
a password-type input field and explicit confirm/cancel controls; validate the
entered value in the modal (non-empty and min length 6), show toast.error for
validation failures, and submit the password to the existing reset handler only
after the user confirms, ensuring no plaintext password is shown via
window.prompt().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c081a20-7dc2-4824-8b39-80322f5a0939
📒 Files selected for processing (12)
backend/app.jsbackend/controllers/project.controller.jsbackend/controllers/userAuth.controller.jsbackend/queues/authEmailQueue.jsbackend/routes/auth.jsbackend/routes/projects.jsbackend/utils/emailService.jsfrontend/src/components/DatabaseSidebar.jsxfrontend/src/pages/Auth.jsxfrontend/src/pages/CreateCollection.jsxfrontend/src/pages/Database.jsxfrontend/src/pages/ProjectSettings.jsx
💤 Files with no reviewable changes (1)
- frontend/src/pages/ProjectSettings.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/queues/authEmailQueue.js
Collaborator
Author
|
@coderabbitai chat is this project will be good dev tool, in future??? |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Signs pointing in a promising direction 🚀
Areas to keep investing in 💡
Bottom line: The trajectory looks solid. The decisions being made in this PR — abstraction, security-first thinking, and DX improvements — are exactly the kinds of investments that turn a side project into a tool developers trust. Keep shipping! 💪 (`・ω・´)ﾉ |
…, and email sanitization
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/controllers/project.controller.js (1)
463-464:⚠️ Potential issue | 🟡 MinorDuplicate
project.save()call.
project.save()is called twice in succession, causing an unnecessary database write operation.🐛 Proposed fix
if (!project.resources.db.isExternal) { project.databaseUsed = (project.databaseUsed || 0) + docSize; } await project.save(); - await project.save(); res.json(result);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/project.controller.js` around lines 463 - 464, There is a duplicated database write: remove the redundant project.save() call so the entity is persisted only once; locate the consecutive calls to project.save() (the duplicate after the first await project.save()) in the controller function and delete the extra await project.save() to avoid the unnecessary second write.backend/controllers/userAuth.controller.js (1)
21-24:⚠️ Potential issue | 🟡 MinorDuplicate schema validation.
userSignupSchema.parse(req.body)is called twice (Lines 21 and 24). The result from Line 21 (data) is unused.🐛 Proposed fix
const project = req.project; - const data = userSignupSchema.parse(req.body); - // Zod Validation (Prevents NoSQL Injection too) const { email, password, username, ...otherData } = userSignupSchema.parse(req.body);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/userAuth.controller.js` around lines 21 - 24, Remove the duplicate call to userSignupSchema.parse(req.body): stop calling it twice and reuse the parsed result; replace the second parse invocation by destructuring from the already-parsed variable (either use the existing data variable or eliminate data and parse once into { email, password, username, ...otherData }). Update the code around userSignupSchema.parse, data, and the destructuring of email/password/username to ensure only one parse occurs and the parsed object is used.
♻️ Duplicate comments (1)
backend/controllers/userAuth.controller.js (1)
48-59:⚠️ Potential issue | 🟠 MajorOrphaned unverified accounts remain on side-effect failures.
If
redis.set(Line 51) orauthEmailQueue.add(Line 54) fails after the user is inserted (Line 48), the signup returns 500 but leaves an unverified user in the database. Subsequent signup attempts will fail with "User already exists" without providing a fresh OTP.🛡️ Proposed fix - rollback on failure
const result = await collection.insertOne(newUser); + try { // Save OTP to Redis (5 mins expiry) await redis.set(`project:${project._id}:otp:verification:${email}`, otp, 'EX', 300); // Queue Email await authEmailQueue.add('send-verification-email', { email, otp, type: 'verification', pname: project.name }); + } catch (sideEffectErr) { + // Rollback: remove the created user to allow retry + await collection.deleteOne({ _id: result.insertedId }); + throw sideEffectErr; + } const token = jwt.sign(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/userAuth.controller.js` around lines 48 - 59, After inserting the user via collection.insertOne(newUser) you must treat redis.set(...) and authEmailQueue.add(...) as part of the same operation and rollback the DB insert if either side-effect fails: wrap the redis.set and authEmailQueue.add calls in a try/catch, and in the catch call collection.deleteOne({ _id: result.insertedId }) (or equivalent) to remove the orphaned unverified user, then rethrow or return the original error response; ensure you handle/delete by the insertedId from the insertOne result and log any rollback failures to aid debugging.
🧹 Nitpick comments (3)
frontend/src/pages/Auth.jsx (2)
268-269: Consider replacing nativeconfirm()with a styled confirmation modal.The delete confirmation uses the browser's native
confirm()dialog, which is visually inconsistent with the polished custom modals used elsewhere in this component (Add User, Edit User, Reset Password). For a cohesive UX, consider implementing a styled confirmation modal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Auth.jsx` around lines 268 - 269, Replace the native browser confirm() used in handleDelete with the app's styled confirmation modal: remove the confirm() call in handleDelete and instead open the existing modal component (or create a simple reusable ConfirmModal) that matches Add/Edit/Reset modals, pass the message ("Delete this user permanently? They won't be able to login."), and only proceed with the delete flow when the modal resolves/returns a positive confirmation; update any state hooks (e.g., showConfirm, confirmPayload, onConfirm callback) and the code paths that call the actual delete logic so they trigger after the modal confirmation.
267-281: Route design inconsistency: Consider consolidating with admin user operations.The
deleteRowendpoint (line 63) lacks the explicit middleware (loadProjectForAdmin,checkAuthEnabled) used by other admin user operations (create, update, reset password), creating a pattern inconsistency. While the controller does validate project ownership at line 476, consider aligning this endpoint with the dedicated/admin/userspattern used elsewhere for clarity and maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Auth.jsx` around lines 267 - 281, The delete user flow is calling a non-admin route from handleDelete while other admin user ops use the /admin/users pattern and middleware; update the endpoint and middleware to be consistent by routing deletes through the admin users path and enforcing loadProjectForAdmin and checkAuthEnabled (or mirror their checks) before invoking the controller's deleteRow (which currently does ownership validation at line ~476). Concretely, change the route to the consolidated /api/projects/:projectId/admin/users/... and ensure the route registration attaches loadProjectForAdmin and checkAuthEnabled, or alternatively update handleDelete to call the existing admin delete endpoint if already present, and remove the inconsistent non-admin delete route. Ensure the controller invoked remains deleteRow and retains the ownership validation.backend/utils/emailService.js (1)
179-179: Consider escaping the OTP value in HTML output.While the OTP is server-generated as a 6-digit numeric string, defensive HTML escaping would prevent any edge cases and maintain consistency with the sanitization approach used elsewhere in this function.
♻️ Suggested change
- <div class="otp-box">${otp}</div> + <div class="otp-box">${escapeHtml(String(otp))}</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils/emailService.js` at line 179, Escape the OTP before interpolating into the HTML to prevent any edge cases; locate where the HTML template contains '<div class="otp-box">${otp}</div>' in emailService.js (e.g., the function that builds/sends the OTP email) and replace the raw ${otp} with a safely escaped value by calling the module's existing HTML-escaping/sanitization utility (or add a small escapeHtml function) so the interpolated value is always escaped before inclusion in the template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/userAuth.controller.js`:
- Around line 61-64: JWT tokens are being created with jwt.sign without an
expiration; update the token creation in the signup/register flow (where
jwt.sign is called with { userId: result.insertedId, projectId: project._id }
and project.jwtSecret) to include an expiresIn option (e.g., expiresIn: '1h' or
a project-configured TTL), and apply the same change to the login flow where
jwt.sign is used for issuing tokens so both signup and login tokens expire
appropriately.
- Around line 396-409: The update payload (updateData) is passed directly into
{$set} allowing NoSQL operator injection; fix by importing and using the
existing sanitize function from input.validation.js to remove any keys beginning
with '$' (and nested operator keys) before calling collection.updateOne.
Specifically, in the controller where you build updateData (and after you delete
password and _id) call sanitize(updateData) and use the sanitized object in the
updateOne {$set: sanitizedUpdateData} call (the relevant symbols: updateData,
sanitize, getAuthCollection, collection.updateOne).
In `@backend/utils/emailService.js`:
- Around line 145-147: The display name quoting logic for
finalDisplayName/safeDisplayName fails to escape backslashes, causing malformed
headers when names contain "\"; update the replacement to first escape
backslashes and then escape quotes (i.e., replace backslashes with double
backslashes, then replace " with \" ) in the branch that builds the quoted name
so the resulting header string is valid.
In `@frontend/src/pages/CreateCollection.jsx`:
- Around line 337-358: The form currently special-cases auth only when
initialName === 'users' but leaves the editable name uncontrolled, so fix by
enforcing the exact lowercase 'users' name: when initialName === 'users' disable
the name input (prevent edits via the UI by tying name to initialName and
disabling the field) and also normalize/validate the name before submit (in the
submit handler ensure name is trimmed and lowercased and reject or replace any
non-exact 'users' variants); update related references getInitialFields, name,
setName, fields, setFields and any submit handler to guarantee the backend
always receives the canonical 'users' string.
---
Outside diff comments:
In `@backend/controllers/project.controller.js`:
- Around line 463-464: There is a duplicated database write: remove the
redundant project.save() call so the entity is persisted only once; locate the
consecutive calls to project.save() (the duplicate after the first await
project.save()) in the controller function and delete the extra await
project.save() to avoid the unnecessary second write.
In `@backend/controllers/userAuth.controller.js`:
- Around line 21-24: Remove the duplicate call to
userSignupSchema.parse(req.body): stop calling it twice and reuse the parsed
result; replace the second parse invocation by destructuring from the
already-parsed variable (either use the existing data variable or eliminate data
and parse once into { email, password, username, ...otherData }). Update the
code around userSignupSchema.parse, data, and the destructuring of
email/password/username to ensure only one parse occurs and the parsed object is
used.
---
Duplicate comments:
In `@backend/controllers/userAuth.controller.js`:
- Around line 48-59: After inserting the user via collection.insertOne(newUser)
you must treat redis.set(...) and authEmailQueue.add(...) as part of the same
operation and rollback the DB insert if either side-effect fails: wrap the
redis.set and authEmailQueue.add calls in a try/catch, and in the catch call
collection.deleteOne({ _id: result.insertedId }) (or equivalent) to remove the
orphaned unverified user, then rethrow or return the original error response;
ensure you handle/delete by the insertedId from the insertOne result and log any
rollback failures to aid debugging.
---
Nitpick comments:
In `@backend/utils/emailService.js`:
- Line 179: Escape the OTP before interpolating into the HTML to prevent any
edge cases; locate where the HTML template contains '<div
class="otp-box">${otp}</div>' in emailService.js (e.g., the function that
builds/sends the OTP email) and replace the raw ${otp} with a safely escaped
value by calling the module's existing HTML-escaping/sanitization utility (or
add a small escapeHtml function) so the interpolated value is always escaped
before inclusion in the template.
In `@frontend/src/pages/Auth.jsx`:
- Around line 268-269: Replace the native browser confirm() used in handleDelete
with the app's styled confirmation modal: remove the confirm() call in
handleDelete and instead open the existing modal component (or create a simple
reusable ConfirmModal) that matches Add/Edit/Reset modals, pass the message
("Delete this user permanently? They won't be able to login."), and only proceed
with the delete flow when the modal resolves/returns a positive confirmation;
update any state hooks (e.g., showConfirm, confirmPayload, onConfirm callback)
and the code paths that call the actual delete logic so they trigger after the
modal confirmation.
- Around line 267-281: The delete user flow is calling a non-admin route from
handleDelete while other admin user ops use the /admin/users pattern and
middleware; update the endpoint and middleware to be consistent by routing
deletes through the admin users path and enforcing loadProjectForAdmin and
checkAuthEnabled (or mirror their checks) before invoking the controller's
deleteRow (which currently does ownership validation at line ~476). Concretely,
change the route to the consolidated /api/projects/:projectId/admin/users/...
and ensure the route registration attaches loadProjectForAdmin and
checkAuthEnabled, or alternatively update handleDelete to call the existing
admin delete endpoint if already present, and remove the inconsistent non-admin
delete route. Ensure the controller invoked remains deleteRow and retains the
ownership validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 791e9685-bebc-46df-9f50-f5b93d51e36e
📒 Files selected for processing (10)
backend/controllers/project.controller.jsbackend/controllers/userAuth.controller.jsbackend/middleware/checkAuthEnabled.jsbackend/middleware/loadProjectForAdmin.jsbackend/routes/projects.jsbackend/utils/emailService.jsbackend/utils/input.validation.jsfrontend/src/pages/Auth.jsxfrontend/src/pages/CreateCollection.jsxfrontend/src/pages/Database.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/Database.jsx
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/src/pages/CreateCollection.jsx (1)
362-372:⚠️ Potential issue | 🔴 CriticalCritical:
useStatemisused for side effects — should beuseEffect.
useStateaccepts only an initial value or a lazy initializer. Here the callback fetches data (a side effect) and returns nothing, while the[]dependency array is ignored. This accidentally works because React calls the initializer once, but it violates React's model and will break under StrictMode's double-invocation or future React behavior.🐛 Proposed fix
- // Fetch existing collections for Ref picker - useState(() => { - const fetchCollections = async () => { - try { - const res = await axios.get(`${API_URL}/api/projects/${projectId}`, { - headers: { Authorization: `Bearer ${token}` } - }); - setCollections(res.data.collections || []); - } catch { /* ignore */ } - }; - fetchCollections(); - }, []); + // Fetch existing collections for Ref picker + useEffect(() => { + const fetchCollections = async () => { + try { + const res = await axios.get(`${API_URL}/api/projects/${projectId}`, { + headers: { Authorization: `Bearer ${token}` } + }); + setCollections(res.data.collections || []); + } catch { /* ignore */ } + }; + fetchCollections(); + }, [projectId, token]);Also add
useEffectto the imports at line 1:-import { useState } from 'react'; +import { useState, useEffect } from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/CreateCollection.jsx` around lines 362 - 372, The code is using useState with a side-effecting initializer to fetch collections; replace that with useEffect so the fetchRuns as a proper effect: import useEffect alongside useState at the top, change the block that defines and calls fetchCollections to run inside useEffect (useEffect(() => { const fetchCollections = async () => { ... }; fetchCollections(); }, [projectId, token]) ), keep using setCollections and axios, and handle the catch (don’t leave it empty) — locate the fetchCollections function and setCollections usage in CreateCollection.jsx and convert the useState initializer to a useEffect hook.frontend/src/pages/Database.jsx (1)
100-112:⚠️ Potential issue | 🟠 MajorAlways replace or clear
activeCollectionwhen the URL collection is invalid.On Lines 100-112, a truthy
collectionquery param that is no longer present inres.data.collectionsskips the fallback branch and leaves the previousactiveCollectionintact. That can keep the page bound to a stale collection after a stale deep link or a project switch, instead of resetting to a valid collection for the newly loaded project. Compute a fallback even whenqueryCollectionis missing from the payload, or explicitly clear the selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Database.jsx` around lines 100 - 112, If a truthy queryCollection string isn't found in res.data.collections, clear or replace the stale activeCollection instead of leaving it unchanged: inside the load/response handler where you read queryCollection and call setActiveCollection, check whether found = res.data.collections.find(...) is truthy; if not, compute the fallback (use filtered = res.data.collections.filter(c => c.name !== 'users'), then choose filtered[0] or res.data.collections[0] if available) and call setActiveCollection with that fallback, or call setActiveCollection(null) to explicitly clear selection when there are no valid collections; update the logic around queryCollection, found, filtered, and setActiveCollection to always set a value (or null) so activeCollection is never left stale.backend/controllers/userAuth.controller.js (1)
23-44:⚠️ Potential issue | 🔴 CriticalStrip system-managed fields out of
otherDatabefore persisting.Because
userSignupSchemais.passthrough()and...otherDatais spread afteremailVerified, a signup request can sendemailVerified: trueand mark itself verified immediately. The same merge pattern increateAdminUseralso lets request payloads override other reserved keys like_id.Also applies to: 143-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/userAuth.controller.js` around lines 23 - 44, The signup handler is accidentally allowing clients to override system-managed fields because userSignupSchema.parse(...) uses .passthrough() and otherData is spread into newUser; fix by sanitizing otherData before constructing newUser: explicitly remove/reserve keys like emailVerified, _id, createdAt, password (and any other server-managed fields) — either by destructuring them out of req.body (e.g., const { emailVerified, _id, createdAt, ...cleanData } = userSignupSchema.parse(req.body)) or by filtering keys from otherData, then use cleanData when building newUser; apply the same sanitization logic to the createAdminUser flow so request payloads cannot override reserved fields.backend/controllers/project.controller.js (1)
824-850:⚠️ Potential issue | 🔴 CriticalInvalidate the Redis project caches on delete.
This path deletes the project record but leaves the project-id and API-key cache entries intact. Until those keys expire, callers can still resolve a deleted project from cache—which is especially risky now that external DB/storage resources are intentionally preserved.
🧹 Suggested fix
await Project.deleteOne({ _id: projectId }); storageRegistry.delete(projectId.toString()); + await deleteProjectById(projectId.toString()); + await deleteProjectByApiKeyCache(project.publishableKey); + await deleteProjectByApiKeyCache(project.secretKey); res.json({ message: "Project and all associated resources deleted successfully" });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/project.controller.js` around lines 824 - 850, After deleting the Project document and removing storageRegistry (the block with Project.deleteOne({ _id: projectId }) and storageRegistry.delete(projectId.toString())), also invalidate any Redis caches that can resolve the deleted project (e.g., project and API-key entries). Call your Redis client to DEL the keys that cache the project by id and the api key (for example keys like `project:${projectId}` and `apikey:${project.apiKey}` or whatever key naming your codebase uses); ensure you obtain the project.apiKey before calling Project.deleteOne (or fetch the cached key mapping) so you can remove both entries atomically (or in a small multi/transaction) right after deletion.
♻️ Duplicate comments (3)
backend/controllers/userAuth.controller.js (2)
57-60:⚠️ Potential issue | 🟠 MajorIssue expiring JWTs for end-user auth tokens.
Both token issuances omit
expiresIn, so a leaked user token remains valid until the project's JWT secret rotates. Add an explicit TTL on bothjwt.sign()calls.🔐 Suggested fix
const token = jwt.sign( { userId: result.insertedId, projectId: project._id }, - project.jwtSecret + project.jwtSecret, + { expiresIn: '7d' } ); @@ const token = jwt.sign( { userId: user._id, projectId: project._id }, - project.jwtSecret + project.jwtSecret, + { expiresIn: '7d' } );Also applies to: 91-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/userAuth.controller.js` around lines 57 - 60, The jwt.sign calls that create end-user auth tokens (the one assigning token near jwt.sign({ userId: result.insertedId, projectId: project._id }, project.jwtSecret) and the second jwt.sign call around lines 91-94) must include an explicit expiresIn option to prevent indefinitely valid tokens; update both jwt.sign invocations to pass a third argument with a reasonable TTL (e.g., { expiresIn: 'Xh' } or a configurable value from project settings) while preserving payload and project.jwtSecret.
46-55:⚠️ Potential issue | 🟠 MajorRollback the inserted user if OTP setup fails.
insertOne()succeeds before Redis and queue writes. If either side effect throws, signup returns 500 but leaves an unverified account behind, and retries get stuck on"User already exists"without a fresh OTP.↩️ Suggested fix
const result = await collection.insertOne(newUser); - - await redis.set(`project:${project._id}:otp:verification:${email}`, otp, 'EX', 300); - - await authEmailQueue.add('send-verification-email', { - email, - otp, - type: 'verification', - pname: project.name - }); + try { + await redis.set(`project:${project._id}:otp:verification:${email}`, otp, 'EX', 300); + await authEmailQueue.add('send-verification-email', { + email, + otp, + type: 'verification', + pname: project.name + }); + } catch (sideEffectErr) { + await collection.deleteOne({ _id: result.insertedId }); + throw sideEffectErr; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/userAuth.controller.js` around lines 46 - 55, The signup flow inserts the user with collection.insertOne(newUser) before setting the OTP in Redis and enqueuing the verification email, so if redis.set or authEmailQueue.add fails the unverified user remains; wrap the post-insert side effects (redis.set and authEmailQueue.add) in a try/catch and on any failure perform a compensating delete using the inserted id (result.insertedId) to roll back the newly created user (call collection.deleteOne({ _id: result.insertedId })), await the deletion, log the failure, and rethrow or return the original error so the caller receives the 500 response.backend/utils/emailService.js (1)
141-144:⚠️ Potential issue | 🟠 MajorEscape backslashes before quoting the display name.
A project name containing
\still generates an invalid quoted header here, which can breakfrom/replyTodelivery for auth emails. The quoting branch needs to escape backslashes before quotes.🛡️ Suggested fix
- const safeDisplayName = rawPname.replace(/[\r\n]/g, '').trim(); + const safeDisplayName = rawPname.replace(/[\r\n]/g, '').trim() || 'urBackend'; const finalDisplayName = /^[a-zA-Z0-9 ]+$/.test(safeDisplayName) ? safeDisplayName - : `"${safeDisplayName.replace(/"/g, '\\"')}"`; + : `"${safeDisplayName.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils/emailService.js` around lines 141 - 144, The quoted-display-name branch for finalDisplayName fails to escape backslashes, so a rawPname containing "\" produces an invalid header; update the transformation used to build finalDisplayName (which currently calls safeDisplayName.replace(/"/g, '\\"')) to first escape backslashes then escape quotes (i.e., run a replace that converts "\" to "\\\\" before replacing quotes) so both backslashes and double-quotes are properly escaped when wrapping the name in quotes.
🧹 Nitpick comments (2)
frontend/src/pages/CreateCollection.jsx (2)
316-326: Strip thelockedproperty before sending to API.The
lockedflag is UI-only state. Currently it's included in the payload since only_idis destructured out.🛠️ Suggested fix
function cleanFieldsForApi(fields) { return fields.map(f => { - const { _id, ...clean } = f; + const { _id, locked, ...clean } = f; if (clean.fields) clean.fields = cleanFieldsForApi(clean.fields); if (clean.items?.fields) { clean.items = { ...clean.items, fields: cleanFieldsForApi(clean.items.fields) }; } return clean; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/CreateCollection.jsx` around lines 316 - 326, The cleanFieldsForApi function is currently only stripping _id, so the UI-only locked property is still sent to the API; update cleanFieldsForApi to also remove locked at every level (when mapping each field, destructure out locked along with _id or explicitly delete clean.locked) and ensure the same removal happens for nested clean.fields and clean.items.fields so no locked flags leak into the payload.
397-406: Normalize the collection name before validation.The validation uses exact string match
name === 'users'without trimming or case normalization. Whitespace or case variations would bypass the auth field checks. Even if the name input is later disabled for the users flow, normalizing provides defense-in-depth.🛠️ Suggested fix
+ const normalizedName = name.trim(); + const isUsersCollection = normalizedName.toLowerCase() === 'users'; + - if (!name) return toast.error("Collection name is required"); + if (!normalizedName) return toast.error("Collection name is required"); if (fields.some(f => !f.key)) return toast.error("All fields must have a name"); - if (name === 'users') { + if (isUsersCollection) { + if (normalizedName !== 'users') { + return toast.error("Auth collection must be named exactly 'users'."); + } const hasEmail = fields.find(f => f.key === 'email' && f.type === 'String' && f.required); const hasPassword = fields.find(f => f.key === 'password' && f.type === 'String' && f.required); if (!hasEmail || !hasPassword) { return toast.error("The 'users' collection MUST have 'email' and 'password' as required String fields."); } } @@ await axios.post(`${API_URL}/api/projects/collection`, { projectId, - collectionName: name, + collectionName: isUsersCollection ? 'users' : normalizedName, schema: cleanFieldsForApi(fields) }, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/CreateCollection.jsx` around lines 397 - 406, Normalize the collection name before the 'users' check by trimming and lowercasing it (e.g., compute a normalizedName = name.trim().toLowerCase()) and use that for the comparison instead of raw name; then keep the existing checks that compute hasEmail and hasPassword against fields (using keys 'email' and 'password') and return the same toast error if the normalizedName equals 'users' and required fields are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/userAuth.controller.js`:
- Around line 310-313: The jwt.verify call in endpoints (e.g., updateProfile(),
changePasswordUser(), and the other handlers around the tokenHeader checks and
jwt.verify(token, project.jwtSecret)) can throw for expired/malformed tokens and
must be wrapped in a nested try-catch like me() does; modify each endpoint to
first extract tokenHeader and token, then perform jwt.verify inside its own
try-catch and return res.status(401).json({ error: "Access Denied: Invalid or
expired token" }) on verification failure, allowing the outer error handler to
only handle unexpected errors.
- Around line 12-16: getAuthCollection currently returns a native Mongo
collection and allows auth endpoints to write directly, bypassing the project's
compiled model-based validation; update auth write paths (those using
getAuthCollection and any auth handlers around lines referenced) to use the
project's compiled Mongoose model (the same model used by
project.collections[].model or the compileModel helper) or to validate payloads
against that model before persisting: locate getAuthCollection and the auth
write handlers, obtain/compile the project's model, run model.validate()/new
Model(payload).validateSync() (or Model.validate) and return a 400 on validation
errors, and only then persist (either via Model.create/update or by applying the
validated document to the native collection) so custom types, nested objects,
arrays and required constraints are enforced.
In `@frontend/src/pages/Auth.jsx`:
- Around line 36-69: DynamicUserForm currently only handles Boolean/Number/Date
and falls back to a text input for Object, Array, and Ref fields; update the
rendering logic (in Auth.jsx inside the JSX branch that checks field.type and
uses onChange/formData/field.key) to handle these complex types: render Object
and Array as a resizable textarea showing JSON.stringify(formData[field.key] ??
{}) and onChange try to JSON.parse the value (use try/catch and preserve a local
parse error state) before calling onChange(field.key, parsedValue); render Ref
fields as either a dropdown/select when the field provides reference options
(e.g., field.refOptions or field.refCollection) falling back to a text input for
the referenced ID, and ensure required/placeholder behavior matches other
inputs; keep using onChange(field.key, value) and ensure the value passed is the
correct typed object/array or reference id.
---
Outside diff comments:
In `@backend/controllers/project.controller.js`:
- Around line 824-850: After deleting the Project document and removing
storageRegistry (the block with Project.deleteOne({ _id: projectId }) and
storageRegistry.delete(projectId.toString())), also invalidate any Redis caches
that can resolve the deleted project (e.g., project and API-key entries). Call
your Redis client to DEL the keys that cache the project by id and the api key
(for example keys like `project:${projectId}` and `apikey:${project.apiKey}` or
whatever key naming your codebase uses); ensure you obtain the project.apiKey
before calling Project.deleteOne (or fetch the cached key mapping) so you can
remove both entries atomically (or in a small multi/transaction) right after
deletion.
In `@backend/controllers/userAuth.controller.js`:
- Around line 23-44: The signup handler is accidentally allowing clients to
override system-managed fields because userSignupSchema.parse(...) uses
.passthrough() and otherData is spread into newUser; fix by sanitizing otherData
before constructing newUser: explicitly remove/reserve keys like emailVerified,
_id, createdAt, password (and any other server-managed fields) — either by
destructuring them out of req.body (e.g., const { emailVerified, _id, createdAt,
...cleanData } = userSignupSchema.parse(req.body)) or by filtering keys from
otherData, then use cleanData when building newUser; apply the same sanitization
logic to the createAdminUser flow so request payloads cannot override reserved
fields.
In `@frontend/src/pages/CreateCollection.jsx`:
- Around line 362-372: The code is using useState with a side-effecting
initializer to fetch collections; replace that with useEffect so the fetchRuns
as a proper effect: import useEffect alongside useState at the top, change the
block that defines and calls fetchCollections to run inside useEffect
(useEffect(() => { const fetchCollections = async () => { ... };
fetchCollections(); }, [projectId, token]) ), keep using setCollections and
axios, and handle the catch (don’t leave it empty) — locate the fetchCollections
function and setCollections usage in CreateCollection.jsx and convert the
useState initializer to a useEffect hook.
In `@frontend/src/pages/Database.jsx`:
- Around line 100-112: If a truthy queryCollection string isn't found in
res.data.collections, clear or replace the stale activeCollection instead of
leaving it unchanged: inside the load/response handler where you read
queryCollection and call setActiveCollection, check whether found =
res.data.collections.find(...) is truthy; if not, compute the fallback (use
filtered = res.data.collections.filter(c => c.name !== 'users'), then choose
filtered[0] or res.data.collections[0] if available) and call
setActiveCollection with that fallback, or call setActiveCollection(null) to
explicitly clear selection when there are no valid collections; update the logic
around queryCollection, found, filtered, and setActiveCollection to always set a
value (or null) so activeCollection is never left stale.
---
Duplicate comments:
In `@backend/controllers/userAuth.controller.js`:
- Around line 57-60: The jwt.sign calls that create end-user auth tokens (the
one assigning token near jwt.sign({ userId: result.insertedId, projectId:
project._id }, project.jwtSecret) and the second jwt.sign call around lines
91-94) must include an explicit expiresIn option to prevent indefinitely valid
tokens; update both jwt.sign invocations to pass a third argument with a
reasonable TTL (e.g., { expiresIn: 'Xh' } or a configurable value from project
settings) while preserving payload and project.jwtSecret.
- Around line 46-55: The signup flow inserts the user with
collection.insertOne(newUser) before setting the OTP in Redis and enqueuing the
verification email, so if redis.set or authEmailQueue.add fails the unverified
user remains; wrap the post-insert side effects (redis.set and
authEmailQueue.add) in a try/catch and on any failure perform a compensating
delete using the inserted id (result.insertedId) to roll back the newly created
user (call collection.deleteOne({ _id: result.insertedId })), await the
deletion, log the failure, and rethrow or return the original error so the
caller receives the 500 response.
In `@backend/utils/emailService.js`:
- Around line 141-144: The quoted-display-name branch for finalDisplayName fails
to escape backslashes, so a rawPname containing "\" produces an invalid header;
update the transformation used to build finalDisplayName (which currently calls
safeDisplayName.replace(/"/g, '\\"')) to first escape backslashes then escape
quotes (i.e., run a replace that converts "\" to "\\\\" before replacing quotes)
so both backslashes and double-quotes are properly escaped when wrapping the
name in quotes.
---
Nitpick comments:
In `@frontend/src/pages/CreateCollection.jsx`:
- Around line 316-326: The cleanFieldsForApi function is currently only
stripping _id, so the UI-only locked property is still sent to the API; update
cleanFieldsForApi to also remove locked at every level (when mapping each field,
destructure out locked along with _id or explicitly delete clean.locked) and
ensure the same removal happens for nested clean.fields and clean.items.fields
so no locked flags leak into the payload.
- Around line 397-406: Normalize the collection name before the 'users' check by
trimming and lowercasing it (e.g., compute a normalizedName =
name.trim().toLowerCase()) and use that for the comparison instead of raw name;
then keep the existing checks that compute hasEmail and hasPassword against
fields (using keys 'email' and 'password') and return the same toast error if
the normalizedName equals 'users' and required fields are missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54c8d4a5-88d4-488c-ba06-bdbf10587065
📒 Files selected for processing (10)
backend/controllers/project.controller.jsbackend/controllers/userAuth.controller.jsbackend/middleware/checkAuthEnabled.jsbackend/middleware/loadProjectForAdmin.jsbackend/routes/projects.jsbackend/utils/emailService.jsbackend/utils/input.validation.jsfrontend/src/pages/Auth.jsxfrontend/src/pages/CreateCollection.jsxfrontend/src/pages/Database.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/middleware/checkAuthEnabled.js
- backend/middleware/loadProjectForAdmin.js
…WT handling, Frontend UI)
… in userAuth.controller.js
yash-pouranik
temporarily deployed
to
feat/auth-system-dx-upgrade - urBackend-frankfrut PR #36
Render
Destroyed
This was referenced Mar 27, 2026
16 tasks
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
2 participants
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.
Description: Major Auth System & Developer Experience (DX) Upgrade
📋 Summary
This Pull Request introduces a comprehensive upgrade to the urBackend Authentication system and internal resource management. The goal of this update was to transition the Auth service from a rigid structure to a fully dynamic, schema-driven experience while significantly hardening data security and resource safety.
🚀 Key Changes
1. 🛡️ Global Authentication Infrastructure
isAuthEnabledlogic. Toggling Auth now intelligently manages the project state and triggers a "Guided Setup" workflow if the required collections are missing.checkAuthEnabledmiddleware to strictly enforce authentication rules at the API level.2. 📋 Advanced User Management (Dashboard)
userscollection schema. It generates native input fields (Email, Number, Date, Boolean, etc.) for all custom fields defined by the developer, replacing the legacy JSON editor.bcrypthashing flow.3. 🛡️ Data Privacy & Hardening
passwordfield is now strictly stripped from project schema metadata returned to the frontend. It is completely invisible in general database views.userscollection is now hidden from the general Database Sidebar and Project Overview to centralize security management on the Auth page.passwordfield operations for theuserscollection to prevent accidental exposure or unhashed overwrites.4. 🧨 Resource Safety & Cleanup
🏗️ DX & Maintenance
/api/projects/:projectId/admin/...pathing.TypeErrorin emailService.js and corrected Zod validation export mismatches.🧪 Verification Plan
userscollection exists.userscollection and verify it appears as a form input in the "Add User" modal.passwordfield in the general Database tab (should be hidden/blocked).This PR brings the Auth service to a release-ready state with a focus on ease of use and maximum security. 🚀🦾
Built with ❤️ for urBackend.
Summary by CodeRabbit
New Features
Security Enhancements
UI Improvements