feat(GRO-565): Better Auth Phase 3 - password change, OIDC discovery, session cleanup, email verification #268

2026-04-12T02:47:59Z

groombook-engineer[bot] commented

2026-04-12 02:47:59 +00:00

(Migrated from github.com)

Summary

Better Auth Phase 3 implementation: password change, OIDC discovery, session cleanup, email verification
Cherry-picked from commit 9cce0bc

Definition of Done

Clean PR for GRO-565 exists and CI passes
PR #267 closed
Clean PR for GRO-566 exists separately

cc @cpfarhood

## Summary - Better Auth Phase 3 implementation: password change, OIDC discovery, session cleanup, email verification - Cherry-picked from commit `9cce0bc` ## Definition of Done - [ ] Clean PR for GRO-565 exists and CI passes - [ ] PR #267 closed - [ ] Clean PR for GRO-566 exists separately cc @cpfarhood

github-actions[bot] commented

2026-04-12 02:55:03 +00:00

(Migrated from github.com)

Deployed to groombook-dev

Images: pr-268
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-268` **URL:** https://dev.groombook.farh.net Ready for UAT validation.

github-actions[bot] commented

2026-04-12 11:32:06 +00:00

(Migrated from github.com)

Deployed to groombook-dev

Images: pr-268
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-268` **URL:** https://dev.groombook.farh.net Ready for UAT validation.

groombook-engineer[bot] commented

2026-04-12 14:40:33 +00:00

(Migrated from github.com)

Security Review: PR #268 (GRO-565: Better Auth Phase 3)

Status: APPROVED with findings

Reviewed files: auth.ts, reminders.ts, AccountSettings.tsx, auth-client.ts

Finding 1: OIDC Discovery URL Injection (Medium)

Location: apps/api/src/lib/auth.ts:183-224

Issue: discoveryUrlStr is constructed via string interpolation from providerConfig.issuerUrl (a user-controlled environment variable). An attacker controlling issuerUrl could set it to a malicious URL that returns a crafted OIDC configuration with attacker-controlled token_endpoint and userinfo_endpoint URLs.

const discoveryUrlStr = `${providerConfig.issuerUrl}/.well-known/openid-configuration`;

When internalBaseUrl is set, the code rewrites the token/userinfo hosts, but the initial fetch URL itself remains attacker-controlled. The discovery fetch happens before any validation, and the fetched config is used to set production OAuth endpoints.

Risk: If an attacker can influence issuerUrl (e.g., via environment variable injection or a misconfigured deployment), they could redirect token exchange and userinfo requests to an attacker-controlled endpoint, enabling token interception.

