feat(apps): ad-hoc redaction for apps logs#40096
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-04-10T21:17:22.932ZApplied to files:
📚 Learning: 2026-02-24T19:09:09.561ZApplied to files:
📚 Learning: 2026-02-25T20:10:16.987ZApplied to files:
🪛 Biome (2.4.11)apps/meteor/ee/server/apps/orchestrator.js[error] 13-13: Illegal use of an import declaration outside of a module (parse) 🔇 Additional comments (2)
WalkthroughAdds targeted redaction and URL censoring across app logging: introduces fast-redact config and applies it before persisting log entries; adds a censorUrl utility and uses it in multiple logging sites; configures the orchestrator logger with redaction paths; updates package deps to include fast-redact. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Logger
participant Redactor
participant Storage
participant DB
Client->>Middleware: incoming request / app event
Middleware->>Logger: emit log (censorUrl applied to URLs)
Logger->>Redactor: redact configured sensitive fields
Redactor->>Storage: forward redacted log entry
Storage->>DB: insertOne(redacted entry)
DB-->>Storage: ack
Storage-->>Logger: optional persistence confirmation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
🦋 Changeset detectedLatest commit: 9f0c0e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 43 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Looks like this PR is ready to merge! 🎉 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40096 +/- ##
========================================
Coverage 69.73% 69.74%
========================================
Files 3290 3291 +1
Lines 119072 119089 +17
Branches 21447 21384 -63
========================================
+ Hits 83032 83053 +21
- Misses 32725 32737 +12
+ Partials 3315 3299 -16
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
force-pushed
the
fix/adhoc-logs-redaction
branch
from
7b99d30 to
fc820c8
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
coderabbitai
bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts (1)
68-70: Consider simplifying the mutation loop for readability.Current nested
forEach+ ternary +voidworks, but a small control-flow rewrite is easier to scan and maintain.♻️ Suggested refactor
- logEntry.entries.forEach((entry) => { - entry.args.forEach((arg) => void (typeof arg === 'object' && arg !== null ? this.redact(arg) : arg)); - }); + for (const entry of logEntry.entries) { + for (const arg of entry.args) { + if (arg && typeof arg === 'object') { + this.redact(arg); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts` around lines 68 - 70, The nested forEach with a ternary and void is hard to read; replace it with simple loops that explicitly check each arg and call this.redact when the arg is a non-null object. Locate the block in AppRealLogStorage where logEntry.entries.forEach(...) is used and rewrite using a clear loop over logEntry.entries and then over each entry.args (e.g., for/of or indexed for) so you do: for each arg in entry.args if (arg !== null && typeof arg === 'object') this.redact(arg); this keeps the same in-place mutation behavior but is much more readable and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts`:
- Around line 14-33: The fastRedact configuration in this.redact only lists
lowercase header/query/param keys so case-sensitive path matching can miss
"Authorization" or "X-Auth-Token" variants; update the paths passed to
fastRedact (where fastRedact is invoked and this.redact is assigned) to include
the common case variants for those keys (e.g., add headers.Authorization,
headers["X-Auth-Token"], query["X-Auth-Token"], params["X-Auth-Token"], and any
capitalized Authorization/query/param variants) or switch to a case-insensitive
redaction strategy supported by fast-redact (e.g., normalize keys before
redaction) so uppercase/capitalized header names cannot bypass redaction.
---
Nitpick comments:
In `@apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts`:
- Around line 68-70: The nested forEach with a ternary and void is hard to read;
replace it with simple loops that explicitly check each arg and call this.redact
when the arg is a non-null object. Locate the block in AppRealLogStorage where
logEntry.entries.forEach(...) is used and rewrite using a clear loop over
logEntry.entries and then over each entry.args (e.g., for/of or indexed for) so
you do: for each arg in entry.args if (arg !== null && typeof arg === 'object')
this.redact(arg); this keeps the same in-place mutation behavior but is much
more readable and maintainable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f02f1a7-c1b1-41de-a29d-c4c5bcb0112f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
apps/meteor/ee/server/apps/storage/AppRealLogStorage.tsapps/meteor/package.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/package.json
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/package.json
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts
🔇 Additional comments (1)
apps/meteor/package.json (1)
206-206: Dependency additions look consistent with the feature scope.
fast-redact(runtime) and@types/fast-redact(dev) match the new server-side redaction implementation and typing usage.Also applies to: 356-356
d-gubert
force-pushed
the
fix/adhoc-logs-redaction
branch
3 times, most recently
from
e916389 to
8153d62
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/tools/src/censorUrl.ts (1)
33-39: Consider redacting additional common sensitive query parameters.The function currently redacts
queryandaccess_token. Other common sensitive query parameters liketoken,auth_token,api_key,apikey,secret,password, andkeyare often used to pass credentials in URLs. If any of these appear in app request URLs, they would be logged in plain text.🔧 Suggested enhancement
+const sensitiveParams = ['query', 'access_token', 'token', 'auth_token', 'api_key', 'apikey', 'key', 'secret', 'password']; + export function censorUrl(url: string | URL): string { try { const parsedUrl = new URL(url); if (parsedUrl.username) { parsedUrl.username = '*Redacted*'; } if (parsedUrl.password) { parsedUrl.password = '*Redacted*'; } - if (parsedUrl.searchParams.has('query')) { - parsedUrl.searchParams.set('query', '*Redacted*'); - } - - if (parsedUrl.searchParams.has('access_token')) { - parsedUrl.searchParams.set('access_token', '*Redacted*'); - } + for (const param of sensitiveParams) { + if (parsedUrl.searchParams.has(param)) { + parsedUrl.searchParams.set(param, '*Redacted*'); + } + } return parsedUrl.toString(); } catch { return url.toString(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tools/src/censorUrl.ts` around lines 33 - 39, Replace the two hard-coded conditionals in censorUrl that only redact 'query' and 'access_token' with a loop over a whitelist of sensitive parameter names (e.g., ['query','access_token','token','auth_token','api_key','apikey','secret','password','key']) and call parsedUrl.searchParams.set(name, '*Redacted*') for any present parameter; update the logic in the censorUrl function (where parsedUrl and parsedUrl.searchParams are used) to iterate the sensitive-params array and redact matches (optionally matching case-insensitively by comparing lowercased parameter names).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/ee/server/apps/lib/redactor.ts`:
- Around line 3-17: Add the missing X-User-Id header variants to the sensitive
requestFields array in redactor.ts by inserting 'headers["x-user-id"]' and
'headers["X-User-Id"]' alongside the existing header entries (follow the same
pattern as 'headers["x-auth-token"]' and 'headers["X-Auth-Token"]'). Update the
requestFields constant so the redactor will redact both lowercase and
capitalized X-User-Id headers.
---
Nitpick comments:
In `@packages/tools/src/censorUrl.ts`:
- Around line 33-39: Replace the two hard-coded conditionals in censorUrl that
only redact 'query' and 'access_token' with a loop over a whitelist of sensitive
parameter names (e.g.,
['query','access_token','token','auth_token','api_key','apikey','secret','password','key'])
and call parsedUrl.searchParams.set(name, '*Redacted*') for any present
parameter; update the logic in the censorUrl function (where parsedUrl and
parsedUrl.searchParams are used) to iterate the sensitive-params array and
redact matches (optionally matching case-insensitively by comparing lowercased
parameter names).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6cb3b5a-72a5-4642-9818-6fa79f92e06a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
apps/meteor/app/api/server/middlewares/logger.tsapps/meteor/app/apps/server/bridges/http.tsapps/meteor/app/apps/server/bridges/persistence.tsapps/meteor/ee/server/apps/lib/redactor.tsapps/meteor/ee/server/apps/orchestrator.jsapps/meteor/ee/server/apps/storage/AppRealLogStorage.tsapps/meteor/package.jsonpackages/server-fetch/src/index.tspackages/tools/src/censorUrl.tspackages/tools/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- packages/tools/src/index.ts
- apps/meteor/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 📦 Meteor Build (coverage)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/apps/server/bridges/http.tsapps/meteor/app/api/server/middlewares/logger.tsapps/meteor/ee/server/apps/orchestrator.jspackages/server-fetch/src/index.tsapps/meteor/app/apps/server/bridges/persistence.tspackages/tools/src/censorUrl.tsapps/meteor/ee/server/apps/lib/redactor.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-03-11T16:46:55.955Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249
Timestamp: 2026-03-11T16:46:55.955Z
Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.
Applied to files:
apps/meteor/app/apps/server/bridges/http.tsapps/meteor/app/apps/server/bridges/persistence.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/apps/server/bridges/http.tsapps/meteor/app/api/server/middlewares/logger.tspackages/server-fetch/src/index.tsapps/meteor/app/apps/server/bridges/persistence.tspackages/tools/src/censorUrl.tsapps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/apps/server/bridges/http.tsapps/meteor/app/api/server/middlewares/logger.tspackages/server-fetch/src/index.tsapps/meteor/app/apps/server/bridges/persistence.tspackages/tools/src/censorUrl.tsapps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/app/api/server/middlewares/logger.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/api/server/middlewares/logger.tsapps/meteor/ee/server/apps/orchestrator.jsapps/meteor/app/apps/server/bridges/persistence.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/ee/server/apps/orchestrator.js
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/ee/server/apps/orchestrator.js
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/apps/server/bridges/persistence.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/apps/server/bridges/persistence.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/apps/server/bridges/persistence.ts
🪛 Biome (2.4.10)
apps/meteor/ee/server/apps/orchestrator.js
[error] 12-12: Illegal use of an import declaration outside of a module
(parse)
[error] 31-31: Illegal use of an import declaration outside of a module
(parse)
🔇 Additional comments (7)
apps/meteor/app/apps/server/bridges/persistence.ts (1)
18-18: LGTM! Structured logging changes enable redaction.The refactoring to use object-based logging (
{ msg, associations }) for methods with sensitive association data aligns well with the PR's redaction architecture. Methods without associations correctly continue using simple string messages. Both patterns are supported by thedebugLog(...args: any[])interface.Also applies to: 31-34, 55-55, 86-86, 107-107, 122-122
apps/meteor/ee/server/apps/lib/redactor.ts (1)
37-42: LGTM! The fast-redact configuration is appropriate.Using
serialize: falseallows the redactor to mutate objects in-place without JSON stringification, andstrict: falseprevents errors when paths don't exist in the target object. These settings are suitable for logging scenarios where objects may have variable shapes.apps/meteor/app/apps/server/bridges/http.ts (1)
76-76: LGTM! URL censoring applied correctly.The URL is censored before logging while spreading the rest of
infointo the log object. The sensitive headers and auth data withininfo.requestwill be handled by the logger's redaction configuration defined inredactionFieldPaths.apps/meteor/app/api/server/middlewares/logger.ts (1)
15-15: LGTM! URL censoring applied to request logging.The middleware now properly censors request URLs before logging, preventing sensitive query parameters and credentials from appearing in logs.
packages/server-fetch/src/index.ts (2)
25-25: LGTM! Simplified HTTPS check.The change from
/^https/.test(url)tourl.startsWith('https')is functionally equivalent and slightly more readable. The upstream SSRF validation (incheckForSsrf.ts) ensures only validhttp:orhttps:URLs reach this function, so edge cases are already handled.
71-71: LGTM! Consistent URL censoring across logging paths.URL censoring is correctly applied to:
- SSRF validation failure logs (Line 71)
- Debug logs for requests skipping SSRF validation (Line 86)
- Redirect following logs (Line 112)
This prevents sensitive URL components from being exposed in server logs.
Also applies to: 86-86, 112-112
apps/meteor/ee/server/apps/orchestrator.js (1)
48-48: No action needed. Pino's child logger has supported theredactoption since PR#1059(2021), allowing child-specific redaction configuration that overrides the parent logger's settings. The code on line 48 correctly passes redaction paths to the child logger.> Likely an incorrect or invalid review comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 11 files
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="packages/tools/src/censorUrl.ts">
<violation number="1" location="packages/tools/src/censorUrl.ts:23">
P1: Relative URL strings are not redacted: `new URL(url)` throws without a base, then the catch path returns the original sensitive URL unchanged.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/ee/server/apps/lib/redactor.ts (2)
19-35: Consider narrowingcustomFieldsredaction scope.Using
customFields.*masks all custom fields, which may remove useful non-sensitive diagnostics. If CORE-1995 scope is intended, prefercustomFields.fodselsnummer-specific paths.Proposed narrowing
-const entityFields = ['password', 'pass', 'customFields.*', '_unmappedProperties_']; +const entityFields = ['password', 'pass', 'customFields.fodselsnummer', '_unmappedProperties_'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/apps/lib/redactor.ts` around lines 19 - 35, The redaction currently uses a broad customFields.* entry in entityFields (used by entityFields, roomFields, and redactionFieldPaths) which masks all custom fields; change entityFields to remove or replace 'customFields.*' with the specific field(s) required by CORE-1995 (e.g., 'customFields.fodselsnummer') so only those sensitive custom field paths are redacted; update roomFields and redactionFieldPaths will automatically reflect the narrower scope because they derive from entityFields and ensure you leave other entries like '_unmappedProperties_' intact.
24-33: Remove inline implementation comments in this module.These comments should be moved to external docs/PR context to keep implementation code clean per repo standards.
As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/apps/lib/redactor.ts` around lines 24 - 33, This module contains inline implementation comments next to the array entries (e.g., the comments around 'query.query' and `request.query`)—remove those inline comments from the code in redactor.ts and relocate any needed explanations to external docs or the PR description; specifically edit the array construction that uses requestFields, entityFields, roomFields and the literal entries 'query.query' and 'request.query' to strip out the explanatory comments while keeping the same string entries and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/ee/server/apps/lib/redactor.ts`:
- Around line 19-35: The redaction currently uses a broad customFields.* entry
in entityFields (used by entityFields, roomFields, and redactionFieldPaths)
which masks all custom fields; change entityFields to remove or replace
'customFields.*' with the specific field(s) required by CORE-1995 (e.g.,
'customFields.fodselsnummer') so only those sensitive custom field paths are
redacted; update roomFields and redactionFieldPaths will automatically reflect
the narrower scope because they derive from entityFields and ensure you leave
other entries like '_unmappedProperties_' intact.
- Around line 24-33: This module contains inline implementation comments next to
the array entries (e.g., the comments around 'query.query' and
`request.query`)—remove those inline comments from the code in redactor.ts and
relocate any needed explanations to external docs or the PR description;
specifically edit the array construction that uses requestFields, entityFields,
roomFields and the literal entries 'query.query' and 'request.query' to strip
out the explanatory comments while keeping the same string entries and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3781759-6be8-472d-9599-b1a0c7a63eab
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
apps/meteor/app/api/server/middlewares/logger.tsapps/meteor/app/apps/server/bridges/http.tsapps/meteor/app/apps/server/bridges/persistence.tsapps/meteor/ee/server/apps/lib/redactor.tsapps/meteor/ee/server/apps/orchestrator.jsapps/meteor/ee/server/apps/storage/AppRealLogStorage.tsapps/meteor/package.jsonpackages/server-fetch/src/index.tspackages/tools/src/censorUrl.tspackages/tools/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- apps/meteor/package.json
- packages/tools/src/censorUrl.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/tools/src/index.tsapps/meteor/app/apps/server/bridges/http.tsapps/meteor/app/api/server/middlewares/logger.tsapps/meteor/ee/server/apps/orchestrator.jspackages/server-fetch/src/index.tsapps/meteor/app/apps/server/bridges/persistence.tsapps/meteor/ee/server/apps/lib/redactor.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40096
File: apps/meteor/ee/server/apps/lib/redactor.ts:3-17
Timestamp: 2026-04-10T21:17:19.306Z
Learning: In RocketChat/Rocket.Chat, `X-User-Id` / `x-user-id` headers must NOT be added to redaction paths in apps log redaction (e.g., `apps/meteor/ee/server/apps/lib/redactor.ts`). The maintainer (d-gubert) has confirmed that X-User-Id is an identifier, not a credential — its presence in logs is useful for diagnostics, and `X-Auth-Token` is the only header that constitutes a real secret. Do not suggest redacting X-User-Id in future reviews of this area.
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/tools/src/index.tsapps/meteor/app/apps/server/bridges/http.tsapps/meteor/app/api/server/middlewares/logger.tspackages/server-fetch/src/index.tsapps/meteor/app/apps/server/bridges/persistence.tsapps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/tools/src/index.tsapps/meteor/app/apps/server/bridges/http.tsapps/meteor/app/api/server/middlewares/logger.tspackages/server-fetch/src/index.tsapps/meteor/app/apps/server/bridges/persistence.tsapps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-03-11T16:46:55.955Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249
Timestamp: 2026-03-11T16:46:55.955Z
Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.
Applied to files:
apps/meteor/app/apps/server/bridges/http.tsapps/meteor/app/apps/server/bridges/persistence.tsapps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/ee/server/apps/orchestrator.jsapps/meteor/app/apps/server/bridges/persistence.tsapps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-04-10T21:17:19.306Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40096
File: apps/meteor/ee/server/apps/lib/redactor.ts:3-17
Timestamp: 2026-04-10T21:17:19.306Z
Learning: In RocketChat/Rocket.Chat, `X-User-Id` / `x-user-id` headers must NOT be added to redaction paths in apps log redaction (e.g., `apps/meteor/ee/server/apps/lib/redactor.ts`). The maintainer (d-gubert) has confirmed that X-User-Id is an identifier, not a credential — its presence in logs is useful for diagnostics, and `X-Auth-Token` is the only header that constitutes a real secret. Do not suggest redacting X-User-Id in future reviews of this area.
Applied to files:
apps/meteor/ee/server/apps/orchestrator.jsapps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/ee/server/apps/orchestrator.jsapps/meteor/app/apps/server/bridges/persistence.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/apps/server/bridges/persistence.tsapps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/apps/server/bridges/persistence.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/apps/server/bridges/persistence.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/ee/server/apps/lib/redactor.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/ee/server/apps/lib/redactor.ts
🪛 Biome (2.4.10)
apps/meteor/ee/server/apps/orchestrator.js
[error] 12-12: Illegal use of an import declaration outside of a module
(parse)
[error] 31-31: Illegal use of an import declaration outside of a module
(parse)
🔇 Additional comments (6)
packages/tools/src/index.ts (1)
1-1: Barrel export update looks good.Exposing
censorUrlfrom the package entrypoint is consistent with the new shared redaction usage.apps/meteor/ee/server/apps/orchestrator.js (1)
12-12: Logger redaction wiring is solid.Switching to a child logger with
redact: redactionFieldPathsis a good integration point for centralized sanitization.Also applies to: 31-31, 48-48
apps/meteor/app/apps/server/bridges/http.ts (1)
6-6: Good move to structured + censored URL logging.This preserves request context while avoiding raw URL leakage in app bridge debug logs.
Also applies to: 76-76
apps/meteor/app/api/server/middlewares/logger.ts (1)
2-2: URL censoring in middleware context is correct.This is a clean, minimal hardening change with no behavior drift.
Also applies to: 15-15
packages/server-fetch/src/index.ts (1)
5-5: Redacted URL logging coverage here is strong.Applying
censorUrlat SSRF/error/debug/redirect points closes common leak paths without changing request flow.Also applies to: 71-71, 86-86, 112-112
apps/meteor/app/apps/server/bridges/persistence.ts (1)
18-18: Nice hardening of persistence debug logs.Dropping raw
datafrom debug output and keeping structured metadata is the right trade-off for safety and debuggability.Also applies to: 31-35, 55-55, 86-86, 107-107, 122-122
Member
Author
|
@copilot write a unit test file that covers packages/tools/src/censorUrl.ts. Follow the patterns found in the file's directory. Be comprehensive in the test cases, make sure to cover all logical branches. |
Contributor
Implemented in d0de2e7. Added Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
d-gubert
commented
Apr 12, 2026
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
ggazzo
previously approved these changes
Apr 15, 2026
sampaiodiego
requested changes
Apr 17, 2026
| } | ||
|
|
||
| this._rocketchatLogger = new Logger('Rocket.Chat Apps'); | ||
| this._rocketchatLogger = SystemLogger.logger.child({ name: 'Rocket.Chat Apps' }, { redact: redactionFieldPaths }); |
Member
There was a problem hiding this comment.
add redact to Logger class
refactor: improve paths on redactor
Agent-Logs-Url: https://github.com/RocketChat/Rocket.Chat/sessions/39bac93f-3599-46eb-802a-702b5a9102d5 Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
Agent-Logs-Url: https://github.com/RocketChat/Rocket.Chat/sessions/225a35e7-c779-467b-9093-192a9bddc17b Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
Co-authored-by: Diego Sampaio <chinello@gmail.com>
d-gubert
force-pushed
the
fix/adhoc-logs-redaction
branch
from
02dee9b to
9f0c0e5
Compare
ggazzo
approved these changes
Apr 17, 2026
sampaiodiego
approved these changes
Apr 17, 2026
d-gubert
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Open
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Redaction for apps logs
Issue(s)
CORE-1995
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Chores
Tests