chore(api): migrate channels.ts to typed HTTP methods - #41415
Merged
Conversation
WIP migrating channels.ts from addRoute to typed API.v1 (moderation pattern — kept manual ChannelsEndpoints entry, no augment). Batch 1: addAll, archive, unarchive, join, kick, leave. - Shared channelResponseSchema ($ref IRoom) + successResponseSchema. - findChannelByIdOrName throws Meteor.Error; the typed router does not map throws to 400, so each handler catches and returns API.v1.failure(msg, errorType) to preserve the previous behavior.
|
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 |
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:
WalkthroughChannel management endpoints are migrated from legacy routes to typed ChangesChannel API route migration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41415 +/- ##
===========================================
+ Coverage 68.48% 68.71% +0.23%
===========================================
Files 4092 4151 +59
Lines 158216 159516 +1300
Branches 28678 28203 -475
===========================================
+ Hits 108351 109612 +1261
+ Misses 44827 44731 -96
- Partials 5038 5173 +135
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Errors from core-services (e.g. Room.join) cross a service boundary and are not instanceof the local Meteor.Error, so the catch dropped their errorType and the channels.join e2e assertions (`expected ... to have property 'errorType'`) failed. Extract message/errorType by shape instead of instanceof, matching the legacy addRoute behavior.
Member
Author
|
/jira ARCH-1464 |
KevLehman
previously approved these changes
Jul 16, 2026
KevLehman
left a comment
KevLehman
left a comment
Member
There was a problem hiding this comment.
didn't know we dropped the "global" try/catch behavior for the api routes.
![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/server/api/v1/channels.ts (1)
158-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a higher-order function to DRY up repetitive error handling.
Since this migration is incremental and many more
channels.*andgroups.*endpoints are planned for subsequent batches, wrapping handlers in a higher-order function can eliminate the identicaltry/catchboilerplate across all endpoints.💡 Example wrapper implementation
function withErrorHandler<T>(action: (this: T) => Promise<any>) { return async function(this: T) { try { return await action.call(this); } catch (error) { const [message, errorType] = errorToFailureArgs(error); return API.v1.failure(message, errorType); } }; } // Usage example: API.v1.post( 'channels.addAll', { /* ... */ }, withErrorHandler(async function() { // Handler logic without try/catch }) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/api/v1/channels.ts` around lines 158 - 161, Introduce a reusable higher-order error-handling wrapper near the affected channel endpoint handlers, using the existing errorToFailureArgs and API.v1.failure behavior. Apply it to the handlers currently containing identical try/catch blocks, including the affected channels.* endpoints, and remove their inline error-handling boilerplate while preserving handler this-context and responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/server/api/v1/channels.ts`:
- Around line 132-135: Update errorToFailureArgs to read the Meteor.Error reason
before message: include an optional reason field in the extracted error shape
and use it when it is a string, falling back to the existing
message/String(error) behavior otherwise. Preserve the current extraction of the
string error code for the second tuple value.
---
Nitpick comments:
In `@apps/meteor/server/api/v1/channels.ts`:
- Around line 158-161: Introduce a reusable higher-order error-handling wrapper
near the affected channel endpoint handlers, using the existing
errorToFailureArgs and API.v1.failure behavior. Apply it to the handlers
currently containing identical try/catch blocks, including the affected
channels.* endpoints, and remove their inline error-handling boilerplate while
preserving handler this-context and responses.
🪄 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: 75387af3-ccf1-4e18-82ad-b7b2bd692be0
📒 Files selected for processing (1)
apps/meteor/server/api/v1/channels.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- 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 (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/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:
apps/meteor/server/api/v1/channels.ts
🧠 Learnings (3)
📚 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/server/api/v1/channels.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/server/api/v1/channels.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/api/v1/channels.ts
github-project-automation
Bot
moved this to Backlog
in rest api conversion to new endpoints
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The typed router has no global error handler, so thrown errors (incl. Meteor.Error) become 500 — handlers must catch client-error throws and return API.v1.failure. core-services errors are not instanceof the local Meteor.Error, so extract message/errorType by shape.
Migrate rename, setCustomFields, setDefault, setDescription, setPurpose,
setTopic, setType. These addRoute endpoints had no validateParams, so use inline
ajv body validators (roomSettingBody helper: room target + setting field),
matching the inline-validator style used in rooms.ts/users.ts. Responses:
channelResponseSchema ($ref IRoom) or the {field} schema; findChannelByIdOrName
throws are caught → API.v1.failure.
Migrate addModerator, addOwner, removeModerator, removeOwner, addLeader, removeLeader (all void, body: isChannelsModeratorsProps). findChannelByIdOrName throws caught -> API.v1.failure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/server/api/v1/channels.ts (1)
152-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the implementation comment.
The helper is self-explanatory from its name and signature. As per coding guidelines, “Avoid code comments in the implementation.” <coding_guidelines>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/api/v1/channels.ts` at line 152, Remove the implementation comment above the channels.set* body validator helper in channels.ts, leaving the helper and its behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/server/api/v1/channels.ts`:
- Around line 153-163: Update the shared roomSettingBody validator to require
the setting field together with at least one room target, roomId or roomName, so
requests cannot validate without a channel identifier; preserve the existing
field schema and additionalProperties restrictions.
In `@docs/api-endpoint-migration.md`:
- Around line 568-576: Update the action example’s catch block to handle only
known client-signaling errors via errorToFailureArgs and API.v1.failure; rethrow
all unexpected exceptions so they retain server-error handling and avoid
exposing internal messages. Keep the successful findChannelByIdOrName response
unchanged.
---
Nitpick comments:
In `@apps/meteor/server/api/v1/channels.ts`:
- Line 152: Remove the implementation comment above the channels.set* body
validator helper in channels.ts, leaving the helper and its behavior unchanged.
🪄 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: 112c3443-1bf2-490a-b272-aa44bbb7566b
📒 Files selected for processing (2)
apps/meteor/server/api/v1/channels.tsdocs/api-endpoint-migration.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: CodeQL-Build
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (2)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 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/server/api/v1/channels.ts
🧠 Learnings (3)
📚 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/server/api/v1/channels.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/server/api/v1/channels.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/api/v1/channels.ts
🔇 Additional comments (2)
docs/api-endpoint-migration.md (1)
561-567: LGTM!Also applies to: 577-594
apps/meteor/server/api/v1/channels.ts (1)
137-150: LGTM!Also applies to: 668-718, 1324-1414, 1416-1447, 1449-1490, 1492-1525, 1527-1560, 1562-1595, 1597-1636, 1638-1688
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/api/v1/channels.ts">
<violation number="1" location="apps/meteor/server/api/v1/channels.ts:1453">
P3: `channels.setDefault` now publishes a string-or-boolean request schema while the exported REST client contract permits only booleans. Align `ChannelsSetDefaultProps` (and its validator schema) with this route, or restrict the route schema to the published contract, so generated and typed consumers agree.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Add an anyOf(roomId, roomName) constraint to the shared roomSettingBody validator so channels.set*/rename bodies must carry a channel identifier, matching the exported REST contracts, and drop the redundant helper comment.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/server/api/v1/channels.ts (2)
152-162: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject requests containing both room identifiers.
anyOfallows bothroomIdandroomName, andfindChannelByIdOrNamewill silently preferroomIdwhen both are sent. UseoneOfif the API should accept exactly one identifier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/api/v1/channels.ts` around lines 152 - 162, Update the roomSettingBody schema to use oneOf instead of anyOf for the roomId and roomName requirements, ensuring requests must provide exactly one identifier and cannot silently prioritize roomId. Keep the existing required field and additionalProperties constraints unchanged.
531-539: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winInclude
success: truein the response generic.
API.v1.success()addssuccessto the body, so the response schema should be typed as{ announcement?: string; success: true }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/api/v1/channels.ts` around lines 531 - 539, Update the response generic passed to ajv.compile for announcementResponseSchema to include success: true alongside the optional announcement field, matching the success property added by API.v1.success().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/meteor/server/api/v1/channels.ts`:
- Around line 152-162: Update the roomSettingBody schema to use oneOf instead of
anyOf for the roomId and roomName requirements, ensuring requests must provide
exactly one identifier and cannot silently prioritize roomId. Keep the existing
required field and additionalProperties constraints unchanged.
- Around line 531-539: Update the response generic passed to ajv.compile for
announcementResponseSchema to include success: true alongside the optional
announcement field, matching the success property added by API.v1.success().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f61ce0dd-712d-4de3-8660-649dc3805d3f
📒 Files selected for processing (1)
apps/meteor/server/api/v1/channels.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: ⚙️ Variables Setup
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (4)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 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/server/api/v1/channels.ts
🧠 Learnings (3)
📚 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/server/api/v1/channels.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/server/api/v1/channels.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/api/v1/channels.ts
🔇 Additional comments (1)
apps/meteor/server/api/v1/channels.ts (1)
186-189: LGTM!Also applies to: 193-215, 218-230, 306-318, 524-529, 541-553, 702-727, 729-752
- info: GET, roomTargetQuery (ajvQuery), returns { channel } ($ref IRoom); 403 for canAccessRoom.
- list: GET, isChannelsListProps query, PaginatedResult<{ channels: IRoom[] }>; typed ourQuery as Filter<IRoom>.
Adds roomTargetQuery + channelsListResponseSchema helpers.
- roles: GET, returns { roles: RoomRoles[] } (schema includes runtime _id the type omits).
- moderators: GET, returns { moderators: sub.u[] }; 403 for canAccessRoom.
- delete: POST, void.
- close: POST void (roomTargetBody helper added). - counters: GET, nullable numeric/date fields, userId query param, 403.
Member
Author
🤝 Handoff — continuing this migration in a fresh sessionState: How to work
Established conventions (reuse the helpers at the top of the file)
Remaining (14) — with notes
Full remaining tracker (channels + groups): the plan artifact / ARCH-1464 handoff comment. |
isChannelsListProps (additionalProperties:false, no roomId) rejected the roomId that existing clients/tests pass to list.joined even though the endpoint ignores it. Use a tolerant inline query that accepts and ignores roomId/roomName. Also loosen members item validation (additionalProperties) to avoid rejecting unforeseen projected fields. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
Comment on lines
+140
to
+151
| function errorToFailureArgs(error: unknown): [string, string | undefined] { | ||
| const e = error as { reason?: unknown; message?: unknown; error?: unknown }; | ||
| // Prefer `reason` so the `[error-code]` suffix that Meteor.Error appends to `message` does not leak to clients. | ||
| let message = String(error); | ||
| if (typeof e?.message === 'string') { | ||
| message = e.message; | ||
| } | ||
| if (typeof e?.reason === 'string') { | ||
| message = e.reason; | ||
| } | ||
| return [message, typeof e?.error === 'string' ? e.error : undefined]; | ||
| } |
There was a problem hiding this comment.
The custom error-handling utility errorToFailureArgs in channels.ts catches any thrown error and extracts its raw message property to return directly to the client. When standard JavaScript exceptions (e.g., TypeError) or database driver errors (e.g., MongoDB write conflicts or query execution errors) occur, they do not contain a reason property (which is specific to Meteor.Error). Consequently, the raw, unfiltered error message is returned to the client, potentially leaking sensitive information such as database collection names, field schemas, query structures, or internal variable names.
Steps to Reproduce
- Authenticate as a regular user.
- Invoke an API endpoint that uses
errorToFailureArgs(e.g.,channels.createorchannels.rename) with parameters designed to trigger a database or runtime exception (such as a duplicate key error or an unexpected type that bypasses validation but fails in business logic). - Observe the server's response, which contains the raw database or runtime error message.
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/server/api/v1/channels.ts
Lines: 140-151
Severity: medium
Vulnerability: Information Disclosure via Unfiltered Internal Error Messages in errorToFailureArgs
Description:
The custom error-handling utility `errorToFailureArgs` in `channels.ts` catches any thrown error and extracts its raw `message` property to return directly to the client. When standard JavaScript exceptions (e.g., `TypeError`) or database driver errors (e.g., MongoDB write conflicts or query execution errors) occur, they do not contain a `reason` property (which is specific to `Meteor.Error`). Consequently, the raw, unfiltered error message is returned to the client, potentially leaking sensitive information such as database collection names, field schemas, query structures, or internal variable names.
Proof of Concept:
**Steps to Reproduce**
1. Authenticate as a regular user.
2. Invoke an API endpoint that uses `errorToFailureArgs` (e.g., `channels.create` or `channels.rename`) with parameters designed to trigger a database or runtime exception (such as a duplicate key error or an unexpected type that bypasses validation but fails in business logic).
3. Observe the server's response, which contains the raw database or runtime error message.
Affected Code:
function errorToFailureArgs(error: unknown): [string, string | undefined] {
const e = error as { reason?: unknown; message?: unknown; error?: unknown };
// Prefer `reason` so the `[error-code]` suffix that Meteor.Error appends to `message` does not leak to clients.
let message = String(error);
if (typeof e?.message === 'string') {
message = e.message;
}
if (typeof e?.reason === 'string') {
message = e.reason;
}
return [message, typeof e?.error === 'string' ? e.error : undefined];
}
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), !accepted_risk <reason>, or !fixed (resolved). Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
Member
Author
There was a problem hiding this comment.
errorToFailureArgs was removed (596e008). The raw-message-for-non-Meteor-errors behavior now only exists in the global ApiClass wrapper (default -> api.failure(e.message)), which is pre-existing and framework-wide. Leaving open as a separate follow-up (classify unexpected errors -> generic 500).
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Revert errorToFailureArgs to use Meteor.Error.message (with the [error-code] suffix) instead of .reason. Existing e2e tests assert the code is present in the response `error` field (channels dup-name, teams.delete room-not-found), matching legacy addRoute behavior. The earlier .reason change (CodeRabbit suggestion) is incompatible with that contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- create: the create modal sends extraData.{topic,federated} (spread verbatim
into createRoom), but the schema's extraData was additionalProperties:false
without those keys, so UI channel creation 400'd and hung. Allow arbitrary
extraData (add topic/federated, additionalProperties:true).
- info: channels.info also serves omnichannel ('l') rooms (livechat test
helpers call it) which lack the owner `u` that IRoom requires. Use a
dedicated schema accepting a full IRoom or any room-shaped object.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The create modal sends broadcast:false, which createRoom stores verbatim, so rooms genuinely carry broadcast:false. IRoom typed it as `true`, making typia emit enum:[true], which failed response validation (400) on channels.create and hung UI channel/team-channel creation. Widen to boolean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
IRoom.broadcast widened to boolean, but ISubscription.broadcast is `true | undefined`. Coerce with `|| undefined` (false -> undefined) at the two cached store copy sites so the assignment stays `true | undefined` without widening the subscription type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sampaiodiego
requested changes
Jul 22, 2026
Security:
- channels.create: close extraData (additionalProperties: false) so callers
can't inject arbitrary room fields (default/featured/retention/abacAttributes);
the create modal only sends the declared fields (mass-assignment fix).
- channels.getIntegrations / channels.files: apply the user-supplied query
first and overlay the trusted permission/rid filters last so a crafted query
can't override the ownership scope or room id.
Correctness:
- errorToFailureArgs now also forwards stack (TEST_MODE) and details, and each
catch passes them to API.v1.failure — restores the legacy addRoute error body.
- channels.create: type members/teams items as strings; drop stray nullable on
readOnly/customFields/excludeSelf/extraData (contract now matches the type).
- Remove dead 'bodyParam X is required' branches in set{Description,Purpose,
Topic} — the body schema already marks the field required.
Left as follow-up (pre-existing, not this migration): raw error-message
disclosure in errorToFailureArgs for non-Meteor errors, and the anyOf vs oneOf
integrations response shape (anyOf validates correctly; oneOf risks overlap).
…piClass error wrapper The global ApiClass wrapper (_internalRouteActionHandler) already catches thrown Meteor.Errors and maps them: error-too-many-requests->429, unauthorized->401/403, forbidden->403/400, default->api.failure(...)=400. The per-handler errorToFailureArgs catch-all was therefore redundant, and worse, it flattened 401/403/429 responses into 400. Removed the helper and all 41 per-handler try/catch wrappers so handlers just throw and let the wrapper map the status. Kept the channels.create validator's custom catch (unauthorized->forbidden), which is not the errorToFailureArgs pattern. Per review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- channels.create: throw Meteor.Error('unauthorized') from the create.validate
permission gate (was a plain Error) and remove the handler's try/catch, so the
global ApiClass wrapper maps it (403, or 401 under breaking-changes) like every
other endpoint — instead of forcing 403 locally.
- docs/api-endpoint-migration.md: correct the 'Error Handling' section. The typed
router DOES map thrown errors: ApiClass wraps every route (incl. typed) in a
try/catch that maps Meteor.Error -> 429/401/403/400. Handlers should just throw;
the per-handler catch-all + errorToFailureArgs were redundant.
IIncomingIntegration/IOutgoingIntegration are discriminated by a required `type` enum (webhook-incoming vs webhook-outgoing), so they are disjoint and oneOf matches exactly one — no overlap risk. Matches the IIntegration contract convention.
The create.validate permission gate throws a plain Error('unauthorized'); the
global wrapper would surface its raw message. Restore the local catch that maps
it to API.v1.forbidden() so the response keeps the stable { error: 'unauthorized' }
(403) contract the e2e test asserts. Reverts the over-eager catch removal in b4312d9.
sampaiodiego
approved these changes
Jul 29, 2026
ggazzo
added
the
stat: QA assured
github-project-automation
Bot
moved this from Backlog
to Done
in rest api conversion to new endpoints
This was referenced Jul 29, 2026
This was referenced Aug 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Continues the API endpoint migration (
docs/api-endpoint-migration.md): migratesapps/meteor/server/api/v1/channels.tsfrom the legacyAPI.v1.addRoute()pattern to the typedAPI.v1.get/postpattern with AJV request/response validation.WIP / incremental — migrating the file in batches.
channels.*endpoints are already declared inChannelsEndpoints(rest-typings), so this follows the moderation pattern: convert the registration and add response schemas, keeping the manualEndpointsentry (noExtractRoutesFromAPIaugmentation, avoidsTS2717).Migrated so far
channels.addAll,channels.archive,channels.unarchive,channels.join,channels.kick,channels.leaveNotes
channelResponseSchema($ref IRoom) +successResponseSchema.findChannelByIdOrNamethrowsMeteor.Errorfor client errors (room-not-found / archived). The typed router does not convert thrown errors to 400 (the legacyaddRoutewrapper did), so each handler catches and returnsAPI.v1.failure(message, errorType)to preserve the previous behavior.channels.*endpoints (andgroups.ts) will follow in subsequent batches;messages/history/filesreturnIMessageand may need relaxed schemas (attachmentoneOfvalidation limitation).Testing
tsc --noEmit, eslint and prettier clean for the migrated file.TEST_MODE; e2echannelssuite will exercise them.Task: ARCH-2262
Summary by CodeRabbit
Bug Fixes
Documentation
try/catchand returnAPI.v1.failure(...)to avoid500responses from uncaught thrown errors.