Skip to content

feat(auth): sign in with email and password, not just Google - #145

Open
jlowapik wants to merge 1 commit into
mainfrom
feat/email-password-login
Open

feat(auth): sign in with email and password, not just Google#145
jlowapik wants to merge 1 commit into
mainfrom
feat/email-password-login

Conversation

@jlowapik

Copy link
Copy Markdown
Contributor

What

Adds email+password accounts alongside Google OAuth. /login now serves a page offering both methods instead of 302'ing straight to /auth/google, backed by POST /api/auth/register and POST /api/auth/login.

The login form was the small half

Sessions were keyed by googleId, so ~13 dashboard routes resolved the caller with an inline getUserByGoogleId(req.session.googleId) — a guaranteed miss for an account whose google_id is NULL. Every one now goes through a single resolveSessionUser (userId first, googleId fallback so sessions already in Redis survive their 7-day TTL rather than logging everyone out on deploy).

The /connect/:mcpSlug OAuth state carries userId for the same reason: without it a password user consents at the provider and gets "User not found" on the way back.

The DB already had password_hash + auth_method columns, google_id was already nullable, UserProfile.authMethod was already typed 'google' | 'password', and bcrypt was already a dependency — all unused. This wires that scaffolding up. No migration needed.

Deliberate calls worth reviewing

  • Passwords over 72 bytes are rejected, not truncated. bcrypt silently discards past that, so accepting them would claim a passphrase protects the account when only its first 72 bytes do.
  • Login runs the bcrypt compare unconditionally. Unknown emails and Google-only accounts compare against a dummy hash of equal cost, so response time is not an account-existence oracle (measured 192ms on both paths). Registration's 409 discloses existence on purpose — a generic error there leaves the user unable to tell "taken" from "broken".
  • regenerateApiKey(googleId) is deleted rather than kept. Its WHERE google_id = $1 matched zero rows for these accounts and reported "user not found" on a valid one. Left in place it's a trap for the next caller; regenerateApiKeyByUserId is the only version.
  • Linking Google to an existing password account keeps the password and keeps auth_method='password' (both the DB ON CONFLICT (email) path and its file-store sibling, which previously didn't link by email at all and would have created a second account for the same address). The dashboard renders authMethod verbatim, so relabelling would announce the loss of a credential that still works.
  • password_hash stays out of DB_USER_COLUMNS and UserProfile, so nothing that serializes a user (/api/me, /api/admin/users, the file-store JSON dump) can leak a hash it never selected.

Rate limiting

8 attempts / 15 min per IP and per email, Redis-backed with an in-memory fallback. It counts every attempt, not just failures — counting failures alone lets one valid login reset the window. The email bucket is the load-bearing half, since the pre-existing trust proxy: true makes req.ip spoofable via X-Forwarded-For.

Known gap

There is no password reset. This repo has no mail infrastructure (no SMTP/Resend/SendGrid anywhere), so a forgotten password has no self-service path until a provider is added. Flagging rather than half-building it.

Testing

  • Full suite 2338/2339. The one failure is calendarEventSchemas.test.ts, a pre-existing untracked file that fails standalone and is not part of this PR.
  • 24 new tests in emailPasswordAuth.test.ts covering registration, login, the timing-parity path, case-insensitive email, rate limiting, Google-account linking, and — importantly — that /api/me, /api/regenerate-key, and /api/me/instances all work with a password session (each would 401 or 404 under the old googleId lookup).
  • Three tests in routes.test.ts updated: they pinned /login and /connect's park-and-login to /auth/google, which is exactly the behaviour this changes.
  • Driven end-to-end against a live server with curl: register → cookie → /api/me → key rotation → logout → re-login, plus wrong-password vs unknown-email parity and the limiter tripping with Retry-After: 887.

⚠️ Not visually reviewed in a browser — the Chrome extension wasn't connected. public/login.html is worth a human eyeball.

🤖 Generated with Claude Code

Adds email+password accounts alongside Google OAuth. /login now serves a
page offering both methods instead of 302'ing straight to /auth/google,
backed by POST /api/auth/register and POST /api/auth/login.

The login form was the small half. Sessions were keyed by googleId, so
~13 dashboard routes resolved the caller with an inline
getUserByGoogleId(req.session.googleId) — a guaranteed miss for an account
whose google_id is NULL. Every one of them now goes through a single
resolveSessionUser (userId first, googleId fallback so sessions already in
Redis survive their 7-day TTL rather than logging everyone out on deploy).
The /connect/:mcpSlug OAuth state carries userId for the same reason:
without it a password user consents at the provider and gets "User not
found" on the way back.

Deliberate calls worth knowing:

- Passwords over 72 bytes are rejected, not truncated. bcrypt silently
  discards past that, so accepting them would claim a passphrase protects
  the account when only its first 72 bytes do.
- Login runs the bcrypt compare unconditionally — unknown emails and
  Google-only accounts compare against a dummy hash of equal cost, so
  response time is not an account-existence oracle (measured 192ms both
  paths). Registration's 409 discloses existence on purpose: a generic
  error there leaves the user unable to tell "taken" from "broken".
- regenerateApiKey(googleId) is deleted rather than kept. Its
  WHERE google_id = $1 matched zero rows for these accounts and reported
  "user not found" on a valid one; leaving it is a trap for the next
  caller. regenerateApiKeyByUserId is the only version.
- Linking Google to an existing password account keeps the password and
  keeps auth_method='password' (both the DB ON CONFLICT path and its
  file-store sibling). The dashboard renders authMethod verbatim, so
  relabelling would announce the loss of a credential that still works.
- password_hash stays out of DB_USER_COLUMNS and UserProfile, so nothing
  that serializes a user can leak a hash it never selected.

Rate limiting caps attempts per IP and per email (8 / 15 min, Redis-backed
with an in-memory fallback). It counts every attempt, not just failures —
counting failures alone lets one valid login reset the window. The email
bucket is the load-bearing half, since trust proxy: true makes req.ip
spoofable via X-Forwarded-For.

Known gap: there is no password reset. This repo has no mail
infrastructure, so a forgotten password has no self-service path until a
provider is added.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 56 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0810aced-0e1e-4217-9123-b84fa991f5c8

📥 Commits

Reviewing files that changed from the base of the PR and between d736e6b and 3c22349.

📒 Files selected for processing (11)
  • claude.md
  • public/dashboard.html
  • public/index.html
  • public/login.html
  • src/__tests__/emailPasswordAuth.test.ts
  • src/__tests__/routes.test.ts
  • src/auth/password.ts
  • src/userStore.ts
  • src/website/loginRateLimit.ts
  • src/website/sessionStore.ts
  • src/website/webServer.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
78.8% Coverage on New Code (required ≥ 80%)
E Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant