Implement email authentication with login and registration#120
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds first-party email/password authentication to the Hono app (in addition to existing Google OAuth), introducing new login/registration routes and pages, a users table + migration, and updating unauthenticated redirect behavior to point to the new login page.
Changes:
- Add
/auth/loginand/auth/registerHTML routes, plus password hashing/verification utilities. - Introduce
userstable in Drizzle schema with a migration and snapshot metadata. - Update the root route (and its tests) to redirect unauthenticated users to
/auth/login, and add an agent guide (AGENTS.md).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| hono/routes/index.tsx | Redirect unauthenticated users to the new login route. |
| hono/routes/index.test.tsx | Update redirect test to expect /auth/login. |
| hono/routes/emailAuth.ts | New email login/registration handlers using DB + password hashing. |
| hono/routes/emailAuth.test.ts | Initial unit tests for email auth routes. |
| hono/db/schema.ts | Add users table definition. |
| hono/db/migrations/meta/_journal.json | Register new migration in Drizzle journal. |
| hono/db/migrations/meta/0001_snapshot.json | Add schema snapshot including users table. |
| hono/db/migrations/0001_nosy_rawhide_kid.sql | SQL migration creating users table. |
| hono/components/RegisterPage.tsx | New registration page UI. |
| hono/components/LoginPage.tsx | New login page UI. |
| hono/components/AuthPage.css | Styling for login/register pages. |
| hono/auth/password.ts | Implement scrypt-based password hashing + verification. |
| hono/auth/password.test.ts | Unit tests for hashing/verification behavior. |
| hono/app.tsx | Register the new emailAuth routes. |
| AGENTS.md | Add repo-specific contributor/agent guide and conventions. |
Comments suppressed due to low confidence (2)
hono/auth/password.ts:25
- In
verifyPassword, the scrypt callback has the same issue ashashPassword: it callsreject(err)but then continues toresolve(derivedKey). Add an early return after rejecting (or switch to a promisified API) so failures don't fall through.
const derivedKey = await new Promise<Buffer>((resolve, reject) => {
scrypt(password, salt, KEY_LENGTH, (err, derivedKey) => {
if (err) reject(err);
resolve(derivedKey);
});
hono/routes/emailAuth.ts:90
- Email inputs are inserted as-is. Consider normalizing
email(trim + lowercase) before the uniqueness check and insert; otherwise the DB unique constraint will allow case-variant duplicates (e.g.,User@x.comvsuser@x.com).
const body = await c.req.parseBody();
const name = body.name as string;
const email = body.email as string;
const password = body.password as string;
if (!name || !email || !password) {
return c.html(`<!DOCTYPE html>${await RegisterPage("All fields are required.")}`, 400);
}
if (password.length < 8) {
return c.html(`<!DOCTYPE html>${await RegisterPage("Password must be at least 8 characters.")}`, 400);
}
// Check if user already exists
const [existing] = await db.select().from(users).where(eq(users.email, email)).limit(1);
| emailAuth.post("/auth/login", async (c) => { | ||
| try { | ||
| const body = await c.req.parseBody(); | ||
| const email = body.email as string; | ||
| const password = body.password as string; |
There was a problem hiding this comment.
Tests for these routes currently cover validation/error paths, but don’t cover the primary success flows (successful login sets session + redirects; successful registration inserts user + sets session; existing-email conflict path; password verification true path). Add unit tests for these behaviors so the new auth feature is adequately covered.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| app.route("/", health); | ||
| app.route("/", auth); | ||
| app.route("/", emailAuth); | ||
| app.route("/", testAuth); | ||
| app.route("/", messagesRoute); |
There was a problem hiding this comment.
This PR introduces form-based login/registration POST endpoints that mutate session state. With the current session cookie configuration (SameSite is set to none in production), these routes are vulnerable to CSRF/login-CSRF. Add CSRF protection (token in the form + server-side validation) or adjust cookie SameSite strategy so cross-site POSTs don’t include the session cookie.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| const body = await c.req.parseBody(); | ||
| const email = body.email as string; | ||
| const password = body.password as string; | ||
|
|
||
| if (!email || !password) { | ||
| return c.html(`<!DOCTYPE html>${await LoginPage("Email and password are required.")}`, 400); | ||
| } |
There was a problem hiding this comment.
Email inputs are used as-is. Without trimming/canonicalization (e.g., email.trim().toLowerCase()), users can register duplicate accounts differing only by case/whitespace and later fail to log in if they vary casing. Normalize email before querying/inserting.
This issue also appears on line 76 of the same file.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| const passwordHash = await hashPassword(password); | ||
| await db.insert(users).values({ | ||
| email, | ||
| name, | ||
| passwordHash, |
There was a problem hiding this comment.
Registration does a read-then-insert to enforce unique email. Under concurrent requests, the insert can still hit the DB unique constraint and currently falls into the generic 500 handler. Consider catching the Postgres unique-violation error for users_email_unique (e.g., SQLSTATE 23505) and returning the same 409 “email already exists” response to make this path reliable.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| const hash = await new Promise<string>((resolve, reject) => { | ||
| scrypt(password, salt, KEY_LENGTH, (err, derivedKey) => { | ||
| if (err) reject(err); | ||
| resolve(derivedKey.toString("hex")); | ||
| }); |
There was a problem hiding this comment.
In hashPassword, the scrypt callback calls reject(err) but then continues to resolve(...). If err is non-null, derivedKey can be undefined and this can throw, or the Promise can attempt to settle twice. Return immediately after rejecting (or use promisify(scrypt)) so error cases are handled correctly.
This issue also appears on line 21 of the same file.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Show login page | ||
| emailAuth.get("/auth/login", async (c) => { | ||
| const error = c.req.query("error"); |
There was a problem hiding this comment.
The repository’s agent guide explicitly asks to avoid code comments in favor of self-documenting names. This file introduces several section comments (e.g., "Show login page"). Please remove these comments to match the established convention.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…121) * Initial plan * test(emailAuth): add unit tests for success flows and conflict paths Co-authored-by: mahata <23497+mahata@users.noreply.github.com> Agent-Logs-Url: https://github.com/mahata/mlack/sessions/e8292715-c961-480d-8c1a-490e2db65f9a --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mahata <23497+mahata@users.noreply.github.com>
* Initial plan * fix: add CSRF protection for email auth form endpoints Co-authored-by: mahata <23497+mahata@users.noreply.github.com> Agent-Logs-Url: https://github.com/mahata/mlack/sessions/81a07abc-5f9a-4910-8bdc-71951fdb71bb --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mahata <23497+mahata@users.noreply.github.com>
…re (#124) * Initial plan * fix: add Origin header to POST tests blocked by CSRF middleware Co-authored-by: mahata <23497+mahata@users.noreply.github.com> Agent-Logs-Url: https://github.com/mahata/mlack/sessions/86a80766-f33f-4962-ba8e-c5907a70e520 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mahata <23497+mahata@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
#126) * Initial plan * fix: add Origin header to E2E test requests blocked by CSRF middleware Co-authored-by: mahata <23497+mahata@users.noreply.github.com> Agent-Logs-Url: https://github.com/mahata/mlack/sessions/edbeed38-429c-4f9f-beb6-cf8d58dc7ec4 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mahata <23497+mahata@users.noreply.github.com>
Add email authentication functionality, including login and registration routes, and update the user interface accordingly. This change introduces a new user table in the database and implements password hashing for secure authentication. Additionally, it modifies the redirection behavior for unauthenticated users.