Recommendation: Validate that issuerUrl uses an allowed scheme (https:// only) and point to an allowlist of trusted OIDC providers before using it to construct discovery URLs.

Finding 2: Password Change CSRF / Session Fixation (Low)

Location: apps/web/src/portal/sections/AccountSettings.tsx:165

Issue: The password change form requires currentPassword in the API call but the UI submit button is disabled only when newPassword is empty or passwords don't match. If currentPassword state is somehow cleared or manipulated client-side before submission, the request still fires to the backend.

The button condition:

const canSubmit = newPassword.length > 0 && passwordsMatch && !loading;

Risk: Potential for session fixation or CSRF if an attacker can manipulate the form state. The backend correctly requires currentPassword, but the frontend doesn't visually indicate that currentPassword is required until the user clicks submit and gets an error.

Recommendation: Add client-side validation that visually marks currentPassword as required before submit is enabled, or require currentPassword in the button disabled condition.

Finding 3: Email Verification Token in Plaintext (Info)

Location: apps/api/src/lib/auth.ts:245-252

The email verification sends the verification URL directly in plaintext:

text: `Click the link to verify your email: ${url}`,
html: `<p>Click the link to verify your email:</p><a href="${url}">${url}</a>`,

Risk: If emails are intercepted (MITM, compromised mail server, logs), the verification token is exposed. However, this is standard practice for email verification flows.

Recommendation: Consider using a signed JWT or short-lived token format that doesn't grant indefinite access if intercepted. Ensure verification links expire promptly (Better Auth defaults should be checked).

Findings Not Applicable

SQL Injection: Auth library uses drizzle ORM with parameterized queries. No raw SQL concatenation found.
Command Injection: No exec() or spawn() calls in changed code.
Path Traversal: No file system operations in changed code.
IDOR: Password change correctly uses session-based user identification.

Verdict

APPROVED for production with findings above. The code is generally well-structured with proper input validation. Findings 1 and 2 should be addressed in follow-up PRs to harden the security posture. Rate limiting change from memory to database is a security improvement for distributed deployments.

cc @cpfarhood

## Security Review: PR #268 (GRO-565: Better Auth Phase 3) ### Status: APPROVED with findings Reviewed files: auth.ts, reminders.ts, AccountSettings.tsx, auth-client.ts --- ### Finding 1: OIDC Discovery URL Injection (Medium) **Location:** `apps/api/src/lib/auth.ts:183-224` **Issue:** `discoveryUrlStr` is constructed via string interpolation from `providerConfig.issuerUrl` (a user-controlled environment variable). An attacker controlling `issuerUrl` could set it to a malicious URL that returns a crafted OIDC configuration with attacker-controlled `token_endpoint` and `userinfo_endpoint` URLs. ```typescript const discoveryUrlStr = `${providerConfig.issuerUrl}/.well-known/openid-configuration`; ``` When `internalBaseUrl` is set, the code rewrites the token/userinfo hosts, but the initial fetch URL itself remains attacker-controlled. The discovery fetch happens before any validation, and the fetched config is used to set production OAuth endpoints. **Risk:** If an attacker can influence `issuerUrl` (e.g., via environment variable injection or a misconfigured deployment), they could redirect token exchange and userinfo requests to an attacker-controlled endpoint, enabling token interception. **Recommendation:** Validate that `issuerUrl` uses an allowed scheme (`https://` only) and point to an allowlist of trusted OIDC providers before using it to construct discovery URLs. --- ### Finding 2: Password Change CSRF / Session Fixation (Low) **Location:** `apps/web/src/portal/sections/AccountSettings.tsx:165` **Issue:** The password change form requires `currentPassword` in the API call but the UI submit button is disabled only when `newPassword` is empty or passwords don't match. If `currentPassword` state is somehow cleared or manipulated client-side before submission, the request still fires to the backend. The button condition: ```typescript const canSubmit = newPassword.length > 0 && passwordsMatch && !loading; ``` **Risk:** Potential for session fixation or CSRF if an attacker can manipulate the form state. The backend correctly requires `currentPassword`, but the frontend doesn't visually indicate that `currentPassword` is required until the user clicks submit and gets an error. **Recommendation:** Add client-side validation that visually marks `currentPassword` as required before submit is enabled, or require `currentPassword` in the button disabled condition. --- ### Finding 3: Email Verification Token in Plaintext (Info) **Location:** `apps/api/src/lib/auth.ts:245-252` The email verification sends the verification URL directly in plaintext: ```typescript text: `Click the link to verify your email: ${url}`, html: `<p>Click the link to verify your email:</p><a href="${url}">${url}</a>`, ``` **Risk:** If emails are intercepted (MITM, compromised mail server, logs), the verification token is exposed. However, this is standard practice for email verification flows. **Recommendation:** Consider using a signed JWT or short-lived token format that doesn't grant indefinite access if intercepted. Ensure verification links expire promptly (Better Auth defaults should be checked). --- ### Findings Not Applicable - **SQL Injection:** Auth library uses drizzle ORM with parameterized queries. No raw SQL concatenation found. - **Command Injection:** No `exec()` or `spawn()` calls in changed code. - **Path Traversal:** No file system operations in changed code. - **IDOR:** Password change correctly uses session-based user identification. --- ### Verdict **APPROVED for production with findings above.** The code is generally well-structured with proper input validation. Findings 1 and 2 should be addressed in follow-up PRs to harden the security posture. Rate limiting change from `memory` to `database` is a security improvement for distributed deployments. cc @cpfarhood

This repo is archived. You cannot comment on pull requests.

1 Participants

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: groombook/app#268