Release 8.8.0 - #41894
Open
rocketchat-github-ci wants to merge 140 commits into
Open
Conversation
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…tion hooks from roomCoordinator and ui-contexts (#41508) Co-authored-by: Matheus Cardoso <matheus@cardo.so> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…e deflakes and Playwright GitHub annotations) (#41504)
…41484) Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: Matheus Cardoso <matheus@cardo.so> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Douglas Fabris <devfabris@gmail.com>
…40736) Co-authored-by: Diego Sampaio <chinello@gmail.com>
Co-authored-by: Tasso Evangelista <tasso.evangelista@rocket.chat> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
…eturning (#41805) Co-authored-by: gabriellsh <40830821+gabriellsh@users.noreply.github.com>
Co-authored-by: Tasso Evangelista <tasso.evangelista@rocket.chat> Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nel (#41795) Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Matheus Cardoso <matheus@cardo.so>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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: 1d6b435 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 |
Hacktron Security Check - SkippedReason: This PR exceeds Hacktron's 200-file review cap and will not be scanned. Split the PR into smaller changes for review coverage.
|
|
Aleksander Nicacio da Silva seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
|
Important Review skippedToo many files! This PR contains 1487 files, which is 1187 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (64)
📒 Files selected for processing (1487)
You can disable this status message by setting the |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41894 +/- ##
==========================================
+ Coverage 68.45% 69.36% +0.90%
==========================================
Files 4143 4255 +112
Lines 158433 168644 +10211
Branches 28561 30058 +1497
==========================================
+ Hits 108457 116976 +8519
- Misses 44843 46494 +1651
- Partials 5133 5174 +41
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
29 issues found across 1518 files
Not reviewed (too large): apps/meteor/tests/end-to-end/api/chat.ts (~2,612 lines), apps/meteor/tests/end-to-end/api/users.ts (~2,506 lines), packages/i18n/src/locales/de.i18n.json (~2,178 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/lib/statusVisibility/hiddenUsers.ts">
<violation number="1" location="apps/meteor/server/lib/statusVisibility/hiddenUsers.ts:12">
P2: When Spotlight requests only rooms, this helper still awaits a status-visibility service RPC, adding avoidable latency and a service dependency to room-only and anonymous searches. Perform the lookup only when the user-search surface is enabled.</violation>
</file>
<file name="apps/meteor/client/sidebar/RoomList/useSidebarListNavigation.ts">
<violation number="1" location="apps/meteor/client/sidebar/RoomList/useSidebarListNavigation.ts:5">
P2: Only one of the two duplicated copies of useSidebarListNavigation was updated. This file now matches 'rcx-sidebar-v2-collapse-group__bar-button', but the identical hook at apps/meteor/client/views/navigation/sidebar/RoomList/useSidebarListNavigation.ts:6 still checks 'rcx-sidebar-v2-collapse-group__bar', and both are wired into live navigation (the sidebar RoomListWrapper uses this copy, SidepanelListWrapper imports the other). Whichever class name matches the fuselage DOM, at least one surface now has divergent or broken Arrow/Tab handling for collapse groups. Sync the class guard across both copies (and confirm which class fuselage actually emits), or extract the shared hook so they cannot drift again.</violation>
</file>
<file name="apps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesWithData.tsx">
<violation number="1" location="apps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesWithData.tsx:126">
P2: When a workspace overrides these keys in `Custom_Translations` using the previous `%s` placeholders, the warning now displays literal `%s` tokens because this call no longer runs the sprintf postprocessor. Preserve sprintf compatibility alongside the named variables, or migrate existing custom translations.</violation>
</file>
<file name="apps/meteor/server/lib/users/getFullUserData.ts">
<violation number="1" location="apps/meteor/server/lib/users/getFullUserData.ts:151">
P2: If the setting is disabled after the projection is built, this condition skips ID-to-username conversion and returns raw `statusVisibilityDenied` IDs. Remove the field whenever the setting is disabled before returning.
(Based on your team's feedback about keeping preference IDs internal.) .</violation>
</file>
<file name="apps/meteor/app/apps/server/converters/codecs/uploads.ts">
<violation number="1" location="apps/meteor/app/apps/server/converters/codecs/uploads.ts:34">
P2: When an `IUpload` has no `rid`, this callback calls `rooms.convertById(undefined)`, so converting room-less uploads can fail instead of completing without a room. Check `rid` before the lookup and return `undefined` when it is absent.</violation>
</file>
<file name="apps/meteor/client/views/room/ClassificationBanner/lib/engine.ts">
<violation number="1" location="apps/meteor/client/views/room/ClassificationBanner/lib/engine.ts:39">
P2: When a grouped attribute uses an empty `multipleLabel`, this renders a blank banner despite matching values. Fall back to the joined values or require a non-empty `multipleLabel` for grouped attributes.</violation>
</file>
<file name="apps/meteor/client/navbar/NavBarPagesGroup/actions/useEncryptedRoomDescription.ts">
<violation number="1" location="apps/meteor/client/navbar/NavBarPagesGroup/actions/useEncryptedRoomDescription.ts:16">
P3: The new enforced-by-policy branch has no test coverage in useEncryptedRoomDescription.spec.ts. Add a case that sets E2E_Force_Encryption_For_Private_Rooms=true (with E2E_Enable=true) and asserts the "Encrypted_enforced_by_workspace_policy" result for private+encrypted rooms, so the new behavior is locked in.</violation>
</file>
<file name="apps/meteor/app/apps/server/converters/codecs/departments.ts">
<violation number="1" location="apps/meteor/app/apps/server/converters/codecs/departments.ts:29">
P2: When a department has multiple forwarding targets, this codec exposes an array through `IAppsDepartment.departmentsAllowedToForward`, whose declared Apps-Engine type is `string`. Align the Apps-Engine contract and the conversion before returning this value, rather than hiding the mismatch with the cast.</violation>
</file>
<file name="apps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsx">
<violation number="1" location="apps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsx:37">
P2: Replacing `callWithErrorHandling('sendMessage', ...)` with a bare `sdk.rest.post` drops the error toast that `callWithErrorHandling` dispatched via `dispatchToastMessage`. If sending the invite message fails, the user now gets no feedback, only a `console.warn` in the outer catch. Keep the user-facing error handling (e.g., wrap the REST call the same way `groups.create` errors are surfaced, or use a toast helper) so a send failure is visible.</violation>
</file>
<file name="apps/meteor/client/components/GenericNoResults/GenericNoResults.tsx">
<violation number="1" location="apps/meteor/client/components/GenericNoResults/GenericNoResults.tsx:27">
P2: The aria-label is hardcoded to `t('No_results_found')`, but many callers pass a custom `title` (e.g. `GenericNoResults title={t('ABAC_No_attributes')}`, `title={t('No_message_reports')}`). A screen reader will then announce "No results found" while the visible text reads the custom title, producing misleading output. Mirror the visible label: `aria-label={title || t('No_results_found')}`.</violation>
</file>
<file name="apps/meteor/server/lib/saml/lib/ServiceProvider.ts">
<violation number="1" location="apps/meteor/server/lib/saml/lib/ServiceProvider.ts:146">
P2: When a provider name is long enough, the new compound RelayState exceeds SAML HTTP-Redirect’s 80-byte limit and can make authorize requests fail. Keep the RelayState within the binding limit, or fall back to the legacy provider-only state or server-side state for long provider names.</violation>
</file>
<file name="apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts">
<violation number="1" location="apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts:37">
P2: When a loaded reply points to a parent older than the oldest loaded message, this bound omits the parent deletion, so the reply keeps a stale `tmid` after reconnect. Reconcile loaded replies' thread parents separately or adjust the sync query to include those parent deletions.</violation>
</file>
<file name="apps/meteor/client/views/account/security/TwoFactorTOTP.tsx">
<violation number="1" location="apps/meteor/client/views/account/security/TwoFactorTOTP.tsx:20">
P2: When either REST call rejects for an invalid TOTP code, `isInvalidTotpError` receives a `Response`, not the decoded error body, so the user gets a generic response error instead of `Invalid_two_factor_code`. Parse the rejected response body or use the shared REST error conversion before checking the error code.</violation>
</file>
<file name="apps/meteor/server/lib/omnichannel/business-hour/filterBusinessHoursThatMustBeOpened.ts">
<violation number="1" location="apps/meteor/server/lib/omnichannel/business-hour/filterBusinessHoursThatMustBeOpened.ts:51">
P2: During the finish minute, this predicate reports the business hour as open while the existing close cron removes it at the minute’s start. New agents or a business-hour save during that minute can therefore be assigned or reopened inconsistently with existing agents; move the close operation to after the finish minute or otherwise align the close path with this inclusive boundary.</violation>
</file>
<file name="apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx">
<violation number="1" location="apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx:137">
P2: When the room renders before its subscription is populated, this expression passes `undefined` for the thread draft and `useDraft` permanently captures that initial value. Load the server draft when it becomes available, or defer composer initialization until the subscription data is ready.</violation>
</file>
<file name="apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptsModal.tsx">
<violation number="1" location="apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptsModal.tsx:26">
P2: After switching to `useEndpoint`, the existing ReadReceiptsModal stories still mock only the removed DDP method, so Storybook and composed tests hit the mock provider's unimplemented endpoint and never render the receipt fixtures. Replace the story decorators' `withMethod` stubs with `withEndpoint('GET', '/v1/chat.getMessageReadReceipts', ...)` responses matching the REST `{ receipts }` shape.</violation>
</file>
<file name="apps/meteor/app/apps/server/converters/codecs/enums.ts">
<violation number="1" location="apps/meteor/app/apps/server/converters/codecs/enums.ts:26">
P2: Non-string enum values now pass the codec boundary because these schemas are `z.any()`. `UserTypeCodec` and `UserStatusConnectionCodec` can call `.toUpperCase()` on such values, while room/setting values can propagate unchanged; restore the original `z.string()`/`z.string().optional()` schemas for all four codecs.</violation>
</file>
<file name="apps/meteor/app/apps/server/converters/uploads.ts">
<violation number="1" location="apps/meteor/app/apps/server/converters/uploads.ts:33">
P2: When an upload has no `rid`, `convertToApp` now invokes the room converter with `undefined`. Preserve the previous missing-`rid` guard in the upload codec so roomless uploads convert without an invalid room lookup.</violation>
</file>
<file name="apps/meteor/client/lib/userPresence.ts">
<violation number="1" location="apps/meteor/client/lib/userPresence.ts:164">
P2: When a browser user was already marked idle, disabling auto-away leaves `idle` true and force-sends Away on the preference update or next reconnect. Clear browser idle state when auto-away is disabled, or gate `reassertPresence` on auto-away being enabled.</violation>
</file>
<file name="apps/meteor/client/views/room/composer/messageBox/hooks/useDraft.ts">
<violation number="1" location="apps/meteor/client/views/room/composer/messageBox/hooks/useDraft.ts:54">
P2: When the same composer is reopened or edited before a draft-save request resolves, the request's success handler deletes the newer local draft. Clear the key only if it still contains the value that this request saved.</violation>
</file>
<file name="apps/meteor/client/lib/utils/threadMessageUtils.ts">
<violation number="1" location="apps/meteor/client/lib/utils/threadMessageUtils.ts:90">
P2: When older thread pages are loaded, adding or deleting a message in a newer page leaves `pageParams` stale. `getPreviousPageParam` then requests the wrong offset, causing the next pagination fetch to skip a message or return a duplicate; rebase page parameters when the mutation changes preceding-page counts, or invalidate/refetch the pagination state.</violation>
</file>
<file name="apps/meteor/client/views/room/body/hooks/useIsAtBottomRef.ts">
<violation number="1" location="apps/meteor/client/views/room/body/hooks/useIsAtBottomRef.ts:8">
P2: useRef only applies its initial value on the first render, so the seed from RoomManager.getStore(rid)?.atBottom is never re-read when the rid changes on a later render. RoomBody is memoized and stays mounted across room switches (the inner Box/scrollbars are keyed by room._id to force per-room remounts), so opening a room that was previously left scrolled mid-history does not restore its stored atBottom — the ref keeps the value from the first room opened. Recompute the seed when rid changes, e.g. store the persisted value in state or set ref.current when rid differs.</violation>
</file>
<file name="apps/meteor/server/api/lib/queryFiltersStatus.ts">
<violation number="1" location="apps/meteor/server/api/lib/queryFiltersStatus.ts:10">
P2: When a users.list query filters on a field that merely starts with 'status' but is not a presence/redacted status field (e.g. statusLivechat for livechat availability, or a user custom field named like 'statusNote'), queryFiltersStatus returns true and users.ts:723 then excludes hidden users even though the filter has nothing to do with their private status. Consider matching an explicit set of status fields (status, statusText, statusConnection, statusDefault, statusExpiresAt, statusSource) instead of the 'status' prefix, or restricting the check so it doesn't apply to non-presence fields.</violation>
</file>
<file name="apps/meteor/ee/server/api/mcp/server.ts">
<violation number="1" location="apps/meteor/ee/server/api/mcp/server.ts:107">
P2: When a client cancels an in-flight `tools/call`, this branch drops the notification without aborting the corresponding `dispatchTool`; long-running or mutating REST calls continue. Track active request IDs and propagate cancellation to the loopback fetch.</violation>
</file>
<file name="apps/meteor/server/lib/2fa/functions/totp.ts">
<violation number="1" location="apps/meteor/server/lib/2fa/functions/totp.ts:21">
P1: When an account already has another 2FA method, a hijacked session can still call deprecated `2fa:enable` followed by `2fa:validateTempToken` to enroll TOTP without verifying the existing factor. Enforce the verification gate for the legacy enrollment callers too, or move it into a shared authenticated boundary before the DDP methods are removed.</violation>
<violation number="2" location="apps/meteor/server/lib/2fa/functions/totp.ts:139">
P2: After regenerating backup codes, the active security view continues displaying the old `codesRemaining` value because this write provides no new count or client update. Return the new remaining count or update the client state after regeneration.</violation>
</file>
<file name="apps/meteor/server/lib/saml/lib/Utils.ts">
<violation number="1" location="apps/meteor/server/lib/saml/lib/Utils.ts:167">
P3: When a provider name starts with `provider=` and contains `&loginClient=`, a flow without a login client is corrupted on callback because the decoder treats the raw provider as compound RelayState. Use an unambiguous marker or otherwise distinguish legacy raw RelayState before parsing.</violation>
</file>
<file name=".yarnrc.yml">
<violation number="1" location=".yarnrc.yml:2">
P2: `approvedGitRepositories: "**"` allows Yarn to fetch git dependencies from *any* repository. The default allowlist is empty, which blocks all git deps; this change disables that gate entirely. Since `enableScripts: true` is also enabled, a malicious git dependency can run arbitrary install scripts on every developer/CI machine. Scope the allowlist to the actual hosts used by the project instead of the `**` wildcard.</violation>
<violation number="2" location=".yarnrc.yml:16">
P2: `npmMinimalAgeGate: 0` disables Yarn's supply-chain protection that refuses to install npm packages published within the last day (default `1d`, introduced in Yarn 4.12). This reduces the attack surface by letting recently published, potentially compromised or already-republished packages be reviewed before installation. Keeping it at `1d` (or an explicit age) preserves that protection; use `yarn add --no-time-gate` for the rare legitimate case instead of disabling it globally.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 400 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
Re-trigger cubic
| return user; | ||
| }; | ||
|
|
||
| export const enableTotp = async (userId: string | null): Promise<{ secret: string; url: string }> => { |
Contributor
There was a problem hiding this comment.
P1: When an account already has another 2FA method, a hijacked session can still call deprecated 2fa:enable followed by 2fa:validateTempToken to enroll TOTP without verifying the existing factor. Enforce the verification gate for the legacy enrollment callers too, or move it into a shared authenticated boundary before the DDP methods are removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/2fa/functions/totp.ts, line 21:
<comment>When an account already has another 2FA method, a hijacked session can still call deprecated `2fa:enable` followed by `2fa:validateTempToken` to enroll TOTP without verifying the existing factor. Enforce the verification gate for the legacy enrollment callers too, or move it into a shared authenticated boundary before the DDP methods are removed.</comment>
<file context>
@@ -0,0 +1,154 @@
+ return user;
+};
+
+export const enableTotp = async (userId: string | null): Promise<{ secret: string; url: string }> => {
+ const user = await requireUser(userId);
+
</file context>
| return undefined; | ||
| } | ||
|
|
||
| const hidden = await StatusVisibility.getHiddenFrom(viewerId); |
Contributor
There was a problem hiding this comment.
P2: When Spotlight requests only rooms, this helper still awaits a status-visibility service RPC, adding avoidable latency and a service dependency to room-only and anonymous searches. Perform the lookup only when the user-search surface is enabled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/statusVisibility/hiddenUsers.ts, line 12:
<comment>When Spotlight requests only rooms, this helper still awaits a status-visibility service RPC, adding avoidable latency and a service dependency to room-only and anonymous searches. Perform the lookup only when the user-search surface is enabled.</comment>
<file context>
@@ -0,0 +1,21 @@
+ return undefined;
+ }
+
+ const hidden = await StatusVisibility.getHiddenFrom(viewerId);
+
+ return hidden.length ? new Set(hidden) : undefined;
</file context>
|
|
||
| const isListItem = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-item'); | ||
| const isCollapseGroup = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-collapse-group__bar'); | ||
| const isCollapseGroup = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-collapse-group__bar-button'); |
Contributor
There was a problem hiding this comment.
P2: Only one of the two duplicated copies of useSidebarListNavigation was updated. This file now matches 'rcx-sidebar-v2-collapse-group__bar-button', but the identical hook at apps/meteor/client/views/navigation/sidebar/RoomList/useSidebarListNavigation.ts:6 still checks 'rcx-sidebar-v2-collapse-group__bar', and both are wired into live navigation (the sidebar RoomListWrapper uses this copy, SidepanelListWrapper imports the other). Whichever class name matches the fuselage DOM, at least one surface now has divergent or broken Arrow/Tab handling for collapse groups. Sync the class guard across both copies (and confirm which class fuselage actually emits), or extract the shared hook so they cannot drift again.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/sidebar/RoomList/useSidebarListNavigation.ts, line 5:
<comment>Only one of the two duplicated copies of useSidebarListNavigation was updated. This file now matches 'rcx-sidebar-v2-collapse-group__bar-button', but the identical hook at apps/meteor/client/views/navigation/sidebar/RoomList/useSidebarListNavigation.ts:6 still checks 'rcx-sidebar-v2-collapse-group__bar', and both are wired into live navigation (the sidebar RoomListWrapper uses this copy, SidepanelListWrapper imports the other). Whichever class name matches the fuselage DOM, at least one surface now has divergent or broken Arrow/Tab handling for collapse groups. Sync the class guard across both copies (and confirm which class fuselage actually emits), or extract the shared hook so they cannot drift again.</comment>
<file context>
@@ -2,7 +2,7 @@ import { useFocusManager } from '@react-aria/focus';
const isListItem = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-item');
-const isCollapseGroup = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-collapse-group__bar');
+const isCollapseGroup = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-collapse-group__bar-button');
const isListItemMenu = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-item__menu');
</file context>
| sprintf: [users.map((element) => element).join(', ')], | ||
| })}` | ||
| : ''; | ||
| const ifFrom = users.length ? ` ${t('if_they_are_from', { users: users.map((element) => element).join(', ') })}` : ''; |
Contributor
There was a problem hiding this comment.
P2: When a workspace overrides these keys in Custom_Translations using the previous %s placeholders, the warning now displays literal %s tokens because this call no longer runs the sprintf postprocessor. Preserve sprintf compatibility alongside the named variables, or migrate existing custom translations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesWithData.tsx, line 126:
<comment>When a workspace overrides these keys in `Custom_Translations` using the previous `%s` placeholders, the warning now displays literal `%s` tokens because this call no longer runs the sprintf postprocessor. Preserve sprintf compatibility alongside the named variables, or migrate existing custom translations.</comment>
<file context>
@@ -123,19 +123,16 @@ const PruneMessagesWithData = () => {
- sprintf: [users.map((element) => element).join(', ')],
- })}`
- : '';
+ const ifFrom = users.length ? ` ${t('if_they_are_from', { users: users.map((element) => element).join(', ') })}` : '';
const filesOrMessages = attached ? t('files') : t('messages');
</file context>
| return user; | ||
| const ownBlockList = myself ? user.settings?.preferences?.statusVisibilityDenied : undefined; | ||
|
|
||
| if (settings.get<boolean>('Accounts_StatusVisibility_Enabled') && ownBlockList?.length && user.settings?.preferences) { |
Contributor
There was a problem hiding this comment.
P2: If the setting is disabled after the projection is built, this condition skips ID-to-username conversion and returns raw statusVisibilityDenied IDs. Remove the field whenever the setting is disabled before returning.
(Based on your team's feedback about keeping preference IDs internal.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/users/getFullUserData.ts, line 151:
<comment>If the setting is disabled after the projection is built, this condition skips ID-to-username conversion and returns raw `statusVisibilityDenied` IDs. Remove the field whenever the setting is disabled before returning.
(Based on your team's feedback about keeping preference IDs internal.) .</comment>
<file context>
@@ -140,5 +146,13 @@ export async function getFullUserDataByUniqueSearchTerm(
- return user;
+ const ownBlockList = myself ? user.settings?.preferences?.statusVisibilityDenied : undefined;
+
+ if (settings.get<boolean>('Accounts_StatusVisibility_Enabled') && ownBlockList?.length && user.settings?.preferences) {
+ user.settings.preferences.statusVisibilityDenied = (await resolveUsersByIds(ownBlockList)).usernames;
+ }
</file context>
|
|
||
| const { codes, hashedCodes } = TOTP.generateCodes(); | ||
|
|
||
| await Users.update2FABackupCodesByUserId(user._id, hashedCodes); |
Contributor
There was a problem hiding this comment.
P2: After regenerating backup codes, the active security view continues displaying the old codesRemaining value because this write provides no new count or client update. Return the new remaining count or update the client state after regeneration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/2fa/functions/totp.ts, line 139:
<comment>After regenerating backup codes, the active security view continues displaying the old `codesRemaining` value because this write provides no new count or client update. Return the new remaining count or update the client state after regeneration.</comment>
<file context>
@@ -0,0 +1,154 @@
+
+ const { codes, hashedCodes } = TOTP.generateCodes();
+
+ await Users.update2FABackupCodesByUserId(user._id, hashedCodes);
+
+ return { codes };
</file context>
|
|
||
| nodeLinker: node-modules | ||
|
|
||
| npmMinimalAgeGate: 0 |
Contributor
There was a problem hiding this comment.
P2: npmMinimalAgeGate: 0 disables Yarn's supply-chain protection that refuses to install npm packages published within the last day (default 1d, introduced in Yarn 4.12). This reduces the attack surface by letting recently published, potentially compromised or already-republished packages be reviewed before installation. Keeping it at 1d (or an explicit age) preserves that protection; use yarn add --no-time-gate for the rare legitimate case instead of disabling it globally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .yarnrc.yml, line 16:
<comment>`npmMinimalAgeGate: 0` disables Yarn's supply-chain protection that refuses to install npm packages published within the last day (default `1d`, introduced in Yarn 4.12). This reduces the attack surface by letting recently published, potentially compromised or already-republished packages be reviewed before installation. Keeping it at `1d` (or an explicit age) preserves that protection; use `yarn add --no-time-gate` for the rare legitimate case instead of disabling it globally.</comment>
<file context>
@@ -6,10 +9,14 @@ enableGlobalCache: false
+
nodeLinker: node-modules
+npmMinimalAgeGate: 0
+
plugins:
</file context>
Suggested change
| npmMinimalAgeGate: 0 | |
| npmMinimalAgeGate: 1d |
| @@ -1,3 +1,6 @@ | |||
| approvedGitRepositories: | |||
| - "**" | |||
Contributor
There was a problem hiding this comment.
P2: approvedGitRepositories: "**" allows Yarn to fetch git dependencies from any repository. The default allowlist is empty, which blocks all git deps; this change disables that gate entirely. Since enableScripts: true is also enabled, a malicious git dependency can run arbitrary install scripts on every developer/CI machine. Scope the allowlist to the actual hosts used by the project instead of the ** wildcard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .yarnrc.yml, line 2:
<comment>`approvedGitRepositories: "**"` allows Yarn to fetch git dependencies from *any* repository. The default allowlist is empty, which blocks all git deps; this change disables that gate entirely. Since `enableScripts: true` is also enabled, a malicious git dependency can run arbitrary install scripts on every developer/CI machine. Scope the allowlist to the actual hosts used by the project instead of the `**` wildcard.</comment>
<file context>
@@ -1,3 +1,6 @@
+approvedGitRepositories:
+ - "**"
+
checksumBehavior: update
</file context>
Suggested change
| - "**" | |
| - "https://github.com/*" | |
| - "ssh://git@github.com/*" |
| if (!isPrivate) { | ||
| return t('Encrypted_not_available', { roomType: t(roomType) }); | ||
| } | ||
| if (e2eEnforcedForPrivate && encrypted) { |
Contributor
There was a problem hiding this comment.
P3: The new enforced-by-policy branch has no test coverage in useEncryptedRoomDescription.spec.ts. Add a case that sets E2E_Force_Encryption_For_Private_Rooms=true (with E2E_Enable=true) and asserts the "Encrypted_enforced_by_workspace_policy" result for private+encrypted rooms, so the new behavior is locked in.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/navbar/NavBarPagesGroup/actions/useEncryptedRoomDescription.ts, line 16:
<comment>The new enforced-by-policy branch has no test coverage in useEncryptedRoomDescription.spec.ts. Add a case that sets E2E_Force_Encryption_For_Private_Rooms=true (with E2E_Enable=true) and asserts the "Encrypted_enforced_by_workspace_policy" result for private+encrypted rooms, so the new behavior is locked in.</comment>
<file context>
@@ -12,6 +13,9 @@ export const useEncryptedRoomDescription = (roomType: 'channel' | 'team' | 'disc
if (!isPrivate) {
return t('Encrypted_not_available', { roomType: t(roomType) });
}
+ if (e2eEnforcedForPrivate && encrypted) {
+ return t('Encrypted_enforced_by_workspace_policy');
+ }
</file context>
| return {}; | ||
| } | ||
|
|
||
| if (relayState.startsWith('provider=') && relayState.includes('&loginClient=')) { |
Contributor
There was a problem hiding this comment.
P3: When a provider name starts with provider= and contains &loginClient=, a flow without a login client is corrupted on callback because the decoder treats the raw provider as compound RelayState. Use an unambiguous marker or otherwise distinguish legacy raw RelayState before parsing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/saml/lib/Utils.ts, line 167:
<comment>When a provider name starts with `provider=` and contains `&loginClient=`, a flow without a login client is corrupted on callback because the decoder treats the raw provider as compound RelayState. Use an unambiguous marker or otherwise distinguish legacy raw RelayState before parsing.</comment>
<file context>
@@ -137,6 +147,37 @@ export class SAMLUtils {
+ return {};
+ }
+
+ if (relayState.startsWith('provider=') && relayState.includes('&loginClient=')) {
+ const params = new URLSearchParams(relayState);
+ const provider = params.get('provider') ?? undefined;
</file context>
Contributor
There was a problem hiding this comment.
29 issues found across 1518 files
Not reviewed (too large): apps/meteor/tests/end-to-end/api/chat.ts (~2,612 lines), apps/meteor/tests/end-to-end/api/users.ts (~2,506 lines), packages/i18n/src/locales/de.i18n.json (~2,178 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/lib/statusVisibility/hiddenUsers.ts">
<violation number="1" location="apps/meteor/server/lib/statusVisibility/hiddenUsers.ts:12">
P2: When Spotlight requests only rooms, this helper still awaits a status-visibility service RPC, adding avoidable latency and a service dependency to room-only and anonymous searches. Perform the lookup only when the user-search surface is enabled.</violation>
</file>
<file name="apps/meteor/client/sidebar/RoomList/useSidebarListNavigation.ts">
<violation number="1" location="apps/meteor/client/sidebar/RoomList/useSidebarListNavigation.ts:5">
P2: Only one of the two duplicated copies of useSidebarListNavigation was updated. This file now matches 'rcx-sidebar-v2-collapse-group__bar-button', but the identical hook at apps/meteor/client/views/navigation/sidebar/RoomList/useSidebarListNavigation.ts:6 still checks 'rcx-sidebar-v2-collapse-group__bar', and both are wired into live navigation (the sidebar RoomListWrapper uses this copy, SidepanelListWrapper imports the other). Whichever class name matches the fuselage DOM, at least one surface now has divergent or broken Arrow/Tab handling for collapse groups. Sync the class guard across both copies (and confirm which class fuselage actually emits), or extract the shared hook so they cannot drift again.</violation>
</file>
<file name="apps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesWithData.tsx">
<violation number="1" location="apps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesWithData.tsx:126">
P2: When a workspace overrides these keys in `Custom_Translations` using the previous `%s` placeholders, the warning now displays literal `%s` tokens because this call no longer runs the sprintf postprocessor. Preserve sprintf compatibility alongside the named variables, or migrate existing custom translations.</violation>
</file>
<file name="apps/meteor/server/lib/users/getFullUserData.ts">
<violation number="1" location="apps/meteor/server/lib/users/getFullUserData.ts:151">
P2: If the setting is disabled after the projection is built, this condition skips ID-to-username conversion and returns raw `statusVisibilityDenied` IDs. Remove the field whenever the setting is disabled before returning.
(Based on your team's feedback about keeping preference IDs internal.) .</violation>
</file>
<file name="apps/meteor/app/apps/server/converters/codecs/uploads.ts">
<violation number="1" location="apps/meteor/app/apps/server/converters/codecs/uploads.ts:34">
P2: When an `IUpload` has no `rid`, this callback calls `rooms.convertById(undefined)`, so converting room-less uploads can fail instead of completing without a room. Check `rid` before the lookup and return `undefined` when it is absent.</violation>
</file>
<file name="apps/meteor/client/views/room/ClassificationBanner/lib/engine.ts">
<violation number="1" location="apps/meteor/client/views/room/ClassificationBanner/lib/engine.ts:39">
P2: When a grouped attribute uses an empty `multipleLabel`, this renders a blank banner despite matching values. Fall back to the joined values or require a non-empty `multipleLabel` for grouped attributes.</violation>
</file>
<file name="apps/meteor/client/navbar/NavBarPagesGroup/actions/useEncryptedRoomDescription.ts">
<violation number="1" location="apps/meteor/client/navbar/NavBarPagesGroup/actions/useEncryptedRoomDescription.ts:16">
P3: The new enforced-by-policy branch has no test coverage in useEncryptedRoomDescription.spec.ts. Add a case that sets E2E_Force_Encryption_For_Private_Rooms=true (with E2E_Enable=true) and asserts the "Encrypted_enforced_by_workspace_policy" result for private+encrypted rooms, so the new behavior is locked in.</violation>
</file>
<file name="apps/meteor/app/apps/server/converters/codecs/departments.ts">
<violation number="1" location="apps/meteor/app/apps/server/converters/codecs/departments.ts:29">
P2: When a department has multiple forwarding targets, this codec exposes an array through `IAppsDepartment.departmentsAllowedToForward`, whose declared Apps-Engine type is `string`. Align the Apps-Engine contract and the conversion before returning this value, rather than hiding the mismatch with the cast.</violation>
</file>
<file name="apps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsx">
<violation number="1" location="apps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsx:37">
P2: Replacing `callWithErrorHandling('sendMessage', ...)` with a bare `sdk.rest.post` drops the error toast that `callWithErrorHandling` dispatched via `dispatchToastMessage`. If sending the invite message fails, the user now gets no feedback, only a `console.warn` in the outer catch. Keep the user-facing error handling (e.g., wrap the REST call the same way `groups.create` errors are surfaced, or use a toast helper) so a send failure is visible.</violation>
</file>
<file name="apps/meteor/client/components/GenericNoResults/GenericNoResults.tsx">
<violation number="1" location="apps/meteor/client/components/GenericNoResults/GenericNoResults.tsx:27">
P2: The aria-label is hardcoded to `t('No_results_found')`, but many callers pass a custom `title` (e.g. `GenericNoResults title={t('ABAC_No_attributes')}`, `title={t('No_message_reports')}`). A screen reader will then announce "No results found" while the visible text reads the custom title, producing misleading output. Mirror the visible label: `aria-label={title || t('No_results_found')}`.</violation>
</file>
<file name="apps/meteor/server/lib/saml/lib/ServiceProvider.ts">
<violation number="1" location="apps/meteor/server/lib/saml/lib/ServiceProvider.ts:146">
P2: When a provider name is long enough, the new compound RelayState exceeds SAML HTTP-Redirect’s 80-byte limit and can make authorize requests fail. Keep the RelayState within the binding limit, or fall back to the legacy provider-only state or server-side state for long provider names.</violation>
</file>
<file name="apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts">
<violation number="1" location="apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts:37">
P2: When a loaded reply points to a parent older than the oldest loaded message, this bound omits the parent deletion, so the reply keeps a stale `tmid` after reconnect. Reconcile loaded replies' thread parents separately or adjust the sync query to include those parent deletions.</violation>
</file>
<file name="apps/meteor/client/views/account/security/TwoFactorTOTP.tsx">
<violation number="1" location="apps/meteor/client/views/account/security/TwoFactorTOTP.tsx:20">
P2: When either REST call rejects for an invalid TOTP code, `isInvalidTotpError` receives a `Response`, not the decoded error body, so the user gets a generic response error instead of `Invalid_two_factor_code`. Parse the rejected response body or use the shared REST error conversion before checking the error code.</violation>
</file>
<file name="apps/meteor/server/lib/omnichannel/business-hour/filterBusinessHoursThatMustBeOpened.ts">
<violation number="1" location="apps/meteor/server/lib/omnichannel/business-hour/filterBusinessHoursThatMustBeOpened.ts:51">
P2: During the finish minute, this predicate reports the business hour as open while the existing close cron removes it at the minute’s start. New agents or a business-hour save during that minute can therefore be assigned or reopened inconsistently with existing agents; move the close operation to after the finish minute or otherwise align the close path with this inclusive boundary.</violation>
</file>
<file name="apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx">
<violation number="1" location="apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx:137">
P2: When the room renders before its subscription is populated, this expression passes `undefined` for the thread draft and `useDraft` permanently captures that initial value. Load the server draft when it becomes available, or defer composer initialization until the subscription data is ready.</violation>
</file>
<file name="apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptsModal.tsx">
<violation number="1" location="apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptsModal.tsx:26">
P2: After switching to `useEndpoint`, the existing ReadReceiptsModal stories still mock only the removed DDP method, so Storybook and composed tests hit the mock provider's unimplemented endpoint and never render the receipt fixtures. Replace the story decorators' `withMethod` stubs with `withEndpoint('GET', '/v1/chat.getMessageReadReceipts', ...)` responses matching the REST `{ receipts }` shape.</violation>
</file>
<file name="apps/meteor/app/apps/server/converters/codecs/enums.ts">
<violation number="1" location="apps/meteor/app/apps/server/converters/codecs/enums.ts:26">
P2: Non-string enum values now pass the codec boundary because these schemas are `z.any()`. `UserTypeCodec` and `UserStatusConnectionCodec` can call `.toUpperCase()` on such values, while room/setting values can propagate unchanged; restore the original `z.string()`/`z.string().optional()` schemas for all four codecs.</violation>
</file>
<file name="apps/meteor/app/apps/server/converters/uploads.ts">
<violation number="1" location="apps/meteor/app/apps/server/converters/uploads.ts:33">
P2: When an upload has no `rid`, `convertToApp` now invokes the room converter with `undefined`. Preserve the previous missing-`rid` guard in the upload codec so roomless uploads convert without an invalid room lookup.</violation>
</file>
<file name="apps/meteor/client/lib/userPresence.ts">
<violation number="1" location="apps/meteor/client/lib/userPresence.ts:164">
P2: When a browser user was already marked idle, disabling auto-away leaves `idle` true and force-sends Away on the preference update or next reconnect. Clear browser idle state when auto-away is disabled, or gate `reassertPresence` on auto-away being enabled.</violation>
</file>
<file name="apps/meteor/client/views/room/composer/messageBox/hooks/useDraft.ts">
<violation number="1" location="apps/meteor/client/views/room/composer/messageBox/hooks/useDraft.ts:54">
P2: When the same composer is reopened or edited before a draft-save request resolves, the request's success handler deletes the newer local draft. Clear the key only if it still contains the value that this request saved.</violation>
</file>
<file name="apps/meteor/client/lib/utils/threadMessageUtils.ts">
<violation number="1" location="apps/meteor/client/lib/utils/threadMessageUtils.ts:90">
P2: When older thread pages are loaded, adding or deleting a message in a newer page leaves `pageParams` stale. `getPreviousPageParam` then requests the wrong offset, causing the next pagination fetch to skip a message or return a duplicate; rebase page parameters when the mutation changes preceding-page counts, or invalidate/refetch the pagination state.</violation>
</file>
<file name="apps/meteor/client/views/room/body/hooks/useIsAtBottomRef.ts">
<violation number="1" location="apps/meteor/client/views/room/body/hooks/useIsAtBottomRef.ts:8">
P2: useRef only applies its initial value on the first render, so the seed from RoomManager.getStore(rid)?.atBottom is never re-read when the rid changes on a later render. RoomBody is memoized and stays mounted across room switches (the inner Box/scrollbars are keyed by room._id to force per-room remounts), so opening a room that was previously left scrolled mid-history does not restore its stored atBottom — the ref keeps the value from the first room opened. Recompute the seed when rid changes, e.g. store the persisted value in state or set ref.current when rid differs.</violation>
</file>
<file name="apps/meteor/server/api/lib/queryFiltersStatus.ts">
<violation number="1" location="apps/meteor/server/api/lib/queryFiltersStatus.ts:10">
P2: When a users.list query filters on a field that merely starts with 'status' but is not a presence/redacted status field (e.g. statusLivechat for livechat availability, or a user custom field named like 'statusNote'), queryFiltersStatus returns true and users.ts:723 then excludes hidden users even though the filter has nothing to do with their private status. Consider matching an explicit set of status fields (status, statusText, statusConnection, statusDefault, statusExpiresAt, statusSource) instead of the 'status' prefix, or restricting the check so it doesn't apply to non-presence fields.</violation>
</file>
<file name="apps/meteor/ee/server/api/mcp/server.ts">
<violation number="1" location="apps/meteor/ee/server/api/mcp/server.ts:107">
P2: When a client cancels an in-flight `tools/call`, this branch drops the notification without aborting the corresponding `dispatchTool`; long-running or mutating REST calls continue. Track active request IDs and propagate cancellation to the loopback fetch.</violation>
</file>
<file name="apps/meteor/server/lib/2fa/functions/totp.ts">
<violation number="1" location="apps/meteor/server/lib/2fa/functions/totp.ts:21">
P1: When an account already has another 2FA method, a hijacked session can still call deprecated `2fa:enable` followed by `2fa:validateTempToken` to enroll TOTP without verifying the existing factor. Enforce the verification gate for the legacy enrollment callers too, or move it into a shared authenticated boundary before the DDP methods are removed.</violation>
<violation number="2" location="apps/meteor/server/lib/2fa/functions/totp.ts:139">
P2: After regenerating backup codes, the active security view continues displaying the old `codesRemaining` value because this write provides no new count or client update. Return the new remaining count or update the client state after regeneration.</violation>
</file>
<file name="apps/meteor/server/lib/saml/lib/Utils.ts">
<violation number="1" location="apps/meteor/server/lib/saml/lib/Utils.ts:167">
P3: When a provider name starts with `provider=` and contains `&loginClient=`, a flow without a login client is corrupted on callback because the decoder treats the raw provider as compound RelayState. Use an unambiguous marker or otherwise distinguish legacy raw RelayState before parsing.</violation>
</file>
<file name=".yarnrc.yml">
<violation number="1" location=".yarnrc.yml:2">
P2: `approvedGitRepositories: "**"` allows Yarn to fetch git dependencies from *any* repository. The default allowlist is empty, which blocks all git deps; this change disables that gate entirely. Since `enableScripts: true` is also enabled, a malicious git dependency can run arbitrary install scripts on every developer/CI machine. Scope the allowlist to the actual hosts used by the project instead of the `**` wildcard.</violation>
<violation number="2" location=".yarnrc.yml:16">
P2: `npmMinimalAgeGate: 0` disables Yarn's supply-chain protection that refuses to install npm packages published within the last day (default `1d`, introduced in Yarn 4.12). This reduces the attack surface by letting recently published, potentially compromised or already-republished packages be reviewed before installation. Keeping it at `1d` (or an explicit age) preserves that protection; use `yarn add --no-time-gate` for the rare legitimate case instead of disabling it globally.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 400 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
Re-trigger cubic
| return user; | ||
| }; | ||
|
|
||
| export const enableTotp = async (userId: string | null): Promise<{ secret: string; url: string }> => { |
Contributor
There was a problem hiding this comment.
P1: When an account already has another 2FA method, a hijacked session can still call deprecated 2fa:enable followed by 2fa:validateTempToken to enroll TOTP without verifying the existing factor. Enforce the verification gate for the legacy enrollment callers too, or move it into a shared authenticated boundary before the DDP methods are removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/2fa/functions/totp.ts, line 21:
<comment>When an account already has another 2FA method, a hijacked session can still call deprecated `2fa:enable` followed by `2fa:validateTempToken` to enroll TOTP without verifying the existing factor. Enforce the verification gate for the legacy enrollment callers too, or move it into a shared authenticated boundary before the DDP methods are removed.</comment>
<file context>
@@ -0,0 +1,154 @@
+ return user;
+};
+
+export const enableTotp = async (userId: string | null): Promise<{ secret: string; url: string }> => {
+ const user = await requireUser(userId);
+
</file context>
| return undefined; | ||
| } | ||
|
|
||
| const hidden = await StatusVisibility.getHiddenFrom(viewerId); |
Contributor
There was a problem hiding this comment.
P2: When Spotlight requests only rooms, this helper still awaits a status-visibility service RPC, adding avoidable latency and a service dependency to room-only and anonymous searches. Perform the lookup only when the user-search surface is enabled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/statusVisibility/hiddenUsers.ts, line 12:
<comment>When Spotlight requests only rooms, this helper still awaits a status-visibility service RPC, adding avoidable latency and a service dependency to room-only and anonymous searches. Perform the lookup only when the user-search surface is enabled.</comment>
<file context>
@@ -0,0 +1,21 @@
+ return undefined;
+ }
+
+ const hidden = await StatusVisibility.getHiddenFrom(viewerId);
+
+ return hidden.length ? new Set(hidden) : undefined;
</file context>
|
|
||
| const isListItem = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-item'); | ||
| const isCollapseGroup = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-collapse-group__bar'); | ||
| const isCollapseGroup = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-collapse-group__bar-button'); |
Contributor
There was a problem hiding this comment.
P2: Only one of the two duplicated copies of useSidebarListNavigation was updated. This file now matches 'rcx-sidebar-v2-collapse-group__bar-button', but the identical hook at apps/meteor/client/views/navigation/sidebar/RoomList/useSidebarListNavigation.ts:6 still checks 'rcx-sidebar-v2-collapse-group__bar', and both are wired into live navigation (the sidebar RoomListWrapper uses this copy, SidepanelListWrapper imports the other). Whichever class name matches the fuselage DOM, at least one surface now has divergent or broken Arrow/Tab handling for collapse groups. Sync the class guard across both copies (and confirm which class fuselage actually emits), or extract the shared hook so they cannot drift again.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/sidebar/RoomList/useSidebarListNavigation.ts, line 5:
<comment>Only one of the two duplicated copies of useSidebarListNavigation was updated. This file now matches 'rcx-sidebar-v2-collapse-group__bar-button', but the identical hook at apps/meteor/client/views/navigation/sidebar/RoomList/useSidebarListNavigation.ts:6 still checks 'rcx-sidebar-v2-collapse-group__bar', and both are wired into live navigation (the sidebar RoomListWrapper uses this copy, SidepanelListWrapper imports the other). Whichever class name matches the fuselage DOM, at least one surface now has divergent or broken Arrow/Tab handling for collapse groups. Sync the class guard across both copies (and confirm which class fuselage actually emits), or extract the shared hook so they cannot drift again.</comment>
<file context>
@@ -2,7 +2,7 @@ import { useFocusManager } from '@react-aria/focus';
const isListItem = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-item');
-const isCollapseGroup = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-collapse-group__bar');
+const isCollapseGroup = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-collapse-group__bar-button');
const isListItemMenu = (node: EventTarget) => (node as HTMLElement).classList.contains('rcx-sidebar-v2-item__menu');
</file context>
| sprintf: [users.map((element) => element).join(', ')], | ||
| })}` | ||
| : ''; | ||
| const ifFrom = users.length ? ` ${t('if_they_are_from', { users: users.map((element) => element).join(', ') })}` : ''; |
Contributor
There was a problem hiding this comment.
P2: When a workspace overrides these keys in Custom_Translations using the previous %s placeholders, the warning now displays literal %s tokens because this call no longer runs the sprintf postprocessor. Preserve sprintf compatibility alongside the named variables, or migrate existing custom translations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/room/contextualBar/PruneMessages/PruneMessagesWithData.tsx, line 126:
<comment>When a workspace overrides these keys in `Custom_Translations` using the previous `%s` placeholders, the warning now displays literal `%s` tokens because this call no longer runs the sprintf postprocessor. Preserve sprintf compatibility alongside the named variables, or migrate existing custom translations.</comment>
<file context>
@@ -123,19 +123,16 @@ const PruneMessagesWithData = () => {
- sprintf: [users.map((element) => element).join(', ')],
- })}`
- : '';
+ const ifFrom = users.length ? ` ${t('if_they_are_from', { users: users.map((element) => element).join(', ') })}` : '';
const filesOrMessages = attached ? t('files') : t('messages');
</file context>
| return user; | ||
| const ownBlockList = myself ? user.settings?.preferences?.statusVisibilityDenied : undefined; | ||
|
|
||
| if (settings.get<boolean>('Accounts_StatusVisibility_Enabled') && ownBlockList?.length && user.settings?.preferences) { |
Contributor
There was a problem hiding this comment.
P2: If the setting is disabled after the projection is built, this condition skips ID-to-username conversion and returns raw statusVisibilityDenied IDs. Remove the field whenever the setting is disabled before returning.
(Based on your team's feedback about keeping preference IDs internal.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/users/getFullUserData.ts, line 151:
<comment>If the setting is disabled after the projection is built, this condition skips ID-to-username conversion and returns raw `statusVisibilityDenied` IDs. Remove the field whenever the setting is disabled before returning.
(Based on your team's feedback about keeping preference IDs internal.) .</comment>
<file context>
@@ -140,5 +146,13 @@ export async function getFullUserDataByUniqueSearchTerm(
- return user;
+ const ownBlockList = myself ? user.settings?.preferences?.statusVisibilityDenied : undefined;
+
+ if (settings.get<boolean>('Accounts_StatusVisibility_Enabled') && ownBlockList?.length && user.settings?.preferences) {
+ user.settings.preferences.statusVisibilityDenied = (await resolveUsersByIds(ownBlockList)).usernames;
+ }
</file context>
|
|
||
| const { codes, hashedCodes } = TOTP.generateCodes(); | ||
|
|
||
| await Users.update2FABackupCodesByUserId(user._id, hashedCodes); |
Contributor
There was a problem hiding this comment.
P2: After regenerating backup codes, the active security view continues displaying the old codesRemaining value because this write provides no new count or client update. Return the new remaining count or update the client state after regeneration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/2fa/functions/totp.ts, line 139:
<comment>After regenerating backup codes, the active security view continues displaying the old `codesRemaining` value because this write provides no new count or client update. Return the new remaining count or update the client state after regeneration.</comment>
<file context>
@@ -0,0 +1,154 @@
+
+ const { codes, hashedCodes } = TOTP.generateCodes();
+
+ await Users.update2FABackupCodesByUserId(user._id, hashedCodes);
+
+ return { codes };
</file context>
|
|
||
| nodeLinker: node-modules | ||
|
|
||
| npmMinimalAgeGate: 0 |
Contributor
There was a problem hiding this comment.
P2: npmMinimalAgeGate: 0 disables Yarn's supply-chain protection that refuses to install npm packages published within the last day (default 1d, introduced in Yarn 4.12). This reduces the attack surface by letting recently published, potentially compromised or already-republished packages be reviewed before installation. Keeping it at 1d (or an explicit age) preserves that protection; use yarn add --no-time-gate for the rare legitimate case instead of disabling it globally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .yarnrc.yml, line 16:
<comment>`npmMinimalAgeGate: 0` disables Yarn's supply-chain protection that refuses to install npm packages published within the last day (default `1d`, introduced in Yarn 4.12). This reduces the attack surface by letting recently published, potentially compromised or already-republished packages be reviewed before installation. Keeping it at `1d` (or an explicit age) preserves that protection; use `yarn add --no-time-gate` for the rare legitimate case instead of disabling it globally.</comment>
<file context>
@@ -6,10 +9,14 @@ enableGlobalCache: false
+
nodeLinker: node-modules
+npmMinimalAgeGate: 0
+
plugins:
</file context>
Suggested change
| npmMinimalAgeGate: 0 | |
| npmMinimalAgeGate: 1d |
| @@ -1,3 +1,6 @@ | |||
| approvedGitRepositories: | |||
| - "**" | |||
Contributor
There was a problem hiding this comment.
P2: approvedGitRepositories: "**" allows Yarn to fetch git dependencies from any repository. The default allowlist is empty, which blocks all git deps; this change disables that gate entirely. Since enableScripts: true is also enabled, a malicious git dependency can run arbitrary install scripts on every developer/CI machine. Scope the allowlist to the actual hosts used by the project instead of the ** wildcard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .yarnrc.yml, line 2:
<comment>`approvedGitRepositories: "**"` allows Yarn to fetch git dependencies from *any* repository. The default allowlist is empty, which blocks all git deps; this change disables that gate entirely. Since `enableScripts: true` is also enabled, a malicious git dependency can run arbitrary install scripts on every developer/CI machine. Scope the allowlist to the actual hosts used by the project instead of the `**` wildcard.</comment>
<file context>
@@ -1,3 +1,6 @@
+approvedGitRepositories:
+ - "**"
+
checksumBehavior: update
</file context>
Suggested change
| - "**" | |
| - "https://github.com/*" | |
| - "ssh://git@github.com/*" |
| if (!isPrivate) { | ||
| return t('Encrypted_not_available', { roomType: t(roomType) }); | ||
| } | ||
| if (e2eEnforcedForPrivate && encrypted) { |
Contributor
There was a problem hiding this comment.
P3: The new enforced-by-policy branch has no test coverage in useEncryptedRoomDescription.spec.ts. Add a case that sets E2E_Force_Encryption_For_Private_Rooms=true (with E2E_Enable=true) and asserts the "Encrypted_enforced_by_workspace_policy" result for private+encrypted rooms, so the new behavior is locked in.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/navbar/NavBarPagesGroup/actions/useEncryptedRoomDescription.ts, line 16:
<comment>The new enforced-by-policy branch has no test coverage in useEncryptedRoomDescription.spec.ts. Add a case that sets E2E_Force_Encryption_For_Private_Rooms=true (with E2E_Enable=true) and asserts the "Encrypted_enforced_by_workspace_policy" result for private+encrypted rooms, so the new behavior is locked in.</comment>
<file context>
@@ -12,6 +13,9 @@ export const useEncryptedRoomDescription = (roomType: 'channel' | 'team' | 'disc
if (!isPrivate) {
return t('Encrypted_not_available', { roomType: t(roomType) });
}
+ if (e2eEnforcedForPrivate && encrypted) {
+ return t('Encrypted_enforced_by_workspace_policy');
+ }
</file context>
| return {}; | ||
| } | ||
|
|
||
| if (relayState.startsWith('provider=') && relayState.includes('&loginClient=')) { |
Contributor
There was a problem hiding this comment.
P3: When a provider name starts with provider= and contains &loginClient=, a flow without a login client is corrupted on callback because the decoder treats the raw provider as compound RelayState. Use an unambiguous marker or otherwise distinguish legacy raw RelayState before parsing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/saml/lib/Utils.ts, line 167:
<comment>When a provider name starts with `provider=` and contains `&loginClient=`, a flow without a login client is corrupted on callback because the decoder treats the raw provider as compound RelayState. Use an unambiguous marker or otherwise distinguish legacy raw RelayState before parsing.</comment>
<file context>
@@ -137,6 +147,37 @@ export class SAMLUtils {
+ return {};
+ }
+
+ if (relayState.startsWith('provider=') && relayState.includes('&loginClient=')) {
+ const params = new URLSearchParams(relayState);
+ const provider = params.get('provider') ?? undefined;
</file context>
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
20 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.
You can see below a preview of the release change log:
8.8.0
Engine versions
22.22.32.3.18.01.66.0-rc.0Minor Changes
(#41307) Adds classification banners to ABAC-managed rooms: admins can describe US-Government-style classification markings (levels, special access programs, releasability, colors) in a new JSON setting, and matching rooms display a colored classification banner above the room header for all members.
(#41765) Accepts a role name in the
when.hasOneRoleandwhen.hasAllRolesfilters of an app action button(#41474) Changes the default apps-engine runtime backend from
denotonode. The previous behavior can be restored by setting the environment variableAPPS_ENGINE_RUNTIME_BACKEND='deno'(#41788) Adds support for SAML authentication in the mobile and desktop apps via the system browser.
(#41642) Deprecates LDAP and SAML authentication on workspaces without a Premium plan. Both keep working as they are today, but the admin settings now warn that version 9.0.0 will require a license including the
ldap-enterpriseorsaml-enterprisemodule, and a warning is logged when an unlicensed workspace authenticates a user through either of them.(#41095) Adds a workspace setting Force end-to-end encryption on private rooms (
E2E_Force_Encryption_For_Private_Rooms) under Admin → Settings → End-to-End Encryption. When enabled, every newly created private room is encrypted by default and users can no longer opt out: the encryption toggle in the create-room modal is locked on for private rooms, and the server rejects any attempt to create a private room withencrypted: false(e.g. viagroups.create) with the errorerror-encrypted-private-rooms-enforced. Public rooms are unaffected. Federated rooms are exempt since federation does not support E2EE. Creating a discussion under an unencrypted private parent room is rejected with a dedicated error instructing the user to make the parent public or enable encryption on it, and the create-discussion dialog now surfaces creation errors as toasts.(#41355) Replaces the "Drafts in sidebar" feature preview with an always-on draft indicator. Thread-composer drafts are also persisted per thread and indicated in the thread list
(#41082) Adds an AI add-on-gated native Model Context Protocol endpoint and its administration controls in AI Center
(#41747) Adds status visibility, letting users hide their presence and status message from specific people they choose. Blocked people see that user as offline, indistinguishable from genuinely offline, and the block can be lifted at any time — changes apply live, without a reload.
(#40736) Added three new REST endpoints under
/v1/audit.*(EE-only, requires theauditinglicense) covering the audit flows that previously only existed as DDP methods:GET /v1/audit.auditions?startDate=&endDate=→{ auditions: IAuditLog[] }(replacesauditGetAuditions,can-audit-log)POST /v1/audit.messagesbody{ rid?, startDate, endDate, users, msg, type, visitor?, agent? }→{ messages: IMessage[] }(replacesauditGetMessages,can-audit)POST /v1/audit.omnichannelMessagesbody{ startDate, endDate, users, msg, type, visitor?, agent? }→{ messages: IMessage[] }(replacesauditGetOmnichannelMessages,can-audit)Each endpoint is rate-limited at 10 requests / 60s (matching the DDP
DDPRateLimiterrules) and writes the sameAuditLogentry the DDP methods produced. Dates are serialized as ISO strings on the wire. The DDP methods remain registered with deprecation logs pointing at the new routes until 9.0.0.(#41593) Added
POST /v1/chat.readThreadbody{ tmid }, which marks a single thread as read for the caller — clearing the thread from the subscription's unread list and running thebeforeReadMessages/afterReadMessagescallbacks. It replaces thereadThreadsDDP method, which stays registered with a deprecation log pointing at the new route until 9.0.0.POST /v1/subscriptions.readdoes not cover this: it takes{ rid, readThreads? }and operates on the whole room, with no way to address one thread.(#40737) Adds two new REST endpoints completing the Custom OAuth admin surface:
POST /v1/settings.removeCustomOAuthbody{ name }→ removes allAccounts_OAuth_Custom-<Name>-*setting documents (replaces the deprecatedremoveOAuthServiceDDP method).POST /v1/settings.refreshOAuthServices(no body) → re-reads ServiceConfiguration entries from settings (replaces the deprecatedrefreshOAuthServiceDDP method).Both endpoints reuse the
add-oauth-servicepermission andtwoFactorRequiredgates that the DDP methods already enforced.addOAuthServicewas already covered by the existingPOST /v1/settings.addCustomOAuth— its DDP method now also logs a deprecation. The three legacy DDP methods remain registered until 9.0.0.(#40734) Adds five new REST endpoints covering the TOTP 2FA flows that previously only existed as DDP methods:
POST /v1/users.enableTotp→{ secret, url }(replaces2fa:enable)POST /v1/users.disableTotpbody{ code }→{ disabled }(replaces2fa:disable)POST /v1/users.validateTotpbody{ code }→{ codes }(replaces2fa:validateTempToken; also rotates non-PAT login tokens server-side)POST /v1/users.regenerateTotpCodesbody{ code }→{ codes }(replaces2fa:regenerateCodes)GET /v1/users.totpCodesRemaining→{ remaining }(replaces2fa:checkCodesRemaining)users.enableTotpandusers.validateTotprequire two-factor verification (twoFactorRequired) so enrolling a new TOTP device confirms the account owner's identity first — closing a 2FA-enrollment bypass where a hijacked session could register an attacker-controlled TOTP without verifying the existing 2FA. All five endpoints are rate-limited.The legacy DDP methods stay registered with deprecation logs pointing at the new routes until 9.0.0 removes them.
(#41715) Adds an optional
fromTsquery parameter tochat.syncMessages, so it can be used as a replacement for the deprecatedloadMissedMessagesDDP method. It bounds the sync window and must be used together withlastUpdate; sending it with cursor pagination is rejected instead of being ignored.Patch Changes
(#41777) Fixed app action buttons never matching a role scoped to
Subscriptions—owner,moderator,leader, or a custom one. The room was not being passed as the scope of the role check, so a button filtered by one of those roles stayed hidden even for a user who held it in the room.(#41784) Fixed business hours closing for one minute a day when configured as
00:00-23:59. Work hours are set at minute granularity, but the finish time was treated as exclusive, so consecutive daily windows never met and the service dropped to closed for the whole23:59minute — agents could not become available and the Livechat widget rendered the offline form. A work hour now stays open until the end of its finish minute.(#41664) Fixes the composer popups (mentions, slash commands and emojis) staying open after programmatic changes to the composer text, such as canceling the edition of a message that contains a mention.
(#41795) Fixes client-safe errors being reported as exceptions to the channel configured in
Log Exceptions to ChannelwhenLog_Levelis set to2.(#40734) Migrates the
TwoFactorTOTPaccount settings page from the five2fa:*DDP methods to the new TOTP REST endpoints. DDP methods stay registered for external SDK/mobile clients with deprecation logs pointing at the new routes until 9.0.0.(#40736) Migrated the audit panel (
AuditLogTable,useAuditMutation) from the threeauditGet*DDP methods to the new/v1/audit.*REST endpoints. DDP methods stay registered with deprecation logs pointing at the new routes until 9.0.0.(#40737) Migrates the Admin → OAuth services group page from
useMethod(DDP) touseEndpoint(REST):addOAuthService→ existingPOST /v1/settings.addCustomOAuthremoveOAuthService→ newPOST /v1/settings.removeCustomOAuthrefreshOAuthService→ newPOST /v1/settings.refreshOAuthServicesDDP methods stay registered with deprecation logs pointing at the new routes until 9.0.0.
(#41593) Migrated the last two thread-read call sites (
ThreadChat,useThreadMessagesQuery) from thereadThreadsDDP method toPOST /v1/chat.readThread, and pointed the admin "send a test push to my user" setting atPOST /v1/push.testinstead of thepush_testDDP method. Both DDP methods stay registered with deprecation logs pointing at the new routes until 9.0.0.POST /v1/push.testnow also returns themessagetranslation key and itsparams, matching what the DDP method returned, so the admin setting still reports how many devices the test reached.(#41653) Bumps patch-level dependency updates:
react,react-domandreact-isto 19.2.8,wsto 8.21.3,nodemailerto 9.0.5,mailparserto 3.9.15,google-libphonenumberto 3.2.46,twitter-api-v2to 1.29.1,ip-range-checkto 0.2.1,esbuildto 0.28.2,@octokit/coreto 7.0.7 and@octokit/plugin-throttlingto 11.0.5, plus build and test tooling (@swc/core,ts-jest,rollup,html-webpack-plugin,@vitejs/plugin-react,@testing-library/user-event, and the@types/*packages). No behavior changes are expected.(#41876) Fixes room search (
spotlight) failing for anonymous visitors when "Allow Anonymous Read" is enabled(#41481) Adds an Import IdP metadata option to SAML settings that fetches the Identity Provider metadata from a URL and prefills the matching setting fields — certificate, entry point and IDP SLO redirect URL, plus identifier format on Enterprise — for the admin to review before saving.
(#41699) Adds per-client rate limiting to the unauthenticated sendForgotPasswordEmail method, matching the REST users.forgotPassword endpoint
(#41805) Fixes the issue where the message list kept jumping to the latest messages instead of restoring the previous position when switching channels.
(#41707) Fixes thread replies keeping the "sent" single checkmark after everyone had read the thread, only switching to the "viewed" double checkmark once a new message was sent in that thread.
(#41744) Fixes
canAccessRoomthrowing when the user's_idis undefined(#41814) Security Hotfix (https://docs.rocket.chat/docs/security-fixes-and-updates)
(#41749) Replace http with serverFetch in downloadPublicImportFile to add SSRF protection
(#41711) Fixes "Jump to message" not navigating to the message's room when the message belongs to a room other than the currently opened one (e.g. global search results).
(#41717) Fixes federation endpoints rejecting valid requests, which broke:
(#41573) Marks the user session as logged out in the Sessions collection when logging out via
POST /v1/logout. Previously the session cleanup relied on an indirect chain throughwatch.users→Accounts.onLogoutthat could be broken by a race condition, leaving orphaned sessions visible in Device Manager.(#41595) Fixes special characters not being escaped in the visitor name shown in the Omnichannel queue side panel's message preview
(#41683) Fixed the sidebar not scrolling when its content is taller than the viewport — the inner wrapper was pinned to the scroll container's height instead of using it as a minimum, so overflowing content was clipped
(#41755) Fixes
POST /v1/banners.dismissfailing withBanner not foundfor banners stored in the user's record (such as the version update ones), which were never marked as read. The endpoint now marks them as read as the deprecatedbanner/dismissmethod did, and only fails when the banner does not exist in the banners collection nor in the user's record.(#41673) Fixes the messages count displayed on a discussion taking into account system messages which are hidden inside of it, making the count higher than the number of messages actually visible after opening the discussion. The count now excludes every system message type hidden either globally or on the discussion itself. A hint was also added to the
Hide system messagesoption of the room edit panel clarifying that the hidden messages are not included in the count.(#41484) Fixes an issue where deleted thread messages display as empty in thread context
(#41504) Fixed saving a livechat tag failing with "Invalid response" — the model mutated its return value with
_updatedAt, which the endpoint response schema rejects(#41702) Fixes discussion parent message count and last-message timestamp out of sync with the discussion
(#41763) Drops
@rocket.chat/string-helpersin favor of@rocket.chat/tools(#41574) Fixes reaction list modal showing blank entries (mobile) or usernames (web) instead of real names when
UI_Use_Real_Nameis enabled. The broadcast pipeline now enriches reactions with display names via batch query.(#41585) Fixes users being set back to online after a websocket reconnection (connection drop, network change, server restart) even though they had gone idle and never interacted with the UI again. The client now tracks the last UI interaction across connection drops and restates the away status as soon as the reconnected session is authenticated, instead of assuming the new session is online and restarting the idle countdown from scratch.
(#41631) Fixes text rendering without line breaks when its length is higher than the value of the
MESSAGE_MAX_PARSE_LENGTHenvironment variableUpdated dependencies [126e446, b27e38c, e677905, b4557f5, 5a3fd52, 097884f, 4947601, 0869925, 7b7f88f, 18f5eb4, b89a8d4, 9b66ee2, 6dc66fb, 2052886, 5deefe2, 8984df8, 6edcdd7, 7f9592d, a9f3e6c, 742009a]: