refactor: fix ABAC attributes typing in useRoomIcon#38430
refactor: fix ABAC attributes typing in useRoomIcon#38430sharanyamahajan wants to merge 21 commits intoRocketChat:developfrom
Conversation
fix(admin): make Custom HTML card responsive on mobile
…n-loader fix(ui): add skeleton loading states to Room Contextual Bar views
…ptions fix: prevent startup crash on invalid MONGO_OPTIONS JSON
refactor: reduce complexity in RegisterForm
|
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 |
|
|
|
WalkthroughThis PR introduces multiple enhancements across the codebase: ABAC attribute handling in room icon logic, a promise-based initialization lifecycle for CachedStore, refined sidebar item management, account profile form refactoring with field trimming, HomePage layout overflow fixes, new ContextualListSkeleton component, MongoDB configuration safety improvements, architecture documentation, livechat store type expansion and functional component modernization, BaseRaw model API simplification, and web-UI registration form refactoring with centralized validation and error handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/models/src/models/BaseRaw.ts (1)
172-220: Threadsessionthrough trash operations and validatedeleteOneresult.The trash path ignores
FindOneAndDeleteOptionsand doesn't check the deletion result. Whensessionis provided for a transaction, passing it tofindOne,updateOne, anddeleteOneis mandatory—without it, the trash operations fall outside the transaction, breaking atomicity. Additionally, if the document is deleted betweenfindOneanddeleteOne(race condition), the method returns the staledocanyway, diverging from the normalfindOneAndDeletesemantics (which returnsnullon no match).Pass
sessionfrom options to all calls (findOne,updateOne,deleteOne). Check thedeleteResult.deletedCount; if not1, roll back the trash record and returnnullor throw, depending on your intended semantics.Example: thread session and check delete result
async findOneAndDelete(filter: Filter<T>, options?: FindOneAndDeleteOptions): Promise<WithId<T> | null> { + const session = options?.session; - const doc = await this.col.findOne(filter); + const doc = await this.col.findOne(filter, { ...options, projection: undefined, session }); if (!doc) { return null; } const { _id, ...record } = doc; const trash: TDeleted = { ...record, _deletedAt: new Date(), __collection__: this.name, } as unknown as TDeleted; await this.trash.updateOne( { _id } as Filter<TDeleted>, { $set: trash } as UpdateFilter<TDeleted>, - { upsert: true }, + { upsert: true, session }, ); try { - await this.col.deleteOne({ _id } as Filter<T>); + const deleteResult = await this.col.deleteOne({ _id } as Filter<T>, { session }); + if (deleteResult.deletedCount !== 1) { + await this.trash.deleteOne({ _id } as Filter<TDeleted>, { session }); + return null; + } } catch (originalError) { try { - const rollbackResult = await this.trash.deleteOne({ _id } as Filter<TDeleted>); + const rollbackResult = await this.trash.deleteOne({ _id } as Filter<TDeleted>, { session });packages/web-ui-registration/src/RegisterForm.tsx (1)
62-103: Clear staleserverErrorbefore a new submit.If a prior attempt set
serverError, it will persist across new attempts even if the next failure is field-specific. Clearing it on submit prevents stale callouts.✅ Suggested fix
const handleRegister = async ({ password, passwordConfirmation: _, ...formData }: LoginRegisterPayload) => { + setServerError(undefined); registerUser.mutate( { pass: password, ...formData }, { onError: handleRegisterError,
🤖 Fix all issues with AI agents
In `@apps/meteor/client/lib/cachedStores/CachedStore.ts`:
- Around line 321-336: The init method currently swallows errors (via
.catch(console.error)) and always calls setReady(true) in .finally, which marks
the store ready even when performInitialization failed; change init so
initializationPromise preserves rejection (remove the silent swallow), call
setReady(true) only on successful completion of performInitialization (e.g., in
.then), and call setReady(false) or leave it unset on rejection (e.g., in .catch
rethrow after logging), while still clearing initializationPromise in a .finally
block; locate symbols: init, initializationPromise, performInitialization,
setReady, and reconnectionComputation to update the control flow accordingly.
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx`:
- Around line 239-245: Field labels (FieldLabel) use htmlFor but the inputs
rendered by Controller lack matching id attributes; update each Controller
render to pass the corresponding id prop (e.g., nicknameId for the nickname
Controller) into the input component (e.g., TextInput), and do the same for the
Bio and Email controllers using their bioId and emailId identifiers so the label
htmlFor matches the input id and assistive tech can associate them correctly.
- Around line 90-91: The code in AccountProfileForm currently early-returns when
username is falsy (the block checking "if (!username || username ===
previousUsername) { return; }"), which skips validation and allows empty
usernames; fix this by either (A) adding a required validation rule to the
username form field so the form framework enforces non-empty input, or (B)
replacing the early-return with explicit client-side handling that rejects empty
submissions (e.g., call the form's setError/validate method or show a
user-facing error/toast) while keeping the short-circuit for unchanged
usernames; update the username field definition and the submit handler that
references "username" and "previousUsername" accordingly so empty values are
blocked.
In `@apps/meteor/packages/rocketchat-mongo-config/server/index.js`:
- Around line 38-44: The code logs the raw MONGO_OPTIONS string (mongoOptionStr)
which may contain sensitive TLS/mongo credentials; remove the
console.error('Provided value:', mongoOptionStr) call and instead log only
non-sensitive metadata or a redacted form (e.g., list of top-level keys or
string length) and the parsing error (error.message); implement a small
redaction step that checks mongoOptionStr for sensitive property names like
tlsCertificateKeyFilePassword, tlsCAFile, tlsCertificateKeyFile and replaces
their values with '[REDACTED]' before logging, leaving the rest of the error
handling (message variable and throw new Error(message, { cause: error }))
intact so the exception is still raised.
In `@ee/packages/license/__tests__/MockedLicenseBuilder.js`:
- Around line 1-66: Remove the generated CommonJS test artifact
MockedLicenseBuilder.js from the repo (it duplicates the TypeScript source) by
deleting ee/packages/license/__tests__/MockedLicenseBuilder.js and committing
that deletion; keep only MockedLicenseBuilder.ts and ensure exported symbols
(MockedLicenseBuilder, getReadyLicenseManager) are implemented in the .ts file;
if tests fail after removal, update the test runner config (or ts-node/ts-jest
setup) so tests execute TypeScript directly rather than relying on committed .js
files.
In `@packages/livechat/src/components/Menu/index.tsx`:
- Around line 109-114: The Menu function component isn't forwarding refs so
menuRef stays null and positioning in the useLayoutEffect never runs; wrap Menu
with forwardRef from 'preact/compat' (use forwardRef to accept (props, ref) and
attach ref to the root <div> in the Menu render), remove the redundant ref?: any
from the MenuProps type, and ensure callers still pass menuRef to Menu (the
identifier Menu, menuRef, useLayoutEffect, and MenuProps are the key symbols to
update).
🧹 Nitpick comments (12)
docs/architecture/overview.md (1)
25-29: Fix table pipe spacing to satisfy MD060.The table header row is missing spaces around pipes in the “compact” style. This triggers markdownlint.
🧹 Suggested fix
-| Directory | Purpose | Runtime role | -|---------|--------|--------------| +| Directory | Purpose | Runtime role | +| --------- | -------- | ------------ |apps/meteor/client/views/home/HomePageHeader.tsx (1)
15-21: Consider extracting the shared overflow wrapper style.This style object is identical to the one in
HomePage.tsx. For consistency and maintainability, you could extract it to a shared constant (e.g., in a common styles file for home views).// Shared style constant example export const fullWidthOverflowHidden = { width: '100%', maxWidth: '100%', overflowX: 'hidden', } as const;This is a minor suggestion and can be deferred if preferred.
apps/meteor/packages/rocketchat-mongo-config/server/index.js (2)
15-17: Avoid inline comments in implementation.
Per repo guidance, these should live in docs or issues rather than code comments.🧹 Proposed removal
-// FIX for TLS error -// https://github.com/RocketChat/Rocket.Chat/issues/9316 -// TODO: Remove after NodeJS fixAs per coding guidelines, "Avoid code comments in the implementation".
55-61: Avoid inline comments in implementation.
Consider moving these notes to docs/issues to align with the code-comment guideline.🧹 Proposed removal
-// Just print to logs if in TEST_MODE due to a bug in Meteor 2.5 ... - // Send emails to a "fake" stream instead of printing them to consoleAs per coding guidelines, "Avoid code comments in the implementation".
packages/models/src/models/BaseRaw.ts (1)
170-170: Remove banner comments to comply with “no implementation comments”.
These markers add noise and violate the project guideline.Also applies to: 222-222
packages/livechat/src/store/types.ts (1)
7-31: Remove section divider and inline comments.These section headers and the inline note add noise and conflict with the style guide; spacing is sufficient to separate the interfaces.
🧹 Suggested cleanup
-/* ------------------ Alerts ------------------ */ export interface LivechatAlert { id: string; text: string; severity?: 'info' | 'warning' | 'error'; } - -/* ------------------ Queue Info ------------------ */ export interface LivechatQueueInfo { position?: number; estimatedWaitTime?: number; } - -/* ------------------ Config ------------------ */ export interface LivechatConfig { allowSwitchingDepartments?: boolean; showConnecting?: boolean; limitTextLength?: number; } - -/* ------------------ Modal ------------------ */ export interface LivechatModalState { open: boolean; type?: string; - payload?: unknown; // unknown > any (safer) + payload?: unknown; }As per coding guidelines, avoid code comments in the implementation.
packages/livechat/src/store/index.tsx (1)
24-34: Remove section header comments.These section dividers add noise and conflict with the style guide. Apply the same cleanup to the other section headers below as well.
🧹 Suggested cleanup
-/* ---------------- Hidden system messages ---------------- */ - export type LivechatHiddenSytemMessageType = | 'uj' | 'ul' | 'livechat-close' | 'livechat-started' | 'livechat_transfer_history'; - -/* ---------------- Store State ---------------- */As per coding guidelines, avoid code comments in the implementation.
packages/web-ui-registration/src/hooks/useRegisterValidation.ts (1)
32-37: Avoidanyfor password confirmation context.
values: anyloses type safety; a narrow shape is sufficient here.♻️ Suggested typing update
export const useRegisterValidation = ({ requireNameForRegister, requiresPasswordConfirmation, password }: Params) => { const { t } = useTranslation(); const passwordIsValid = useValidatePassword(password); + type PasswordConfirmationValues = { password: string }; return { @@ - validate: (val: string, values: any) => + validate: (val: string, values: PasswordConfirmationValues) => values.password === val ? true : t('registration.component.form.invalidConfirmPass'),packages/web-ui-registration/src/RegisterForm.tsx (1)
40-40: Remove inline section comments in implementation.These comments are now redundant given the structured JSX blocks and add noise in the component.
🧹 Example cleanup
- // Settings @@ - // IDs @@ - {/* Name */} @@ - {/* Email */} @@ - {/* Username */} @@ - {/* Password */} @@ - {/* Confirm Password */} @@ - {/* Reason */}As per coding guidelines: Avoid code comments in the implementation.
Also applies to: 49-49, 124-124, 147-147, 170-170, 193-193, 217-217, 242-242
packages/web-ui-registration/src/hooks/useRegisterErrorHandling.ts (1)
4-14: TightensetErrorand error typing to avoidany.Using
anyhides field-level type safety and error shape expectations. Consider typingsetErrorand the error payload explicitly usingreact-hook-formtypes.♻️ Suggested typing update
import { useTranslation } from 'react-i18next'; import { useToastMessageDispatch } from '@rocket.chat/ui-contexts'; +import type { FieldValues, UseFormSetError } from 'react-hook-form'; -type Params = { - setError: any; +type Params<TFieldValues extends FieldValues> = { + setError: UseFormSetError<TFieldValues>; setLoginRoute: (route: 'login') => void; setServerError: (value: string | undefined) => void; }; + +type RegisterError = { + error?: string; + errorType?: string; + message?: string; +}; -export const useRegisterErrorHandling = ({ setError, setLoginRoute, setServerError }: Params) => { +export const useRegisterErrorHandling = <TFieldValues extends FieldValues>({ + setError, + setLoginRoute, + setServerError, +}: Params<TFieldValues>) => { const { t } = useTranslation(); const dispatchToastMessage = useToastMessageDispatch(); - return (error: any): void => { + return (error: RegisterError): void => {apps/meteor/client/views/account/profile/AccountProfileForm.tsx (1)
155-155: Drop the inline section comments to match repo style.
They add visual noise in JSX without improving structure.As per coding guidelines: Avoid code comments in the implementation.✂️ Suggested cleanup
- {/* Avatar */} @@ - {/* Name + Username */} @@ - {/* Status */} @@ - {/* Nickname */} @@ - {/* Bio */} @@ - {/* Email */}Also applies to: 173-173, 210-210, 238-238, 246-246, 259-259
packages/livechat/src/components/Menu/index.tsx (1)
40-45: Preserve base item classes when callers passclassName.With
{...props}afterclassName, a caller-suppliedclassNamereplaces the basemenu__itemstyles. Consider merging instead.♻️ Suggested adjustment to merge class names
-export const Item = ({ children, primary = false, danger = false, disabled = false, icon = undefined, ...props }: ItemProps) => ( +export const Item = ({ + children, + primary = false, + danger = false, + disabled = false, + icon = undefined, + className, + ...props +}: ItemProps) => ( <button - className={createClassName(styles, 'menu__item', { primary, danger, disabled })} + className={[createClassName(styles, 'menu__item', { primary, danger, disabled }), className].filter(Boolean).join(' ')} disabled={disabled} {...props} >
| private reconnectionComputation: Tracker.Computation | undefined; | ||
| private initializationPromise: Promise<void> | undefined; | ||
|
|
||
| init() { | ||
| if (this.initializationPromise) { | ||
| return this.initializationPromise; | ||
| } | ||
|
|
||
| this.initializationPromise = this.performInitialization() | ||
| .catch(console.error) | ||
| .finally(() => { | ||
| this.initializationPromise = undefined; | ||
| this.setReady(true); | ||
| }); | ||
|
|
||
| return this.initializationPromise; |
There was a problem hiding this comment.
Init resolves “ready” even on failure—consider preserving failure state.
Because errors are swallowed and setReady(true) runs in finally, a failed initialization still marks the store ready, which can mask missing data.
Suggested fix
- this.initializationPromise = this.performInitialization()
- .catch(console.error)
- .finally(() => {
- this.initializationPromise = undefined;
- this.setReady(true);
- });
+ this.initializationPromise = this.performInitialization()
+ .then(() => {
+ this.setReady(true);
+ })
+ .catch((err) => {
+ console.error(err);
+ throw err;
+ })
+ .finally(() => {
+ this.initializationPromise = undefined;
+ });🤖 Prompt for AI Agents
In `@apps/meteor/client/lib/cachedStores/CachedStore.ts` around lines 321 - 336,
The init method currently swallows errors (via .catch(console.error)) and always
calls setReady(true) in .finally, which marks the store ready even when
performInitialization failed; change init so initializationPromise preserves
rejection (remove the silent swallow), call setReady(true) only on successful
completion of performInitialization (e.g., in .then), and call setReady(false)
or leave it unset on rejection (e.g., in .catch rethrow after logging), while
still clearing initializationPromise in a .finally block; locate symbols: init,
initializationPromise, performInitialization, setReady, and
reconnectionComputation to update the control flow accordingly.
| if (!username || username === previousUsername) { | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate server-side validation for empty usernames in the update flow
rg -nP '\bupdateOwnBasicInfo\b' -SRepository: RocketChat/Rocket.Chat
Length of output: 4962
🏁 Script executed:
#!/bin/bash
# Get the server-side implementation of updateOwnBasicInfo
rg -n -A 40 "users.updateOwnBasicInfo" apps/meteor/app/api/server/v1/users.ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 1539
🏁 Script executed:
#!/bin/bash
# Find the validation schema definition
rg -n "isUsersUpdateOwnBasicInfoParamsPOST" -A 20Repository: RocketChat/Rocket.Chat
Length of output: 4037
🏁 Script executed:
#!/bin/bash
# Read the validation schema
cat -n packages/rest-typings/src/v1/users/UsersUpdateOwnBasicInfoParamsPOST.tsRepository: RocketChat/Rocket.Chat
Length of output: 2009
🏁 Script executed:
#!/bin/bash
# Find the executeSaveUserProfile implementation
rg -n "executeSaveUserProfile" -A 30 --type ts --type js | head -150Repository: RocketChat/Rocket.Chat
Length of output: 8405
🏁 Script executed:
#!/bin/bash
# Find saveUserProfile function definition
rg -n "^function saveUserProfile|^const saveUserProfile" -A 40 apps/meteor/server/methods/saveUserProfile.tsRepository: RocketChat/Rocket.Chat
Length of output: 1133
🏁 Script executed:
#!/bin/bash
# Search for the actual saveUserProfile function implementation
rg -n "Meteor.methods\|saveUserProfile\s*=" apps/meteor/server/methods/saveUserProfile.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Read the entire saveUserProfile.ts file to see the implementation
wc -l apps/meteor/server/methods/saveUserProfile.tsRepository: RocketChat/Rocket.Chat
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Read the entire saveUserProfile.ts file
cat -n apps/meteor/server/methods/saveUserProfile.tsRepository: RocketChat/Rocket.Chat
Length of output: 9390
🏁 Script executed:
#!/bin/bash
# Find saveUserIdentity implementation
rg -n "saveUserIdentity" -A 50 --type ts | head -150Repository: RocketChat/Rocket.Chat
Length of output: 15081
🏁 Script executed:
#!/bin/bash
# Find the actual saveUserIdentity implementation file
fd -t f "saveUserIdentity" apps/meteor/app/lib/server/functions/Repository: RocketChat/Rocket.Chat
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Read the saveUserIdentity implementation
cat -n apps/meteor/app/lib/server/functions/saveUserIdentity.tsRepository: RocketChat/Rocket.Chat
Length of output: 6817
🏁 Script executed:
#!/bin/bash
# Get more context around the early return in AccountProfileForm.tsx
sed -n '80,120p' apps/meteor/client/views/account/profile/AccountProfileForm.tsxRepository: RocketChat/Rocket.Chat
Length of output: 1183
🏁 Script executed:
#!/bin/bash
# Search for form field validation and required attribute on username field
rg -n "username" apps/meteor/client/views/account/profile/AccountProfileForm.tsx | head -40Repository: RocketChat/Rocket.Chat
Length of output: 1001
🏁 Script executed:
#!/bin/bash
# Get more context around the form field definition and validation rules
sed -n '1,250p' apps/meteor/client/views/account/profile/AccountProfileForm.tsx | tail -100Repository: RocketChat/Rocket.Chat
Length of output: 2820
🏁 Script executed:
#!/bin/bash
# Check if username field has required/not-null constraint in the database schema or validation
rg -n "username" apps/meteor/app/lib/server/functions/validateUsername.tsRepository: RocketChat/Rocket.Chat
Length of output: 333
Add required validation rule for username field or explicitly handle empty submissions.
The early return at lines 90-91 skips validation when username is empty, and the form field lacks a required rule despite the visual "required" indicator. The backend accepts empty usernames without error (silently ignoring them). To align the UI intent with actual validation, either add a required rule to the form field or implement explicit client-side handling to reject empty submissions.
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx` around lines
90 - 91, The code in AccountProfileForm currently early-returns when username is
falsy (the block checking "if (!username || username === previousUsername) {
return; }"), which skips validation and allows empty usernames; fix this by
either (A) adding a required validation rule to the username form field so the
form framework enforces non-empty input, or (B) replacing the early-return with
explicit client-side handling that rejects empty submissions (e.g., call the
form's setError/validate method or show a user-facing error/toast) while keeping
the short-circuit for unchanged usernames; update the username field definition
and the submit handler that references "username" and "previousUsername"
accordingly so empty values are blocked.
| <Field> | ||
| <FieldLabel htmlFor={nicknameId}>{t('Nickname')}</FieldLabel> | ||
| <FieldRow> | ||
| <Controller | ||
| control={control} | ||
| name='nickname' | ||
| render={({ field }) => ( | ||
| <TextInput {...field} id={nicknameId} flexGrow={1} addon={<Icon name='edit' size='x20' alignSelf='center' />} /> | ||
| )} | ||
| /> | ||
| <Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} />} /> | ||
| </FieldRow> | ||
| </Field> | ||
|
|
There was a problem hiding this comment.
Associate labels with inputs for Nickname, Bio, and Email.
FieldLabel uses htmlFor, but the inputs lack matching id, which breaks label association for assistive tech.
✅ Add missing ids
- <Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} />} />
+ <Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} id={nicknameId} />} />
@@
- render={({ field }) => <TextAreaInput {...field} rows={3} />}
+ render={({ field }) => <TextAreaInput {...field} id={bioId} rows={3} />}
@@
- render={({ field }) => <TextInput {...field} disabled={!allowEmailChange} />}
+ render={({ field }) => <TextInput {...field} id={emailId} disabled={!allowEmailChange} />}Also applies to: 247-257, 260-270
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx` around lines
239 - 245, Field labels (FieldLabel) use htmlFor but the inputs rendered by
Controller lack matching id attributes; update each Controller render to pass
the corresponding id prop (e.g., nicknameId for the nickname Controller) into
the input component (e.g., TextInput), and do the same for the Bio and Email
controllers using their bioId and emailId identifiers so the label htmlFor
matches the input id and assistive tech can associate them correctly.
| const message = 'Invalid MONGO_OPTIONS environment variable: must be valid JSON.'; | ||
|
|
||
| console.error(message); | ||
| console.error('Provided value:', mongoOptionStr); | ||
| console.error('Error:', error.message); | ||
|
|
||
| throw new Error(message, { cause: error }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for MONGO_OPTIONS references and documentation
rg -n "MONGO_OPTIONS" -C3 --type-listRepository: RocketChat/Rocket.Chat
Length of output: 455
🏁 Script executed:
#!/bin/bash
# Search for MONGO_OPTIONS references and documentation
rg -n "MONGO_OPTIONS" -C3Repository: RocketChat/Rocket.Chat
Length of output: 2463
🏁 Script executed:
#!/bin/bash
# Examine the specific file and surrounding context
cat -n apps/meteor/packages/rocketchat-mongo-config/server/index.jsRepository: RocketChat/Rocket.Chat
Length of output: 2656
🏁 Script executed:
#!/bin/bash
# Check if there's any README or documentation about environment variables
find . -type f \( -name "README*" -o -name "*.md" \) | head -20 | xargs grep -l "MONGO_OPTIONS" 2>/dev/nullRepository: RocketChat/Rocket.Chat
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Check the changeset to understand the context
cat .changeset/fix-mongo-options-error-handling.mdRepository: RocketChat/Rocket.Chat
Length of output: 175
🏁 Script executed:
#!/bin/bash
# Search for MongoDB connection options documentation or examples in the repo
rg -i "mongo.*option" --type md | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1017
🌐 Web query:
MongoDB Node.js driver connection options sensitive data
💡 Result:
In the MongoDB Node.js driver, “sensitive data” most commonly ends up in (a) the connection string and (b) a few specific client options—and it’s easy to leak via logs, error reports, or config dumps.
Where sensitive data can appear
1) Connection string (URI)
The URI can embed credentials in the authority section: mongodb://[username:password@]... / mongodb+srv://[username:password@].... Treat the whole URI as a secret if it contains username:password@. [1]
Also remember URI query options (?option=value) are still part of the URI and often get logged alongside it. [1]
2) TLS / certificate-related options
The following Node driver connection options can directly contain secrets or secret-adjacent material:
tlsCertificateKeyFilePassword(private key decryption password) [2]- Client private key material (
key, ortlsCertificateKeyFilepointing to it) and client certs (cert) are sensitive files/data even if not “passwords” [3]
Also, avoid “make it work” insecure TLS toggles in production: tlsAllowInvalidCertificates,tlsAllowInvalidHostnames,tlsInsecure(explicitly warned as insecure / testing-only) [3]
(If you use mongodb+srv://, TLS is enabled by default unless you explicitly disable it.) [1,3]
3) App metadata you might not expect
appName isn’t a credential, but it is recorded in server logs / profiling—so don’t put secrets or user PII in it. [4]
Practical handling recommendations
- Do not hardcode URIs/passwords in source; load them from environment variables or a secret manager.
- Never log the full URI (or options objects) in app logs/telemetry. If you must log, redact: username/password and any TLS key password fields.
- Prefer TLS with valid certificates; don’t ship with
tlsInsecure/ invalid-cert allowances enabled. [3]
Sources: MongoDB connection string formats [1], Node.js driver TLS options and warnings [3], Node.js driver connection options (TLS key password) [2], Node.js driver connection options (appName logging) [4].
Don't log raw MONGO_OPTIONS; sensitive MongoDB options can leak in error logs.
The MONGO_OPTIONS environment variable can contain sensitive MongoDB driver connection options, including tlsCertificateKeyFilePassword (TLS certificate key decryption password) and other TLS-related secrets. Logging the raw JSON string (line 41) exposes these credentials in error logs.
🔒 Proposed redaction
- console.error('Provided value:', mongoOptionStr);
- console.error('Error:', error.message);
+ console.error('Provided value length:', mongoOptionStr.length);
+ console.error('Error:', error);🤖 Prompt for AI Agents
In `@apps/meteor/packages/rocketchat-mongo-config/server/index.js` around lines 38
- 44, The code logs the raw MONGO_OPTIONS string (mongoOptionStr) which may
contain sensitive TLS/mongo credentials; remove the console.error('Provided
value:', mongoOptionStr) call and instead log only non-sensitive metadata or a
redacted form (e.g., list of top-level keys or string length) and the parsing
error (error.message); implement a small redaction step that checks
mongoOptionStr for sensitive property names like tlsCertificateKeyFilePassword,
tlsCAFile, tlsCertificateKeyFile and replaces their values with '[REDACTED]'
before logging, leaving the rest of the error handling (message variable and
throw new Error(message, { cause: error })) intact so the exception is still
raised.
| "use strict"; | ||
| var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) { | ||
| function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); } | ||
| return new (P || (P = Promise))(function (resolve, reject) { | ||
| function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } } | ||
| function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } } | ||
| function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); } | ||
| step((generator = generator.apply(thisArg, _arguments || [])).next()); | ||
| }); | ||
| }; | ||
| var __generator = (this && this.__generator) || function (thisArg, body) { | ||
| var _ = { label: 0, sent: function() { if (t[0] & 1) throw t[1]; return t[1]; }, trys: [], ops: [] }, f, y, t, g = Object.create((typeof Iterator === "function" ? Iterator : Object).prototype); | ||
| return g.next = verb(0), g["throw"] = verb(1), g["return"] = verb(2), typeof Symbol === "function" && (g[Symbol.iterator] = function() { return this; }), g; | ||
| function verb(n) { return function (v) { return step([n, v]); }; } | ||
| function step(op) { | ||
| if (f) throw new TypeError("Generator is already executing."); | ||
| while (g && (g = 0, op[0] && (_ = 0)), _) try { | ||
| if (f = 1, y && (t = op[0] & 2 ? y["return"] : op[0] ? y["throw"] || ((t = y["return"]) && t.call(y), 0) : y.next) && !(t = t.call(y, op[1])).done) return t; | ||
| if (y = 0, t) op = [op[0] & 2, t.value]; | ||
| switch (op[0]) { | ||
| case 0: case 1: t = op; break; | ||
| case 4: _.label++; return { value: op[1], done: false }; | ||
| case 5: _.label++; y = op[1]; op = [0]; continue; | ||
| case 7: op = _.ops.pop(); _.trys.pop(); continue; | ||
| default: | ||
| if (!(t = _.trys, t = t.length > 0 && t[t.length - 1]) && (op[0] === 6 || op[0] === 2)) { _ = 0; continue; } | ||
| if (op[0] === 3 && (!t || (op[1] > t[0] && op[1] < t[3]))) { _.label = op[1]; break; } | ||
| if (op[0] === 6 && _.label < t[1]) { _.label = t[1]; t = op; break; } | ||
| if (t && _.label < t[2]) { _.label = t[2]; _.ops.push(op); break; } | ||
| if (t[2]) _.ops.pop(); | ||
| _.trys.pop(); continue; | ||
| } | ||
| op = body.call(thisArg, _); | ||
| } catch (e) { op = [6, e]; y = 0; } finally { f = t = 0; } | ||
| if (op[0] & 5) throw op[1]; return { value: op[0] ? op[1] : void 0, done: true }; | ||
| } | ||
| }; | ||
| Object.defineProperty(exports, "__esModule", { value: true }); | ||
| exports.getReadyLicenseManager = exports.MockedLicenseBuilder = void 0; | ||
| var src_1 = require("../src"); | ||
| var MockedLicenseBuilder_1 = require("../src/MockedLicenseBuilder"); | ||
| Object.defineProperty(exports, "MockedLicenseBuilder", { enumerable: true, get: function () { return MockedLicenseBuilder_1.MockedLicenseBuilder; } }); | ||
| var getReadyLicenseManager = function () { return __awaiter(void 0, void 0, void 0, function () { | ||
| var license; | ||
| return __generator(this, function (_a) { | ||
| switch (_a.label) { | ||
| case 0: | ||
| license = new src_1.LicenseImp(); | ||
| return [4 /*yield*/, license.setWorkspaceUrl('http://localhost:3000')]; | ||
| case 1: | ||
| _a.sent(); | ||
| license.setLicenseLimitCounter('activeUsers', function () { return 0; }); | ||
| license.setLicenseLimitCounter('guestUsers', function () { return 0; }); | ||
| license.setLicenseLimitCounter('roomsPerGuest', function () { return __awaiter(void 0, void 0, void 0, function () { return __generator(this, function (_a) { | ||
| return [2 /*return*/, 0]; | ||
| }); }); }); | ||
| license.setLicenseLimitCounter('privateApps', function () { return 0; }); | ||
| license.setLicenseLimitCounter('marketplaceApps', function () { return 0; }); | ||
| license.setLicenseLimitCounter('monthlyActiveContacts', function () { return __awaiter(void 0, void 0, void 0, function () { return __generator(this, function (_a) { | ||
| return [2 /*return*/, 0]; | ||
| }); }); }); | ||
| return [2 /*return*/, license]; | ||
| } | ||
| }); | ||
| }); }; | ||
| exports.getReadyLicenseManager = getReadyLicenseManager; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a corresponding TypeScript source file
fd -e ts -p "MockedLicenseBuilder" ee/packages/license/ 2>/dev/null || echo "No .ts MockedLicenseBuilder found"
# List all files in the license package
echo "=== Files in ee/packages/license/ ==="
find ee/packages/license -type f -name "MockedLicenseBuilder*" 2>/dev/null
# Check the project's tsconfig to understand the build setup
echo "=== tsconfig.json at package level ==="
cat ee/packages/license/tsconfig.json 2>/dev/null || echo "tsconfig.json not found at package level"
# Check if there's a root tsconfig or build config
echo "=== Root tsconfig.json ==="
cat tsconfig.json 2>/dev/null | head -50
# Check if compiled files in __tests__ are typical for this repo
echo "=== Sample .js files in __tests__ directories ==="
find ee/packages -name "__tests__" -type d -exec find {} -name "*.js" -type f \; 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 815
Remove the compiled .js file; maintain only the TypeScript source.
The repository contains ee/packages/license/__tests__/MockedLicenseBuilder.ts alongside the compiled MockedLicenseBuilder.js in the same directory. The .js file is redundant transpiled output that should not be committed alongside its TypeScript source. The build configuration (tsconfig.json) does not include __tests__/ in its compilation targets, indicating this .js file is not part of the normal build pipeline.
Remove ee/packages/license/__tests__/MockedLicenseBuilder.js and let the test runner handle TypeScript directly, or configure it to compile on-the-fly.
🤖 Prompt for AI Agents
In `@ee/packages/license/__tests__/MockedLicenseBuilder.js` around lines 1 - 66,
Remove the generated CommonJS test artifact MockedLicenseBuilder.js from the
repo (it duplicates the TypeScript source) by deleting
ee/packages/license/__tests__/MockedLicenseBuilder.js and committing that
deletion; keep only MockedLicenseBuilder.ts and ensure exported symbols
(MockedLicenseBuilder, getReadyLicenseManager) are implemented in the .ts file;
if tests fail after removal, update the test runner config (or ts-node/ts-jest
setup) so tests execute TypeScript directly rather than relying on committed .js
files.
| return ( | ||
| <Menu | ||
| ref={this.handleRef} | ||
| style={{ position: 'absolute', ...this.state.position }} | ||
| placement={this.state.placement} | ||
| onClickCapture={this.handleClick} | ||
| ref={menuRef} | ||
| style={{ position: 'absolute', ...position }} | ||
| placement={placement} | ||
| onClickCapture={handleClick} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Preact 10.25.4 ref forwarding function components default behavior
💡 Result:
In Preact 10.25.4 (and Preact 10.x generally), the default behavior is not to forward refs through function components:
refis treated as a special field (likekey) and is filtered out ofprops, so a function component won’t receiveprops.refby default. (marvinh.dev)- Therefore, if you write
function MyComp(props) { ... }, and render<MyComp ref={r} />, thatrefwill not automatically reach a DOM element insideMyComp. - To forward a ref through a function component in Preact 10, you typically use
forwardReffrompreact/compat. (preactjs.com)
This changed in Preact 11, where refs are forwarded by default (so function components can receive ref as a normal prop), and the Preact 11 upgrade guide even shows how to “revert to the Preact 10 behavior,” confirming what 10.x did by default. (preactjs.com)
Citations:
- 1: https://marvinh.dev/blog/preact-vs-compat/?utm_source=openai
- 2: https://preactjs.com/guide/v10/differences-to-react?utm_source=openai
- 3: https://preactjs.com/guide/v11/upgrade-guide/?utm_source=openai
🏁 Script executed:
# Find and examine the Menu component
find packages/livechat -name "index.tsx" -path "*/Menu/*" -type fRepository: RocketChat/Rocket.Chat
Length of output: 113
🏁 Script executed:
# Check package.json to confirm Preact version
cd packages/livechat && cat package.json | grep -A 5 -B 5 "preact"Repository: RocketChat/Rocket.Chat
Length of output: 876
🏁 Script executed:
cat packages/livechat/src/components/Menu/index.tsxRepository: RocketChat/Rocket.Chat
Length of output: 4222
Menu component requires forwardRef to work with layout measurements.
In Preact 10.25.4, ref props are filtered out before reaching function components and are not forwarded by default. The Menu component is a plain function component without forwardRef, so passing ref={menuRef} won't attach the ref to the underlying <div>. This causes menuRef.current to always be null, making the useLayoutEffect return early and skip all positioning calculations.
Wrap Menu with forwardRef from preact/compat:
import { forwardRef } from 'preact/compat';
export const Menu = forwardRef(({ children, hidden, placement = '', ...props }, ref) => (
<div ref={ref} className={createClassName(styles, 'menu', { hidden, placement })} {...props}>
{children}
</div>
));Remove the ref?: any line from MenuProps as the FIXME comment suggests.
🤖 Prompt for AI Agents
In `@packages/livechat/src/components/Menu/index.tsx` around lines 109 - 114, The
Menu function component isn't forwarding refs so menuRef stays null and
positioning in the useLayoutEffect never runs; wrap Menu with forwardRef from
'preact/compat' (use forwardRef to accept (props, ref) and attach ref to the
root <div> in the Menu render), remove the redundant ref?: any from the
MenuProps type, and ensure callers still pass menuRef to Menu (the identifier
Menu, menuRef, useLayoutEffect, and MenuProps are the key symbols to update).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 20 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/packages/rocketchat-mongo-config/server/index.js">
<violation number="1" location="apps/meteor/packages/rocketchat-mongo-config/server/index.js:41">
P2: Raw MONGO_OPTIONS value is logged on parse failure, potentially exposing Mongo credentials or TLS material in logs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const message = 'Invalid MONGO_OPTIONS environment variable: must be valid JSON.'; | ||
|
|
||
| console.error(message); | ||
| console.error('Provided value:', mongoOptionStr); |
There was a problem hiding this comment.
P2: Raw MONGO_OPTIONS value is logged on parse failure, potentially exposing Mongo credentials or TLS material in logs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/packages/rocketchat-mongo-config/server/index.js, line 41:
<comment>Raw MONGO_OPTIONS value is logged on parse failure, potentially exposing Mongo credentials or TLS material in logs.</comment>
<file context>
@@ -4,50 +4,61 @@ import { PassThrough } from 'stream';
+ const message = 'Invalid MONGO_OPTIONS environment variable: must be valid JSON.';
+
+ console.error(message);
+ console.error('Provided value:', mongoOptionStr);
+ console.error('Error:', error.message);
+
</file context>
| console.error('Provided value:', mongoOptionStr); | |
| console.error('Provided value: [redacted]'); |
|
@sharanyamahajan it seems at this point you are only creating spam PRs, almost all of your PRs have the same unrelated changes, and a lot of them are duplicated, created without checking if someone else has created a PR. Keeping up with this will result in you being blocked from contributing to this repository. So please take more care when creating Pull Requests |
|
@MartinSchoeler Thank you for pointing this out, and I’m sorry for the inconvenience caused. That wasn’t my intention. I understand your concerns regarding duplicated and unrelated changes, and I’ll be much more careful going forward. I’ll make sure to: Check existing issues and PRs before creating new ones Thanks for the feedback,I appreciate the chance to correct this and contribute more responsibly. |
3 participants
Proposed changes (including videos or screenshots)
This PR removes stale
@ts-expect-errorsuppressions and updates theuseRoomIconhook typings to correctly includeabacAttributes, which were already added toIRoomin prior work.The change aligns the hook implementation with existing core typings, improves type safety, and reduces technical debt. No functional or visual behavior is changed.
Issue(s)
Related to #37091
Steps to test or reproduce
@ts-expect-errorsuppressions inuseRoomIcon.Further comments
This change is intentionally limited to type cleanup made possible by existing
IRoomdefinitions. It does not introduce new logic or UI changes.Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.