feat: show ToS/privacy checkbox for new users during login#34
Open
feat: show ToS/privacy checkbox for new users during login#34
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds server-side new-user detection and Terms-of-Service enforcement to the email-OTP flow: a DID lookup determines Changes
Sequence DiagramsequenceDiagram
participant User as User (Browser)
participant LoginSvc as Login Service
participant PDS as PDS
participant Auth as Auth Middleware
rect rgba(100, 150, 200, 0.5)
Note over User,PDS: Email & New-User Check Phase
User->>LoginSvc: GET /oauth/authorize (optional login_hint)
LoginSvc->>PDS: getDidByEmail(email)
PDS-->>LoginSvc: DID or null
LoginSvc->>LoginSvc: compute isNewUser
LoginSvc-->>User: Render login page (show ToS if isNewUser)
User->>LoginSvc: POST /api/auth/new-user-check
LoginSvc->>PDS: getDidByEmail(email)
PDS-->>LoginSvc: DID or null
LoginSvc-->>User: { isNewUser: boolean }
end
rect rgba(150, 100, 200, 0.5)
Note over User,Auth: OTP & ToS Validation Phase
User->>LoginSvc: POST /send-otp (email)
LoginSvc-->>User: OTP sent
User->>User: Check ToS checkbox (if isNewUser)
User->>Auth: POST /sign-in/email-otp (email, otp, tosAccepted)
Auth->>PDS: getDidByEmail(email)
PDS-->>Auth: DID or null
alt isNewUser && !tosAccepted
Auth-->>User: BAD_REQUEST (ToS required)
else
Auth->>Auth: Proceed with OTP verification
Auth-->>User: Success / Set session
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Pull Request Test Coverage Report for Build 23729195009Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/auth-service/src/routes/login-page.ts (1)
363-366: Use visibility classes for the ToS field instead of mutatingstyle.display.The rest of the page already uses
hidden/activeclasses for state, but the new ToS field adds inlinedisplayplus repeatedstyle.displaymutations. Keeping#tos-fieldon the same class-based pattern will make the state transitions easier to follow and stays consistent with the repo rule here.
As per coding guidelines,CSS classes control visibility (hidden,active); avoid inlinedisplaystyle except for dynamic values set at render time.Also applies to: 410-419, 472-475, 557-558, 615-617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 363 - 366, The ToS field currently uses inline style.display mutations for `#tos-field/.tos-field`; change this to the existing class-based visibility pattern by removing any direct style.display assignments and instead toggling the repository's visibility classes (e.g., "hidden"/"active") on the `#tos-field` element; update the CSS so .tos-field respects those classes (or add rules for `#tos-field.hidden` and `#tos-field.active`) and replace all JS occurrences that set element.style.display (including the instances referenced around lines ~410-419, ~472-475, ~557-558, ~615-617) with classList.add/remove/toggle on the same element ID to keep behavior consistent with other page state transitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 242-260: The GET handler for '/api/auth/new-user-check' leaks
account existence and PII via query string; change the flow to stop exposing
email in a public GET. Replace this public GET
(router.get('/api/auth/new-user-check', ...)) by moving the getDidByEmail lookup
into an existing authenticated/opaque POST flow (e.g., the OTP submission
endpoint or the auth_flow state handler) or return the isNewUser value from the
server-side auth_flow stash instead of a standalone GET; ensure you remove or
deprecate the GET route and update callers to POST the email in the request body
within an authenticated or nonce-protected flow so emails are not logged in URLs
and account enumeration is prevented.
- Around line 510-515: The function sendOtpAndCheckNewUser currently uses await
Promise.all([sendOtp(email), checkIsNewUser(email)]) which blocks OTP
success/failure on the slower DID lookup; change it to fire the auxiliary check
asynchronously so OTP flow is not delayed — call sendOtp(email) and await its
result immediately, then kick off checkIsNewUser(email) without awaiting (or
await it separately with a timeout/fallback) and merge results when available;
update both occurrences around sendOtpAndCheckNewUser and the similar block at
lines ~548-560 to ensure the ToS/“isNewUser” UI defaults to visible until the
lookup resolves and any lookup errors/timeouts are handled without preventing
OTP display.
- Around line 410-419: The ToS checkbox (`#tos-accept` inside tos-field) is not
sent to the server and verifyOtp() posts only { email, otp }; add a name (e.g.,
name="tosAccepted") to the checkbox and include its boolean value in the payload
sent by verifyOtp(), then update server-side validation to require and persist
this field and only call recordClientLogin after consent is true; also fix the
consent-page flow so new users are routed to the consent page (where
recordClientLogin runs) before finalizing auth.
---
Nitpick comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 363-366: The ToS field currently uses inline style.display
mutations for `#tos-field/.tos-field`; change this to the existing class-based
visibility pattern by removing any direct style.display assignments and instead
toggling the repository's visibility classes (e.g., "hidden"/"active") on the
`#tos-field` element; update the CSS so .tos-field respects those classes (or add
rules for `#tos-field.hidden` and `#tos-field.active`) and replace all JS
occurrences that set element.style.display (including the instances referenced
around lines ~410-419, ~472-475, ~557-558, ~615-617) with
classList.add/remove/toggle on the same element ID to keep behavior consistent
with other page state transitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36636078-cf70-4ffd-9264-8a1feeb0ebdb
📒 Files selected for processing (2)
.beads/issues.jsonlpackages/auth-service/src/routes/login-page.ts
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/auth-service/src/routes/login-page.ts (3)
418-427:⚠️ Potential issue | 🟠 MajorThe ToS checkbox still never leaves the browser.
verifyOtp()still posts only{ email, otp }, so this is enforced only by the browser’srequiredhandling and can be skipped by any scripted client. If this is supposed to represent consent, send atosAcceptedflag here and reject completion when it is absent or false.Also applies to: 532-538
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 418 - 427, The ToS checkbox is only enforced client-side; update the verifyOtp() flow to send a tosAccepted boolean and enforce it server-side: read the checkbox state (id "tos-accept") when building the payload in the verifyOtp() call and include tosAccepted:true, and update the server handler that finalizes OTP verification to reject the request (HTTP 400/403) when tosAccepted is missing or false; ensure the same change is applied for the other occurrence referenced (lines ~532-538) so both client calls and server-side verification use the new tosAccepted flag.
201-205:⚠️ Potential issue | 🟠 MajorThis is still a public account-enumeration route.
Moving the browser-facing check to POST fixes the public query-string leak, but
/oauth/authorizeis still public, only presence-checksrequest_uri, and embedscsrfTokeninto the returned HTML. That still lets an anonymous client mint a flow and probe arbitrary emails through this standalone endpoint. Also, both new call sites still go throughgetDidByEmail(), which hits/_internal/account-by-email?email=..., so the address can still land in internal access logs/proxies. Please keep this lookup inside an existing opaque auth-flow POST (or stash it onauth_flow) instead of returning account existence from a standalone route.Also applies to: 247-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 201 - 205, The GET handler currently computes isNewUser by calling getDidByEmail(resolvedEmail, pdsInternalUrl, internalSecret) and returns HTML containing csrfToken, which leaks account-existence via a public GET (/oauth/authorize) and logs email to internal account-by-email; remove that lookup from the public GET path (the isNewUser calculation and any direct calls to getDidByEmail in login-page.ts), and instead perform the email existence check inside the protected POST auth flow (or store the result on the existing auth_flow object) so the lookup is made server-side behind authentication/opaque state; ensure the GET only renders without calling getDidByEmail or exposing csrfToken tied to an email probe, and update call sites that referenced isNewUser to read the stored auth_flow value set during POST processing.
523-529:⚠️ Potential issue | 🟠 Major
Promise.all()still blocks the OTP step on the DID lookup.
checkIsNewUser()waits ongetDidByEmail(), which can take up to 3 seconds before returningnull. BecausesendOtpAndCheckNewUser()awaitsPromise.all(...)and the UI transition happens only afterward, a slow PDS still delays the OTP screen and even OTP-send errors. Show the OTP step as soon assendOtp()resolves, then update the ToS state when the auxiliary lookup finishes.Also applies to: 561-573
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 523 - 529, sendOtpAndCheckNewUser currently uses Promise.all([sendOtp(email), checkIsNewUser(email)]) which blocks the OTP UI until the DID lookup completes; instead kick off checkIsNewUser(email) without awaiting it, await sendOtp(email) immediately so the OTP step and any sendOtp errors appear as soon as sendOtp resolves, and then handle the checkIsNewUser result asynchronously (e.g., isNewUserPromise.then(isNewUser => update ToS state / resolve a secondary promise)); update the logic in sendOtpAndCheckNewUser (and the duplicate block at the other occurrence) to start checkIsNewUser(email) in background, await sendOtp(email) first, and propagate OTP errors immediately while applying the new-user result when it arrives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 418-427: The ToS checkbox is only enforced client-side; update the
verifyOtp() flow to send a tosAccepted boolean and enforce it server-side: read
the checkbox state (id "tos-accept") when building the payload in the
verifyOtp() call and include tosAccepted:true, and update the server handler
that finalizes OTP verification to reject the request (HTTP 400/403) when
tosAccepted is missing or false; ensure the same change is applied for the other
occurrence referenced (lines ~532-538) so both client calls and server-side
verification use the new tosAccepted flag.
- Around line 201-205: The GET handler currently computes isNewUser by calling
getDidByEmail(resolvedEmail, pdsInternalUrl, internalSecret) and returns HTML
containing csrfToken, which leaks account-existence via a public GET
(/oauth/authorize) and logs email to internal account-by-email; remove that
lookup from the public GET path (the isNewUser calculation and any direct calls
to getDidByEmail in login-page.ts), and instead perform the email existence
check inside the protected POST auth flow (or store the result on the existing
auth_flow object) so the lookup is made server-side behind authentication/opaque
state; ensure the GET only renders without calling getDidByEmail or exposing
csrfToken tied to an email probe, and update call sites that referenced
isNewUser to read the stored auth_flow value set during POST processing.
- Around line 523-529: sendOtpAndCheckNewUser currently uses
Promise.all([sendOtp(email), checkIsNewUser(email)]) which blocks the OTP UI
until the DID lookup completes; instead kick off checkIsNewUser(email) without
awaiting it, await sendOtp(email) immediately so the OTP step and any sendOtp
errors appear as soon as sendOtp resolves, and then handle the checkIsNewUser
result asynchronously (e.g., isNewUserPromise.then(isNewUser => update ToS state
/ resolve a secondary promise)); update the logic in sendOtpAndCheckNewUser (and
the duplicate block at the other occurrence) to start checkIsNewUser(email) in
background, await sendOtp(email) first, and propagate OTP errors immediately
while applying the new-user result when it arrives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce5a88fe-9a89-4bd0-b7dc-896cfefa5b57
📒 Files selected for processing (1)
packages/auth-service/src/routes/login-page.ts
chore: allow up to 1% coverage decrease in Coveralls
Kzoeps
force-pushed
the
karma/hyper-186-agreeing-to-tos-and-privacy
branch
from
27fdb7f to
432106a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/auth-service/src/routes/login-page.ts (1)
523-529:⚠️ Potential issue | 🟠 Major
Promise.all()still ties the OTP transition to the auxiliary DID lookup.
sendOtpAndCheckNewUser()does not let the UI advance untilcheckIsNewUser()resolves, so a slow new-user lookup can still hold the OTP step open even aftersendOtp()is done. Await the send result first, then reconcile the ToS UI asynchronously.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 523 - 529, sendOtpAndCheckNewUser currently uses Promise.all so a slow checkIsNewUser blocks returning the OTP result; change it to await sendOtp(email) first (using the existing sendOtp function) and then invoke checkIsNewUser(email) (the existing helper) asynchronously without awaiting—e.g. kick off checkIsNewUser(...).then(...).catch(...) or dispatch a background task to reconcile the ToS/UI—so the function returns the OTP result immediately while the new-user lookup completes independently.
🧹 Nitpick comments (1)
packages/auth-service/src/routes/login-page.ts (1)
418-427: Prefer class-based visibility for the ToS field.This state is now flipped in several places, and direct
style.displaymutations are duplicating visibility logic that the rest of the page already handles with classes.As per coding guidelines, "CSS classes control visibility (
hidden,active); avoid inlinedisplaystyle except for dynamic values set at render time".Also applies to: 481-483, 572-573, 645-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 418 - 427, Replace inline visibility styling on the ToS container with class-based toggling: remove the style="display:none" from the element with id "tos-field" and ensure visibility is driven by CSS classes (e.g., add/remove a "hidden" or "active" class) consistent with the rest of the page; update any code that previously mutated style.display on "#tos-field" (and the other occurrences manipulating the ToS block) to toggle the same class instead, leaving the checkbox input id "tos-accept" and name "tosAccepted" unchanged so existing event handlers continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/__tests__/better-auth-otp.test.ts`:
- Around line 9-14: Update the test-suite header comment to accurately reflect
the implemented assertions: change the line describing DID lookup failures from
"fail open" to indicate they are treated as fail-closed unless tosAccepted ===
true, and ensure the summary lists enforceTosAcceptance (hooks.before) behaviors
exactly as tested (returning users skip ToS, new user with tosAccepted true
allowed, new user with tosAccepted false/absent rejected with BAD_REQUEST, and
getDidByEmail failures require tosAccepted === true to proceed). Target the
comment block that documents enforceTosAcceptance and the suite summary near the
top of better-auth-otp.test.ts so the description matches the test cases.
In `@packages/auth-service/src/better-auth.ts`:
- Around line 52-54: The hook is calling toLowerCase() on ctx.body.email which
can throw for non-string inputs and won't trim surrounding whitespace; update
the logic around the email variable (in better-auth.ts) to first verify typeof
(ctx.body as { email?: unknown } | undefined)?.email === 'string', then
normalize by trimming and lowercasing (e.g. email = email.trim().toLowerCase());
if the email is not a string or is empty after trim, return to let better-auth's
schema validation reject it. Ensure you reference the existing email variable
and the ctx.body access when making the change.
- Around line 60-64: The current new-user ToS check uses a truthy test on the
extracted tosAccepted value which allows non-boolean values like "yes" or 1 to
pass; update the extraction/validation for tosAccepted in better-auth.ts to
require a literal boolean true (e.g., evaluate (ctx.body as { tosAccepted?:
boolean } | undefined)?.tosAccepted === true) and use that strict check in the
if condition so only the boolean true satisfies the gate.
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 258-263: The route reads req.body.email without ensuring req.body
exists, causing a crash for requests with no JSON; update the email extraction
to guard req.body (e.g. check req.body exists or use optional chaining when
accessing req.body.email) and if req.body is missing treat it like a missing
email and return the same 400 response; update the block around the email
assignment in login-page.ts (where req.body.email is used) so it safely handles
undefined req.body before trimming/lowering the value.
---
Duplicate comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 523-529: sendOtpAndCheckNewUser currently uses Promise.all so a
slow checkIsNewUser blocks returning the OTP result; change it to await
sendOtp(email) first (using the existing sendOtp function) and then invoke
checkIsNewUser(email) (the existing helper) asynchronously without awaiting—e.g.
kick off checkIsNewUser(...).then(...).catch(...) or dispatch a background task
to reconcile the ToS/UI—so the function returns the OTP result immediately while
the new-user lookup completes independently.
---
Nitpick comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 418-427: Replace inline visibility styling on the ToS container
with class-based toggling: remove the style="display:none" from the element with
id "tos-field" and ensure visibility is driven by CSS classes (e.g., add/remove
a "hidden" or "active" class) consistent with the rest of the page; update any
code that previously mutated style.display on "#tos-field" (and the other
occurrences manipulating the ToS block) to toggle the same class instead,
leaving the checkbox input id "tos-accept" and name "tosAccepted" unchanged so
existing event handlers continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18cb539a-f276-41d1-b58c-c7145e156eac
📒 Files selected for processing (3)
packages/auth-service/src/__tests__/better-auth-otp.test.tspackages/auth-service/src/better-auth.tspackages/auth-service/src/routes/login-page.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/auth-service/src/routes/login-page.ts (1)
258-261:⚠️ Potential issue | 🟡 MinorGuard
req.bodybefore readingA POST with no JSON body or the wrong content type leaves
req.bodyundefined here, causing a crash before the 400 response can be returned.🛠️ Proposed fix
+ const body = req.body as { email?: unknown } | undefined const email = - typeof req.body.email === 'string' - ? req.body.email.trim().toLowerCase() + typeof body?.email === 'string' + ? body.email.trim().toLowerCase() : ''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 258 - 261, Guard access to req.body before reading email: update the email extraction in the login handler so you first check req.body exists and is an object (e.g., typeof req.body === 'object' && req.body !== null) and then verify req.body.email is a string; only then call trim().toLowerCase(), otherwise set email to '' and let the existing 400/validation logic run. Adjust the block that currently assigns const email = typeof req.body.email === 'string' ? req.body.email.trim().toLowerCase() : '' to perform the additional req.body null/typeof guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 255-256: The POST route for '/api/auth/new-user-check' (the
router.post call in login-page.ts) is currently untested and unused; either
remove this route and any helper functions it relies on, or add integration
tests that mount the better-auth handler (the auth middleware/handler defined
above in this file) and exercise the '/api/auth/new-user-check' endpoint to
assert expected status and response body for both new and existing users; if
keeping the route, create tests that instantiate the express app with the
better-auth handler mounted and send POST requests to '/api/auth/new-user-check'
to validate behavior and edge cases, otherwise delete the
router.post('/api/auth/new-user-check', ...) block and any associated helpers to
avoid dead code.
---
Duplicate comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 258-261: Guard access to req.body before reading email: update the
email extraction in the login handler so you first check req.body exists and is
an object (e.g., typeof req.body === 'object' && req.body !== null) and then
verify req.body.email is a string; only then call trim().toLowerCase(),
otherwise set email to '' and let the existing 400/validation logic run. Adjust
the block that currently assigns const email = typeof req.body.email ===
'string' ? req.body.email.trim().toLowerCase() : '' to perform the additional
req.body null/typeof guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58fb0485-489e-405d-b5f1-300c8ef184a6
📒 Files selected for processing (3)
packages/auth-service/src/__tests__/better-auth-otp.test.tspackages/auth-service/src/better-auth.tspackages/auth-service/src/routes/login-page.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/auth-service/src/tests/better-auth-otp.test.ts
- packages/auth-service/src/better-auth.ts
Kzoeps
force-pushed
the
karma/hyper-186-agreeing-to-tos-and-privacy
branch
from
432106a to
e28e624
Compare
!tosAccepted accepted any truthy value (e.g. "yes", 1). Use tosAccepted !== true to enforce a strict boolean check.
Promise.all() was blocking the OTP screen until the DID lookup completed (up to 3s). Now sendOtp() is awaited first so the OTP step appears immediately; checkIsNewUser() runs concurrently and the ToS checkbox is shown asynchronously when the result arrives.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
features/passwordless-authentication.feature (1)
27-33: Tighten the negative-path assertion to avoid false positives.Line 32 currently asserts only a generic error. This can pass for unrelated failures (e.g., bad OTP) and miss regressions in ToS enforcement.
Suggested test assertion refinement
Scenario: New user cannot complete OTP sign-in without accepting Terms of Service When the user requests an OTP for "newuser@example.com" Then the login page shows an OTP verification form And the OTP form shows a Terms of Service checkbox When the user submits the OTP code without accepting the Terms of Service - Then the verification form shows an error message + Then the verification form shows the error "You must accept the Terms of Service to create an account." + And the auth service returns a 400 Bad Request And the sign-in is not completed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/passwordless-authentication.feature` around lines 27 - 33, Tighten the negative-path assertion for the scenario "New user cannot complete OTP sign-in without accepting Terms of Service": change the generic step "Then the verification form shows an error message" to assert the specific ToS validation (e.g., error text containing "You must accept the Terms of Service" or the exact frontend validation key) and also assert that the authentication state did not change (replace/augment "And the sign-in is not completed" with an explicit check that no session/token was issued or that the user remains unauthenticated). Update the steps "When the user submits the OTP code without accepting the Terms of Service" and corresponding verification steps to validate both the specific ToS error message and absence of an auth token/session.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@features/passwordless-authentication.feature`:
- Around line 27-33: Tighten the negative-path assertion for the scenario "New
user cannot complete OTP sign-in without accepting Terms of Service": change the
generic step "Then the verification form shows an error message" to assert the
specific ToS validation (e.g., error text containing "You must accept the Terms
of Service" or the exact frontend validation key) and also assert that the
authentication state did not change (replace/augment "And the sign-in is not
completed" with an explicit check that no session/token was issued or that the
user remains unauthenticated). Update the steps "When the user submits the OTP
code without accepting the Terms of Service" and corresponding verification
steps to validate both the specific ToS error message and absence of an auth
token/session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14773e5f-a937-41d1-838b-b9d31d6c4676
📒 Files selected for processing (2)
features/passwordless-authentication.featurefeatures/security.feature
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
isNewUsercomment:null = unknown,true = existing user,false = new userWe currently dont store the accepted TOS anywhere but we do validate it on
betterAuthhook. So the user cant tamper client side and then get through, validation exists server side tooSummary by CodeRabbit