feat(auth-service): log better-auth OTP verification failures#186
feat(auth-service): log better-auth OTP verification failures#186aspiers wants to merge 2 commits into
Conversation
The main OAuth login flow posts directly from the browser to `/api/auth/sign-in/email-otp`, handled by better-auth's node handler, which we mount with no HTTP access logging. On verification failure better-auth throws an `APIError` whose message is the exact reason — "OTP expired", "Invalid OTP" or "Too many attempts" — but these reached no log at all, because `createBetterAuth()` configures neither `logger` nor `onAPIError`. Wire `onAPIError.onError` to a small `logBetterAuthApiError()` helper that logs 4xx `APIError`s at `warn` (visible at prod's default `info` level) with the status and reason message. 3xx redirects are filtered by better-auth before this runs; 5xx errors are already logged by its own handler, so both are skipped to avoid duplicate noise. Counting `OTP expired` vs `Invalid OTP` over time separates genuinely late emails from users retyping stale codes. Closes #184 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: de5456e The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
Coverage Report for CI Build 28799043777Coverage increased (+0.1%) to 56.169%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR improves observability in @certified-app/auth-service by surfacing better-auth email OTP verification failures (e.g. “OTP expired” vs “Invalid OTP”) into the service’s own pino logs via onAPIError, so production can distinguish delivery delay from user error.
Changes:
- Added an exported helper
logBetterAuthApiError()and wired it intobetterAuth({ onAPIError: { onError } })to log 4xxAPIErrors atwarn. - Added unit tests covering logging/skip behavior for 4xx vs 3xx/5xx and non-
APIErrorvalues. - Added a changeset documenting the new operator-facing logging behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/auth-service/src/better-auth.ts |
Adds logBetterAuthApiError() and hooks better-auth onAPIError to emit structured warn logs for OTP verification failures. |
packages/auth-service/src/__tests__/log-better-auth-api-error.test.ts |
Unit tests verifying which APIErrors are logged vs skipped and validating logged fields. |
.changeset/log-otp-verification-failures.md |
Changeset describing the new warn-level logging for OTP failure reasons for operators. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Surface better-auth 4xx client errors in our own logs. | ||
| * | ||
| * The main OAuth login flow posts straight from the browser to | ||
| * /api/auth/sign-in/email-otp, handled by better-auth's node handler. On |
There was a problem hiding this comment.
Done in de5456e — updated the module header to note the new onAPIError logging hook and dropped the stale "foundation-only, no existing behavior is changed" line.
- Include the APIError object (`err`) in the warn log so its cause and
stack are available when debugging unexpected 4xx errors, matching the
`{ err, ... }` convention used elsewhere in the package.
- Update the module header comment: the module now changes runtime
behaviour (adds warn logging), so drop the stale "foundation-only,
no existing behavior is changed" note.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
🚅 Deployed to the ePDS-pr-186 environment in ePDS
|
Problem
Fixes #184.
better-auth distinguishes expired OTPs from wrong OTPs when verifying (
OTP expiredvsInvalid OTP), but in production we never saw either:/api/auth/sign-in/email-otp(packages/auth-service/src/routes/login-page.ts), handled bytoNodeHandler(betterAuthInstance)with no HTTP access logging.onErroronly logs 4xxAPIErrors whenlogger.levelis set in thebetterAuth()options.createBetterAuth()configured neitherloggernoronAPIError, so "OTP expired" failures were completely silent server-side.Only the secondary
/account/verify-otproute logged verification failures.Change
Wire
onAPIError.onErrorincreateBetterAuth()to a small exported helperlogBetterAuthApiError():APIErrors atwarn(visible at prod's defaultinfolevel) under theauth:better-authlogger, with thestatus,statusCode, and reasonmessage(OTP expired/Invalid OTP/Too many attempts).FOUNDbefore this runs anyway) and 5xx errors (already logged by better-auth's own handler) to avoid noise and double-logging.APIErrorvalues.The email address is deliberately not logged: better-auth invokes
onAPIError.onErrorwith the instance-wide auth context, not the per-request endpoint context, so the request body is not reliably available there. The issue's "where safely available" caveat anticipated this.The handler is extracted into a pure, exported function so it can be unit-tested directly without instantiating a full better-auth instance — matching the existing test style in this package.
Why
Counting
OTP expiredvsInvalid OTPoccurrences over time directly answers how many users hit the 10-minute expiry window, and correlating those with email-send timestamps attributes them to delivery delay vs user behaviour. This is the signal that separates genuinely-late emails from users retyping old codes, and we were dropping it.Testing
log-better-auth-api-error.test.ts(7 cases): expired/invalid/too-many-attempts all logged atwarnwith the right reason;body.messagefallback toerror.message; 5xx, 3xx redirects, and non-APIErrorvalues all skipped.pnpm typecheck— clean.pnpm lint— clean.🤖 Generated with Claude Code