Conversation
Contributor
WalkthroughAdds an email verification flow: client verify page and styles, client autocomplete tweaks, backend token lookup and verify handlers/queries/types, auth route base path changed to /api/auth, and test infra updated for serial DB-safe test execution. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Client as Client App
participant Server as Backend API
participant DB as Database
User->>Client: Submit registration
Client->>Server: POST /api/auth/register
Server->>DB: Insert user + create token
DB-->>Server: Insert result
Server-->>Client: Success (guidance to verify)
User->>Client: Open /verify?token=xxx
Client->>Client: read token from URL
Client->>Server: GET /api/auth/verify?token=xxx
Server->>DB: find_token_by_value(token)
DB-->>Server: token row (user_id, expires_at)
Server->>DB: verify_user(user_id) (UPDATE users set verified=true)
DB-->>Server: Update result
Server-->>Client: Verification success
Client->>User: Show verified message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 (1)
rust-server/tests/api/common.rs (1)
128-158: 🧹 Nitpick | 🔵 TrivialGood approach for testing DB failure scenarios.
Setting
db: Nonelets you verify your app handles database outages gracefully — that's defensive programming at its finest.One subtle observation for your learning: this function sets
TEST_DB_URLand returns aTestServerHandle, meaning when dropped, it will attempt DB truncation even though this test never connected to the DB. That's harmless (the cleanup silently ignores failures), but slightly wasteful.If you wanted to be meticulous, you could introduce a variant that skips DB cleanup entirely for "no-DB" test scenarios. But honestly, the current approach is pragmatic — one cleanup path is simpler to maintain than conditional paths. Sometimes "good enough" is the professional choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-server/tests/api/common.rs` around lines 128 - 158, The test currently calls TEST_DB_URL.set(...) and returns a TestServerHandle which triggers DB cleanup on drop even though AppState.db is None; to avoid the unnecessary truncation, either (A) stop setting TEST_DB_URL in build_test_server_without_db (remove the TEST_DB_URL.set call and test_db_url local), or (B) add a new handle/constructor (e.g., TestServerHandle::new_no_db or TestServer::new_no_cleanup) that skips registering the DB truncation cleanup and return that from build_test_server_without_db; locate symbols build_test_server_without_db, TEST_DB_URL, AppState { db: None }, TestServerHandle::new/TestServer::new to implement the chosen change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/app/`(auth)/verify/page.tsx:
- Line 10: Remove the client-side logging of the verification token by deleting
the console.log("token:", token) statement in verify/page.tsx; ensure no other
calls log the token variable in that file (or any client code) so the sensitive
token is not output to the browser console.
- Around line 8-18: The page currently only logs the token and shows a static
success message; replace the TODO and the static UI in the verify page to
actually validate the token by calling your backend verification endpoint (use
the token variable), handle success and error responses, and render distinct UI
states (loading, verified success, invalid/expired error) accordingly; update
the component (the page default export / verify page code that uses token) to
POST/GET the token to the server, process the JSON response, and conditionally
render messages and any follow-up actions (redirect, signin link) based on the
server result.
In `@client/src/lib/api/auth.ts`:
- Around line 90-95: The verifyAccount helper is missing in
client/src/lib/api/auth.ts so the frontend verify page cannot complete
verification; implement an exported async function verifyAccount(token: string)
that calls the backend GET endpoint (e.g., "/auth/verify" or the project API
base + "/auth/verify") passing the token (query param or header to match
backend), checks response.ok, parses JSON or handles non-2xx by throwing an
Error with server message, and returns the parsed result or boolean success;
ensure signatures and exports match existing helpers in auth.ts (same error
handling and types) so client/src/app/(auth)/verify/page.tsx can import and
await verifyAccount(token).
In `@rust-server/.cargo/config.toml`:
- Around line 1-7: The [test] section and test-threads setting are ignored by
Cargo; remove the invalid [test] table and instead set RUST_TEST_THREADS = "1"
inside the existing [env] table so the Rust test harness runs single-threaded;
specifically, delete the [test] block (and test-threads) and add the environment
variable RUST_TEST_THREADS = "1" under the [env] section in this config.
In `@rust-server/src/db/queries.rs`:
- Around line 31-33: find_token_by_value currently selects user_id by token
only, allowing expired tokens to pass; update the SQL in find_token_by_value to
include an expiry check (e.g., "SELECT user_id FROM tokens WHERE token = $1 AND
expires_at > now()") and bind token as before (no extra params needed if using
now()), or if you prefer explicit time use chrono::Utc::now() and bind it as a
second parameter; ensure the function still returns Ok(None) when no row is
found (expired or missing) and that the query maps the returned user_id to Uuid
as before.
In `@rust-server/src/features/auth/handlers.rs`:
- Around line 126-130: The token lookup currently accepts any matching value;
change the find_token_by_value query to also require expires_at > now() (or
equivalent DB timestamp check) so expired tokens are rejected, and return None
if not found so the existing ok_or_else(AppError::not_found(...)) still works;
after calling verify_user(db, user_id) consume the token in the database (either
delete the row or set a used/consumed flag) to enforce one-time use—update the
post-verification logic in the handler to call the token-deletion/mark-used
function (or add that logic to the same transaction that verified the user) so
find_token_by_value, expires_at, and verify_user are referenced and the token
cannot be replayed.
- Around line 88-106: The availability check currently treats Some("") or
whitespace as valid — update the logic to trim and reject empty/whitespace-only
inputs before hitting the DB: after the initial match on
params.email/params.username, normalize ¶ms.email and ¶ms.username by
trimming and treat trimmed empty strings as None (or return
Err(AppError::bad_request("Invalid query"))), then only call
find_user_by_email(db, email) and find_user_by_username(db, username) when the
trimmed value is non-empty; reference params.email, params.username,
find_user_by_email, and find_user_by_username to locate where to add the
trim-and-empty check and return appropriate AppError::bad_request for blank
inputs.
In `@rust-server/src/features/auth/routes.rs`:
- Around line 13-15: Routes in auth module currently hardcode the "/auth"
prefix, causing tight coupling when mounted; change the route definitions in
auth::routes (the .route calls that reference register_user, check_availability,
verify_account) to use relative paths ("/register", "/available", "/verify") and
then mount the module with a namespace where it belongs (e.g. use auth_router()
from auth::routes and nest it at "/api/auth" in the features/module that
composes routers) so the auth_router remains reusable and free of its own
namespace.
In `@rust-server/src/features/auth/types.rs`:
- Around line 35-38: The public struct name AvailibilityQuery contains a
spelling typo; rename the type to AvailabilityQuery throughout the codebase (the
struct definition and all references/imports/usages) to avoid breaking changes
and maintain consistency: update the struct declaration for AvailibilityQuery,
any pattern matches, constructor calls, type annotations, function signatures,
and module exports that reference AvailibilityQuery to use AvailabilityQuery
instead.
---
Outside diff comments:
In `@rust-server/tests/api/common.rs`:
- Around line 128-158: The test currently calls TEST_DB_URL.set(...) and returns
a TestServerHandle which triggers DB cleanup on drop even though AppState.db is
None; to avoid the unnecessary truncation, either (A) stop setting TEST_DB_URL
in build_test_server_without_db (remove the TEST_DB_URL.set call and test_db_url
local), or (B) add a new handle/constructor (e.g., TestServerHandle::new_no_db
or TestServer::new_no_cleanup) that skips registering the DB truncation cleanup
and return that from build_test_server_without_db; locate symbols
build_test_server_without_db, TEST_DB_URL, AppState { db: None },
TestServerHandle::new/TestServer::new to implement the chosen change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01f0607b-3c9d-452b-bd4f-58f8b7051721
📒 Files selected for processing (12)
client/src/app/(auth)/layout.cssclient/src/app/(auth)/register/_components/RegisterCredentials.tsxclient/src/app/(auth)/register/_components/RegisterEmail.tsxclient/src/app/(auth)/verify/page.tsxclient/src/lib/api/auth.tsrust-server/.cargo/config.tomlrust-server/src/db/queries.rsrust-server/src/features/auth/handlers.rsrust-server/src/features/auth/queries.rsrust-server/src/features/auth/routes.rsrust-server/src/features/auth/types.rsrust-server/tests/api/common.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
rust-server/.cargo/config.toml (1)
1-3:⚠️ Potential issue | 🟠 MajorMove
RUST_TEST_THREADSunder[env]; current placement may be ignored.
RUST_TEST_THREADSshould be nested in an[env]table in Cargo config. As written on Line 3 at top-level, Cargo may not apply it, so tests can still execute in parallel.🔧 Proposed fix
+[env] # Force single-threaded test execution to avoid database race conditions. # Each test truncates the database, so concurrent execution causes conflicts. RUST_TEST_THREADS = "1"In Cargo `.cargo/config.toml`, are environment variables like `RUST_TEST_THREADS` only valid under the `[env]` table, and is a top-level `RUST_TEST_THREADS = "1"` ignored?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-server/.cargo/config.toml` around lines 1 - 3, The RUST_TEST_THREADS setting is currently at top-level and may be ignored; move the RUST_TEST_THREADS = "1" entry into an [env] table so Cargo picks it up (i.e., add an [env] section and place RUST_TEST_THREADS = "1" under it), ensuring the environment variable is applied for test runs; verify the change by running cargo test to confirm single-threaded execution.client/src/app/(auth)/verify/page.tsx (1)
4-21:⚠️ Potential issue | 🟠 MajorHey, padawan! This verification page needs to actually... verify something.
Right now, you're reading the
tokenbut not doing anything with it. The backend endpoint atGET /api/auth/verify?token=...is ready and waiting! Here's the professional approach:
- Call your backend when the page loads (via
useEffect)- Handle all three states: loading, success, and error
- Fix the messaging — this page is where users land after clicking the email link, so "check your inbox" doesn't make sense here
This is a core UX principle: never show success until you've confirmed it. Users clicking an expired or invalid link will see "success" messaging, which erodes trust.
🛠️ Here's the pattern you want to learn
"use client"; +import { useEffect, useState } from "react"; import { useSearchParams } from "next/navigation"; export default function VerifyPage() { const searchParams = useSearchParams(); const token = searchParams.get("token"); + const [status, setStatus] = useState<"loading" | "success" | "error">("loading"); + const [message, setMessage] = useState("Verifying your account..."); - // TODO: Implement actual verification logic by sending the token to the server for validation + useEffect(() => { + if (!token) { + setStatus("error"); + setMessage("Missing verification token."); + return; + } + + fetch(`${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/verify?token=${encodeURIComponent(token)}`) + .then((res) => res.json()) + .then((data) => { + if (data.success) { + setStatus("success"); + setMessage(data.message ?? "Your account is now verified!"); + } else { + setStatus("error"); + setMessage(data.error ?? "Verification failed."); + } + }) + .catch(() => { + setStatus("error"); + setMessage("Network error. Please try again."); + }); + }, [token]); return ( <div className="auth-verify"> <div className="container"> <h1>Verify your email</h1> - <p> - A verification link has been sent to your email. Please check your - inbox and click the link to verify your account. - </p> + <p>{message}</p> + {status === "success" && <a href="/login">Sign in</a>} </div> </div> ); }Pro tip: Extract this into a custom hook or API helper (like
verifyAccount(token)) to keep your component clean and testable. You've already got that pattern going inclient/src/lib/api/auth.ts!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/`(auth)/verify/page.tsx around lines 4 - 21, The VerifyPage currently reads token via useSearchParams but never calls the backend; update VerifyPage to call the verification endpoint (or existing verifyAccount(token) helper from client/src/lib/api/auth.ts) inside a useEffect when a token is present, manage loading/success/error states (e.g., local state variables like isLoading, isSuccess, error), and render appropriate messages for each state (loading indicator, success confirmation when API returns success, and a clear error message with possible actions on failure); ensure the GET /api/auth/verify?token=... request is awaited and errors are caught and stored in error state.rust-server/src/features/auth/handlers.rs (1)
127-151:⚠️ Potential issue | 🔴 CriticalSecurity gap: Token replay attack — tokens must be single-use.
You're checking expiry (great!), but after
verify_usersucceeds, the token remains valid in the database. This means:
- Replay attacks: Anyone who intercepts the verification link can use it repeatedly
- Audit trail confusion: You can't tell when verification actually happened
- Resource exhaustion: Attackers could hammer verify with valid tokens
The fix: Delete or mark the token as consumed after successful verification. This is a fundamental principle — verification tokens are bearer credentials and must be one-time use.
🔐 Here's how to fix it
First, add a delete function in your queries:
// In rust-server/src/db/queries.rs or auth/queries.rs pub async fn delete_token(db: &PgPool, token: &str) -> Result<(), AppError> { sqlx::query("DELETE FROM tokens WHERE token = $1") .bind(token) .execute(db) .await .map_err(AppError::Sql)?; Ok(()) }Then use it in your handler:
verify_user(db, user_id).await?; + delete_token(db, &token).await?; Ok(ApiResponse::ok("Email verified successfully", None)) }Alternatively, use a transaction to make
verify_user+delete_tokenatomic — that's the production-grade approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-server/src/features/auth/handlers.rs` around lines 127 - 151, verify_account currently leaves the verification token valid after calling verify_user, enabling token replay; fix by removing or marking the token consumed after successful verification (e.g., call a new delete_token(db, &token) or mark_token_consumed(db, &token) once verify_user(db, user_id) returns Ok) and handle/delete errors appropriately; for correctness and safety make the verify_user + delete_token operation atomic by running them inside a database transaction (use your DB transaction API), and ensure you reference find_token_by_value, verify_user, and the new delete_token/mark_token_consumed functions when implementing the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-server/src/db/queries.rs`:
- Around line 22-29: The #[allow(dead_code)] attribute on the find_user_by_id
function masks unused code rot; remove the attribute and either keep the
function in active use or delete it and rely on Git to restore it later—locate
the pub async fn find_user_by_id(...) definition and delete the
#[allow(dead_code)] line (or remove the whole function if it's not needed yet)
so unused scaffolding doesn't linger in production code.
- Around line 31-39: In find_token_by_value, avoid "SELECT *" — change the SQL
to explicitly list the token table columns you need (e.g., id, token, user_id,
expires_at, created_at) in the sqlx::query string and keep using
.bind(token).fetch_optional(db).await; also update any downstream reads that
access columns from the returned PgRow to reference those explicit column names
(or better: map the row into a typed struct) so schema changes won’t break
parsing and you only fetch required fields.
In `@rust-server/src/features/auth/handlers.rs`:
- Line 148: verify_user currently executes an UPDATE but ignores the execution
result so a non-existent user (rows_affected == 0) will appear as a successful
verification; change verify_user in auth/queries.rs to capture the
sqlx::query(...).execute(db).await result, check result.rows_affected(), and
return an AppError::not_found (or appropriate error) when rows_affected() == 0,
ensuring the existing caller in handlers.rs (which calls verify_user(db,
user_id).await?) will propagate the error instead of reporting "Email verified
successfully".
---
Duplicate comments:
In `@client/src/app/`(auth)/verify/page.tsx:
- Around line 4-21: The VerifyPage currently reads token via useSearchParams but
never calls the backend; update VerifyPage to call the verification endpoint (or
existing verifyAccount(token) helper from client/src/lib/api/auth.ts) inside a
useEffect when a token is present, manage loading/success/error states (e.g.,
local state variables like isLoading, isSuccess, error), and render appropriate
messages for each state (loading indicator, success confirmation when API
returns success, and a clear error message with possible actions on failure);
ensure the GET /api/auth/verify?token=... request is awaited and errors are
caught and stored in error state.
In `@rust-server/.cargo/config.toml`:
- Around line 1-3: The RUST_TEST_THREADS setting is currently at top-level and
may be ignored; move the RUST_TEST_THREADS = "1" entry into an [env] table so
Cargo picks it up (i.e., add an [env] section and place RUST_TEST_THREADS = "1"
under it), ensuring the environment variable is applied for test runs; verify
the change by running cargo test to confirm single-threaded execution.
In `@rust-server/src/features/auth/handlers.rs`:
- Around line 127-151: verify_account currently leaves the verification token
valid after calling verify_user, enabling token replay; fix by removing or
marking the token consumed after successful verification (e.g., call a new
delete_token(db, &token) or mark_token_consumed(db, &token) once verify_user(db,
user_id) returns Ok) and handle/delete errors appropriately; for correctness and
safety make the verify_user + delete_token operation atomic by running them
inside a database transaction (use your DB transaction API), and ensure you
reference find_token_by_value, verify_user, and the new
delete_token/mark_token_consumed functions when implementing the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9451327-f7f3-466f-903f-536f470625b6
📒 Files selected for processing (8)
client/src/app/(auth)/verify/page.tsxclient/src/lib/api/auth.spec.tsrust-server/.cargo/config.tomlrust-server/src/db/queries.rsrust-server/src/features/auth/handlers.rsrust-server/src/features/auth/routes.rsrust-server/src/features/auth/types.rsrust-server/src/features/mod.rs
| return Err(AppError::bad_request("Token has expired")); | ||
| } | ||
|
|
||
| verify_user(db, user_id).await?; |
Contributor
There was a problem hiding this comment.
Silent success on non-existent users.
Looking at verify_user in auth/queries.rs, it runs an UPDATE without checking rows_affected. If user_id doesn't exist (e.g., user deleted between token creation and verification), the query silently succeeds and you return "Email verified successfully" for a ghost user.
Consider checking the result:
pub async fn verify_user<'e, E>(db: E, user_id: Uuid) -> Result<(), AppError>
where
E: Executor<'e, Database = Postgres>,
{
let result = sqlx::query("UPDATE users SET verified = true WHERE id = $1")
.bind(user_id)
.execute(db)
.await
.map_err(AppError::Sql)?;
if result.rows_affected() == 0 {
return Err(AppError::not_found("User not found"));
}
Ok(())
}This is edge-case defensive coding, but it prevents confusing scenarios in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-server/src/features/auth/handlers.rs` at line 148, verify_user currently
executes an UPDATE but ignores the execution result so a non-existent user
(rows_affected == 0) will appear as a successful verification; change
verify_user in auth/queries.rs to capture the sqlx::query(...).execute(db).await
result, check result.rows_affected(), and return an AppError::not_found (or
appropriate error) when rows_affected() == 0, ensuring the existing caller in
handlers.rs (which calls verify_user(db, user_id).await?) will propagate the
error instead of reporting "Email verified successfully".
This was referenced Apr 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests