P2: fix(auth): filter unsupported OTP characters - #223
Conversation
🦋 Changeset detectedLatest commit: b308698 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 26 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (10)
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 |
|
🚅 Deployed to the ePDS-pr-223 environment in ePDS
|
Coverage Report for CI Build 30940616604Coverage increased (+0.06%) to 57.909%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
aspiers
changed the title
191c97b to
d3646f1
Compare
d3646f1 to
b163ef6
Compare
b163ef6 to
d082ad6
Compare
d082ad6 to
5dc9cce
Compare
|
(reply generated by OpenAI Codex) Addressed the maintainability finding reported in the SonarQube summary in 5dc9cce by replacing the numeric filter’s |
|
(reply generated by OpenAI Codex) CI context: the latest PR-attached E2E job did not execute the suite; it timed out waiting for Railway to mark deployment 5dc9cce successful. Railway has an active build/deployment delay incident. Format, lint, typecheck, build, tests, coverage, SonarCloud, and Coveralls pass. |
|
(reply generated by OpenAI Codex) Railway’s incident is resolved, the final deployment completed, and the current SHA now passes the full deployed E2E suite: https://github.com/hypercerts-org/ePDS/actions/runs/30563833577 |
The previous commit left the change in three inconsistent states. The main sign-in screen renders a segmented grid of one-character boxes rather than a single input, and its input/paste handlers only stripped whitespace. Codes copied with surrounding punctuation, and letters typed into a digits-only code, still reached the server and failed verification. It also never upper-cased, while alphanumeric codes are generated as A-Z0-9 and the other two forms upper-case on submit — so on desktop, where autocapitalize does nothing, typing a code in lowercase could never succeed. Route both handlers through a shared filterOtpChars() built from buildOtpInputFilter(). The charset filters are supersets of the old whitespace strip, so pasting a spaced code still works, and auto-advance, backspace/arrow navigation, paste-spreading and auto-submit are unaffected. account-login.ts inlined the same regex as a literal ternary instead of calling buildOtpInputFilter() the way recovery.ts does, so the shared helper had two divergent copies from the moment it was introduced. Use the helper. Only the rendered text changes (/\D/g rather than /[^0-9]/g); in non-unicode mode \D is exactly [^0-9]. Hoist otpLength/otpCharset to sit with the other client-side config vars so the filter does not depend on var hoisting across 130 lines. The changeset and the feature narrative both claimed the main sign-in form already had this filter, which was never true. Rewrite them to describe all three forms, and rename the changeset since it is no longer limited to the server-rendered ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
219a5f7 to
0637fa7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
features/otp-character-filtering.feature:3
- The feature description claims coverage for “every OTP form” (segmented sign-in grid, account-login, and recovery), but the only scenario in this feature file exercises the recovery OTP preview. Either add scenarios for the other forms or narrow the feature text so it matches what’s actually tested.
Feature: OTP character filtering
Every OTP form — the segmented sign-in grid, account-login, and recovery —
must apply the configured OTP character policy while the user types.
The feature narrative claimed all three OTP forms were covered while the only scenario drove the recovery preview. The grid is the form nearly every user meets, and it is the riskiest of the three: its filtering lives in JS input/paste listeners rather than an oninput attribute, and the paste listener also spreads the cleaned characters across boxes, auto-advances and auto-submits when full. None of that is visible in the rendered HTML, so login-page.test.ts can assert the filter was wired in but never that it behaves — a regression in the spreading would pass every unit test we have. Plumb otp_charset through /preview/login-otp the way /preview/recovery-otp already does, so a scenario can exercise both policies without redeploying with a different OTP_CHARSET. Reuse the existing .otp-box locator convention from auth.steps.ts. The grid needs its own steps rather than the recovery ones because it is N one-character boxes, not a single input, so the assertion is over all boxes — checking only the filled prefix would miss characters scattered into later boxes. Paste is synthesised as a ClipboardEvent with a DataTransfer: Playwright cannot portably seed the system clipboard, and the handler reads only event.clipboardData, so this still drives the real listener. Reword the narrative to say what is actually covered. account-login is left out because its OTP form is only reachable by POSTing to /account/send-otp, which needs a real better-auth send — there is no preview route to drive it from a browser, and inventing one to serve a test would add production surface for no user-facing gain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/auth-service/src/otp-input.ts:9
OtpCharsetis already defined and exported insrc/lib/otp-config-validation.ts. Introducing anotherOtpCharsettype here duplicates the source of truth and makes it easier for these to diverge over time (e.g., adding a new charset would require updating multiple aliases). Consider importing (and optionally re-exporting) the existing type instead of redefining it in this module.
export type OtpCharset = 'numeric' | 'alphanumeric'
Summary
Normalize full-length OTP inputs in account settings and account recovery as users type. Numeric codes reject non-digits, while alphanumeric codes uppercase and retain only supported characters.
Changes
Testing
pnpm format:checkpnpm lintpnpm typecheckpnpm testpnpm test:coverageScreenshots
Before: a numeric recovery-code field accepted an unsupported letter.
After: attempting the same input leaves the field empty and focused.
Notes
The first PR-attached E2E attempt was blocked by Railway’s resolved build/deployment incident. After the deployment completed, the current SHA passed the full deployed suite in run 30563833577.
Current-branch E2E was dispatched in run 30561600539 but canceled after the PR Railway environment remained unhealthy (
502) throughout service readiness. Local browser coverage, the Cucumber dry run, production build, unit suite, and Coveralls pass.Focused extraction and review of work originally proposed in [WIP] many usability fixes (needs untangling) #165.
The segmented sign-in control is intentionally excluded because PR auth: support mobile paste in segmented code input #204 replaces that implementation.
Manual browser coverage is still recommended for numeric and alphanumeric configurations.