fix: allow unauthenticated access to users.sendConfirmationEmail#40702
Open
ricardogarim wants to merge 1 commit into
Open
fix: allow unauthenticated access to users.sendConfirmationEmail#40702ricardogarim wants to merge 1 commit into
ricardogarim wants to merge 1 commit into
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: afb5ca9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
Contributor
WalkthroughThe PR allows unauthenticated POSTs to users.sendConfirmationEmail, updates the server method to only send verification to a matching unverified stored email address, removes the DDP rate limiter entry, updates DDP client types, refactors tests, and adds a changeset. ChangesEmail Confirmation Authentication and Logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40702 +/- ##
===========================================
- Coverage 69.71% 69.63% -0.08%
===========================================
Files 3339 3339
Lines 123269 123291 +22
Branches 21971 22003 +32
===========================================
- Hits 85931 85852 -79
- Misses 33990 34077 +87
- Partials 3348 3362 +14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/server/methods/sendConfirmationEmail.ts (1)
16-16: ⚡ Quick winRemove the implementation comment.
The predicate is already clear enough without it.
As per coding guidelines "Avoid code comments in the implementation".Suggested change
- // Do not re-send a verification mail to an address that is already verified. const target = user.emails?.find((e) => e.address.toLowerCase() === email.toLowerCase() && !e.verified);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/methods/sendConfirmationEmail.ts` at line 16, Remove the implementation comment "// Do not re-send a verification mail to an address that is already verified." from the sendConfirmationEmail method to comply with "avoid code comments in the implementation" guideline; leave the existing predicate logic (the verification check) intact and ensure no other inline explanatory comments are added in sendConfirmationEmail.apps/meteor/tests/end-to-end/api/users.ts (1)
3724-3763: ⚡ Quick winExercise the unverified-email branch in at least one success-path test.
These cases only prove the uniform
200 { success: true }contract. None of them sets up a known unverified address, so the new!e.verifiedresend path can break without failing this suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/end-to-end/api/users.ts` around lines 3724 - 3763, The tests call api('users.sendConfirmationEmail') but never sets up an unverified address, so the new resend branch (!e.verified) isn't covered; update one success-path test (for example the "should accept unauthenticated requests (login-screen unblock path)" or the unknown-email test) to first create or mark a user with the email adminEmail as unverified (e.g., create a test user or set the user's verified flag to false via your test helpers or direct fixture) and then call request.post(api('users.sendConfirmationEmail')) with that email, asserting the same 200 { success: true } response so the !e.verified resend code path is exercised; reference adminEmail, credentials, and api('users.sendConfirmationEmail') when locating where to add the setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/app/api/server/v1/users.ts`:
- Around line 1498-1499: The route currently calls
sendConfirmationEmail(this.bodyParams.email) as a detached promise which can
cause unhandled rejections; fix by handling its promise before returning: either
await sendConfirmationEmail(...) if you want the route to wait for delivery, or
call sendConfirmationEmail(...).catch(err => { /* log or handle error via
Meteor._debug/processLogger/etc. */ }) to swallow/log errors and then return
API.v1.success(); reference sendConfirmationEmail and the surrounding return
API.v1.success() to locate the call.
---
Nitpick comments:
In `@apps/meteor/server/methods/sendConfirmationEmail.ts`:
- Line 16: Remove the implementation comment "// Do not re-send a verification
mail to an address that is already verified." from the sendConfirmationEmail
method to comply with "avoid code comments in the implementation" guideline;
leave the existing predicate logic (the verification check) intact and ensure no
other inline explanatory comments are added in sendConfirmationEmail.
In `@apps/meteor/tests/end-to-end/api/users.ts`:
- Around line 3724-3763: The tests call api('users.sendConfirmationEmail') but
never sets up an unverified address, so the new resend branch (!e.verified)
isn't covered; update one success-path test (for example the "should accept
unauthenticated requests (login-screen unblock path)" or the unknown-email test)
to first create or mark a user with the email adminEmail as unverified (e.g.,
create a test user or set the user's verified flag to false via your test
helpers or direct fixture) and then call
request.post(api('users.sendConfirmationEmail')) with that email, asserting the
same 200 { success: true } response so the !e.verified resend code path is
exercised; reference adminEmail, credentials, and
api('users.sendConfirmationEmail') when locating where to add the setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 980c6240-73ff-4d5b-9479-578727b010f5
📒 Files selected for processing (5)
.changeset/crisp-taxis-pay.mdapps/meteor/app/api/server/v1/users.tsapps/meteor/server/methods/sendConfirmationEmail.tsapps/meteor/tests/end-to-end/api/users.tspackages/ddp-client/src/types/methods.ts
💤 Files with no reviewable changes (1)
- packages/ddp-client/src/types/methods.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🔨 Test API Livechat (EE) / MongoDB 8.0 coverage (1/1)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/users.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/methods/sendConfirmationEmail.ts
🧠 Learnings (9)
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/crisp-taxis-pay.md
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/v1/users.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/methods/sendConfirmationEmail.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/v1/users.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/methods/sendConfirmationEmail.ts
📚 Learning: 2026-03-15T14:31:25.380Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:25.380Z
Learning: Do not flag this type/schema misalignment in the OpenAPI/migration review for apps/meteor/app/api/server/v1/users.ts. The UserCreateParamsPOST type intentionally uses non-optional fields: fields: string and settings?: IUserSettings without an AJV schema entry, carried over from the original rest-typings (PR `#39647`). Treat this as a known pre-existing divergence and document it as a separate follow-up fix; do not block or mark it as a review issue during the migration.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-03-16T23:33:11.443Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:11.443Z
Learning: In rockets OpenAPI/AJV migration reviews for RocketChat/Rocket.Chat, when reviewing migrations that involve apps/meteor/app/api/server/v1/users.ts, do not require or flag a missing query AJV schema for the fields consumed by parseJsonQuery() (i.e., fields, sort, query) as part of this endpoint's migration PR. The addition of global query-param schemas for parseJsonQuery() usage is a cross-cutting concern and out of scope for individual endpoint migrations. Only flag violations related to the specific scope of the migration, not the absence of a query schema for parseJsonQuery() in this file.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/api/server/v1/users.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/methods/sendConfirmationEmail.ts
📚 Learning: 2026-03-09T18:39:14.020Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:14.020Z
Learning: When implementing batch processing in server methods, favor a single data pass to collect both the items and any derived fields needed for validation. Use the same dataset for both validation and processing to avoid races between validation and execution, and document the approach in code comments. Apply this pattern to similar Meteor Rocket.Chat server methods under apps/meteor/server/methods to prevent race conditions and ensure consistent batch behavior.
Applied to files:
apps/meteor/server/methods/sendConfirmationEmail.ts
🔇 Additional comments (1)
.changeset/crisp-taxis-pay.md (1)
1-6: LGTM!
ricardogarim
force-pushed
the
fix/send-confirmation-email
branch
from
650a0fa to
afb5ca9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/server/methods/sendConfirmationEmail.ts (1)
16-17: 💤 Low valueRemove the inline implementation comment.
The predicate is already clear enough on its own, and this repo asks us to keep TS implementation files free of inline comments.
As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/methods/sendConfirmationEmail.ts` around lines 16 - 17, Remove the inline implementation comment before the find() call: delete the line "// Do not re-send a verification mail to an address that is already verified." so the code only contains the predicate (const target = user.emails?.find((e) => e.address.toLowerCase() === email.toLowerCase() && !e.verified);). Leave the logic and variable names (target, user.emails, email, e.verified) untouched.apps/meteor/tests/end-to-end/api/users.ts (1)
3724-3740: 🏗️ Heavy liftThis no longer exercises the send path.
Because the handler now always returns
200 { success: true }, these cases still pass even if no verification email is ever attempted. Please add coverage for an explicitly unverified address with an observable mail side effect, or coversendConfirmationEmail()directly in a lower-level test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/end-to-end/api/users.ts` around lines 3724 - 3740, The test currently only asserts a 200 response and never verifies that a confirmation email was actually sent; add a case that creates or uses an explicitly unverified user and asserts the mail side-effect (or unit-tests sendConfirmationEmail() directly). Specifically, update the tests around the request to api('users.sendConfirmationEmail') to (a) ensure an unverified account exists (e.g., create or mark a user as unverified), (b) reset or spy/stub the mailer/queue before the request, (c) call the endpoint (or call sendConfirmationEmail() directly) and then assert the mailer/queue was invoked with the expected confirmation email (or that sendConfirmationEmail() returns/produces the expected mail action); target symbols: sendConfirmationEmail, users.sendConfirmationEmail handler and the mailer/queue spy used in tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/app/api/server/v1/users.ts`:
- Around line 1498-1500: The current action() always fires-and-forgets
sendConfirmationEmail(this.bodyParams.email) which hides SMTP/provider failures
for authenticated callers; change it to check the authenticated context (e.g.,
this.userId or this.user) and if the request is authenticated await
sendConfirmationEmail(...) and let errors bubble so the caller receives the real
error, while keeping the anonymous path as a non-blocking call that logs
failures; ensure you still return API.v1.success() on success and preserve the
SystemLogger.error logging for the anonymous case.
---
Nitpick comments:
In `@apps/meteor/server/methods/sendConfirmationEmail.ts`:
- Around line 16-17: Remove the inline implementation comment before the find()
call: delete the line "// Do not re-send a verification mail to an address that
is already verified." so the code only contains the predicate (const target =
user.emails?.find((e) => e.address.toLowerCase() === email.toLowerCase() &&
!e.verified);). Leave the logic and variable names (target, user.emails, email,
e.verified) untouched.
In `@apps/meteor/tests/end-to-end/api/users.ts`:
- Around line 3724-3740: The test currently only asserts a 200 response and
never verifies that a confirmation email was actually sent; add a case that
creates or uses an explicitly unverified user and asserts the mail side-effect
(or unit-tests sendConfirmationEmail() directly). Specifically, update the tests
around the request to api('users.sendConfirmationEmail') to (a) ensure an
unverified account exists (e.g., create or mark a user as unverified), (b) reset
or spy/stub the mailer/queue before the request, (c) call the endpoint (or call
sendConfirmationEmail() directly) and then assert the mailer/queue was invoked
with the expected confirmation email (or that sendConfirmationEmail()
returns/produces the expected mail action); target symbols:
sendConfirmationEmail, users.sendConfirmationEmail handler and the mailer/queue
spy used in tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fec8ef23-1415-472a-ad8f-09673b60ea80
📒 Files selected for processing (5)
.changeset/crisp-taxis-pay.mdapps/meteor/app/api/server/v1/users.tsapps/meteor/server/methods/sendConfirmationEmail.tsapps/meteor/tests/end-to-end/api/users.tspackages/ddp-client/src/types/methods.ts
💤 Files with no reviewable changes (1)
- packages/ddp-client/src/types/methods.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/crisp-taxis-pay.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test API (CE) / MongoDB 8.0 (1/1)
- GitHub Check: 🔨 Test API Livechat (EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (3/4)
- GitHub Check: 🔨 Test API Livechat (CE) / MongoDB 8.0 (1/1)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
- GitHub Check: 🔨 Test API (EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test Federation Matrix
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/users.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/methods/sendConfirmationEmail.ts
🧠 Learnings (8)
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/v1/users.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/methods/sendConfirmationEmail.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/v1/users.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/methods/sendConfirmationEmail.ts
📚 Learning: 2026-03-15T14:31:25.380Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:25.380Z
Learning: Do not flag this type/schema misalignment in the OpenAPI/migration review for apps/meteor/app/api/server/v1/users.ts. The UserCreateParamsPOST type intentionally uses non-optional fields: fields: string and settings?: IUserSettings without an AJV schema entry, carried over from the original rest-typings (PR `#39647`). Treat this as a known pre-existing divergence and document it as a separate follow-up fix; do not block or mark it as a review issue during the migration.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-03-16T23:33:11.443Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:11.443Z
Learning: In rockets OpenAPI/AJV migration reviews for RocketChat/Rocket.Chat, when reviewing migrations that involve apps/meteor/app/api/server/v1/users.ts, do not require or flag a missing query AJV schema for the fields consumed by parseJsonQuery() (i.e., fields, sort, query) as part of this endpoint's migration PR. The addition of global query-param schemas for parseJsonQuery() usage is a cross-cutting concern and out of scope for individual endpoint migrations. Only flag violations related to the specific scope of the migration, not the absence of a query schema for parseJsonQuery() in this file.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/api/server/v1/users.tsapps/meteor/tests/end-to-end/api/users.tsapps/meteor/server/methods/sendConfirmationEmail.ts
📚 Learning: 2026-03-09T18:39:14.020Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:14.020Z
Learning: When implementing batch processing in server methods, favor a single data pass to collect both the items and any derived fields needed for validation. Use the same dataset for both validation and processing to avoid races between validation and execution, and document the approach in code comments. Apply this pattern to similar Meteor Rocket.Chat server methods under apps/meteor/server/methods to prevent race conditions and ensure consistent batch behavior.
Applied to files:
apps/meteor/server/methods/sendConfirmationEmail.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
KevLehman
approved these changes
May 27, 2026
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.
Proposed changes (including videos or screenshots)
When Admin -> Accounts -> Require email verification to login is on, an unverified user is blocked at the login screen and offered a "Send confirmation email" button. That button calls
POST /api/v1/users.sendConfirmationEmail, which was declaredauthRequired: true- so the unauthenticated user always got401, the verification mail was never sent, and the only workaround was for an admin to re-trigger verification by hand.Root cause
apps/meteor/app/api/server/v1/users.ts- the route was added in #26415 to back the authenticated Account Profile page, whereauthRequired: truewas correct. When the login screen later started calling the same endpoint, the auth gate was never relaxed. The login-screen flow has therefore never worked end-to-end - latent integration bug, not a regression.Solution
authRequired: false, drop401from the response schema, handler is fire-and-forget (void) and always returnsAPI.v1.success().sendConfirmationEmail- skip the send if the address is already verified. Function signature and behavior for the other caller (setEmail.ts) are preserved.Issue(s)
SUP-1043
Steps to test or reproduce
Pre-fix:
401. User is stuck.Post-fix:
200, verification mail is dispatched, link verifies the address, retry-login succeeds.200 {"success":true}:Further comments
users.forgotPassword- uniform response shape, per-IP rate limit preserved, mail only goes to the address recorded on the user document.Summary by CodeRabbit
New Features
Bug Fixes