fix: fix incoming webhook alias validation#38855
Open
roushnisareen wants to merge 8 commits into
Open
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
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:
WalkthroughAdds alias validation for integrations: new validator module, server-side checks in add/update incoming integration methods, client-side form validation and max-length enforcement, API schema constraints, i18n messages, and end-to-end tests for alias creation and update validation. Changes
Sequence Diagram(s)mermaid Client->>Validator: onChange / before submit -> validateIntegrationAlias(alias) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
roushnisareen
changed the title
![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: 4
🧹 Nitpick comments (4)
packages/rest-typings/src/v1/integrations/IntegrationsCreateProps.ts (1)
88-92: Same hardcoded60asIntegrationsUpdateProps.ts— referenceINTEGRATION_ALIAS_MAX_LENGTH.Also applies to: 193-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rest-typings/src/v1/integrations/IntegrationsCreateProps.ts` around lines 88 - 92, Replace the hardcoded alias maxLength value in IntegrationsCreateProps (the alias property block) with the shared constant INTEGRATION_ALIAS_MAX_LENGTH used in IntegrationsUpdateProps; update both occurrences mentioned (line regions around the alias definition and the other spot at 193-197) to reference INTEGRATION_ALIAS_MAX_LENGTH so both create and update schemas use the same constant instead of the literal 60.apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx (1)
82-91:validateAliasshould be stabilized withuseCallback— new function reference on every render.Defining
validateAliasinside the component body creates a fresh reference every render. react-hook-form uses referential equality forrules.validate; a changing reference can cause unnecessary field re-registrations and re-validation cycles. Wrap inuseCallbackwith[t]as the dependency.Additionally, the parameter name
alias(line 82) shadows the outeraliasvariable fromwatch()at line 41, which can cause confusion during maintenance.♻️ Proposed fix
- const validateAlias = (alias: string): string | undefined => { - switch (validateIntegrationAlias(alias)) { - case 'too-long': - return t('error-invalid-alias-length', { max: INTEGRATION_ALIAS_MAX_LENGTH }); - case 'invalid-characters': - return t('error-invalid-alias'); - default: - return; - } - }; + const validateAliasField = useCallback( + (value: string): string | undefined => { + switch (validateIntegrationAlias(value)) { + case 'too-long': + return t('error-invalid-alias-length', { max: INTEGRATION_ALIAS_MAX_LENGTH }); + case 'invalid-characters': + return t('error-invalid-alias'); + default: + return undefined; + } + }, + [t], + );And update the
useIdimports/usages if needed:- rules={{ validate: validateAlias }} + rules={{ validate: validateAliasField }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx` around lines 82 - 91, The local function validateAlias is recreated every render and also shadows the outer alias from watch(); wrap validateAlias in React.useCallback with dependency [t] to stabilize the function reference so it can be passed to react-hook-form rules.validate without causing re-registrations, and rename its parameter (e.g., to value or input) to avoid shadowing the watched alias variable; ensure the implementation still calls validateIntegrationAlias(value) and returns the same translated error strings.packages/rest-typings/src/v1/integrations/IntegrationsUpdateProps.ts (1)
94-98: Consider referencingINTEGRATION_ALIAS_MAX_LENGTHinstead of hardcoding60.Both alias schema definitions hardcode
60. IfINTEGRATION_ALIAS_MAX_LENGTHinvalidateAlias.tsis ever updated, the AJV schema will silently diverge from the runtime validator.Also applies to: 207-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rest-typings/src/v1/integrations/IntegrationsUpdateProps.ts` around lines 94 - 98, The AJV schema for the alias property in IntegrationsUpdateProps.ts currently hardcodes maxLength: 60 which can diverge from the runtime validator; update both alias schema entries (the alias property definitions around the first occurrence and the second at lines ~207-211) to reference the shared constant INTEGRATION_ALIAS_MAX_LENGTH (import it from validateAlias.ts) instead of using the literal 60 so the schema and runtime validation stay in sync.apps/meteor/tests/end-to-end/api/incoming-integrations.ts (1)
965-987: Missing test: update path with a too-long alias.The create path has both character-invalid and too-long tests, but the update path only covers invalid characters (lines 965–987). Adding a parallel too-long test for
integrations.updatewould complete parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/incoming-integrations.ts` around lines 965 - 987, Add a new test mirroring the create-path "too-long alias" case for the update path: create a test named like "should return an error when updating an integration with too-long alias" that calls request.put(api('integrations.update')) with the same fields used in the existing update test but set alias to a string longer than the allowed max (e.g., generate > allowed length) and include integrationId: integration._id; assert a 400 response and that res.body.success is false and res.body.errorType equals the too-long alias error (e.g., 'error-too-long-alias') to match the create-path parity with integrations.update.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/meteor/app/integrations/lib/validateAlias.tsapps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.tsapps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsxapps/meteor/tests/end-to-end/api/incoming-integrations.tspackages/i18n/src/locales/en.i18n.jsonpackages/rest-typings/src/v1/integrations/IntegrationsCreateProps.tspackages/rest-typings/src/v1/integrations/IntegrationsUpdateProps.ts
🧰 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/integrations/server/methods/incoming/updateIncomingIntegration.tspackages/rest-typings/src/v1/integrations/IntegrationsUpdateProps.tsapps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsxpackages/rest-typings/src/v1/integrations/IntegrationsCreateProps.tsapps/meteor/app/integrations/lib/validateAlias.tsapps/meteor/tests/end-to-end/api/incoming-integrations.tsapps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts
🧠 Learnings (12)
📚 Learning: 2025-09-18T17:32:33.969Z
Learnt from: aleksandernsilva
Repo: RocketChat/Rocket.Chat PR: 36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR `#36972` through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
Applied to files:
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json
🧬 Code graph analysis (3)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
apps/meteor/app/integrations/lib/validateAlias.ts (1)
validateIntegrationAlias(5-25)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx (1)
apps/meteor/app/integrations/lib/validateAlias.ts (2)
validateIntegrationAlias(5-25)INTEGRATION_ALIAS_MAX_LENGTH(1-1)
apps/meteor/tests/end-to-end/api/incoming-integrations.ts (1)
packages/core-services/src/index.ts (1)
api(55-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (2)
packages/i18n/src/locales/en.i18n.json (1)
6237-6238: Looks good — clear and consistent error messaging.
The new alias validation strings are concise and match the validation rules.apps/meteor/app/integrations/lib/validateAlias.ts (1)
3-3: Verify that\p{L}/\p{N}(any Unicode letter/number) is the intended character scope.The regex admits any Unicode letter (
\p{L}) and number (\p{N}) — including CJK, Arabic, Cyrillic, etc. The PR description states "letters, numbers, spaces, ., _, ', -" without specifying Unicode breadth. If only ASCII alphanumerics were intended, the regex should be[a-zA-Z0-9 ._'-].
🤖 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/app/integrations/lib/validateAlias.ts`:
- Around line 10-14: validateIntegrationAlias trims the alias for validation but
addIncomingIntegration and updateIncomingIntegration persist the raw
integration.alias, causing stored values to keep surrounding whitespace or
whitespace-only strings; update the server methods (addIncomingIntegration and
updateIncomingIntegration) to sanitize before saving by replacing
integration.alias with a trimmed value (and if trimmed is empty, set alias to
null/undefined or omit it from $set) so persisted aliases match what the
validator accepts and no whitespace-only alias is stored.
In
`@apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts`:
- Around line 82-86: The server currently maps all validateIntegrationAlias
results to a single Meteor.Error('error-invalid-alias') in
addIncomingIntegration and updateIncomingIntegration; change the throw so that
when validateIntegrationAlias(...) returns 'too-long' you throw
Meteor.Error('error-invalid-alias-length', 'Invalid alias length', { method:
'addIncomingIntegration' }) (and analogous for updateIncomingIntegration), and
preserve the existing Meteor.Error('error-invalid-alias') for the
'invalid-characters' case so API/DDP consumers can distinguish the two failure
modes.
In
`@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`:
- Around line 75-79: The current throw uses a method-specific error code; change
it to use the same single shared error code used in addIncomingIntegration.ts
when validateIntegrationAlias(integration.alias) !== 'ok' so both handlers
collapse to one canonical error code. Replace the Meteor.Error first argument in
the updateIncomingIntegration throw with the shared code used in
addIncomingIntegration.ts (keep the message 'Invalid alias' and the details
object { method: 'updateIncomingIntegration' }) and run tests to ensure
consistency.
In `@apps/meteor/tests/end-to-end/api/incoming-integrations.ts`:
- Around line 257-278: The test "should return an error when creating an
incoming integration with an alias longer than the allowed length" currently
allows two error types but the REST AJV schema (maxLength: 60) always produces
'error-invalid-params', so update the assertion in this test (the it block) to
expect only 'error-invalid-params'; alternatively, if you want to cover the
server-side too-long branch in validateIntegrationAlias, add a separate
DDP-level test that calls the integrations.create method directly (bypassing
AJV) and asserts 'error-invalid-alias'.
---
Nitpick comments:
In
`@apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx`:
- Around line 82-91: The local function validateAlias is recreated every render
and also shadows the outer alias from watch(); wrap validateAlias in
React.useCallback with dependency [t] to stabilize the function reference so it
can be passed to react-hook-form rules.validate without causing
re-registrations, and rename its parameter (e.g., to value or input) to avoid
shadowing the watched alias variable; ensure the implementation still calls
validateIntegrationAlias(value) and returns the same translated error strings.
In `@apps/meteor/tests/end-to-end/api/incoming-integrations.ts`:
- Around line 965-987: Add a new test mirroring the create-path "too-long alias"
case for the update path: create a test named like "should return an error when
updating an integration with too-long alias" that calls
request.put(api('integrations.update')) with the same fields used in the
existing update test but set alias to a string longer than the allowed max
(e.g., generate > allowed length) and include integrationId: integration._id;
assert a 400 response and that res.body.success is false and res.body.errorType
equals the too-long alias error (e.g., 'error-too-long-alias') to match the
create-path parity with integrations.update.
In `@packages/rest-typings/src/v1/integrations/IntegrationsCreateProps.ts`:
- Around line 88-92: Replace the hardcoded alias maxLength value in
IntegrationsCreateProps (the alias property block) with the shared constant
INTEGRATION_ALIAS_MAX_LENGTH used in IntegrationsUpdateProps; update both
occurrences mentioned (line regions around the alias definition and the other
spot at 193-197) to reference INTEGRATION_ALIAS_MAX_LENGTH so both create and
update schemas use the same constant instead of the literal 60.
In `@packages/rest-typings/src/v1/integrations/IntegrationsUpdateProps.ts`:
- Around line 94-98: The AJV schema for the alias property in
IntegrationsUpdateProps.ts currently hardcodes maxLength: 60 which can diverge
from the runtime validator; update both alias schema entries (the alias property
definitions around the first occurrence and the second at lines ~207-211) to
reference the shared constant INTEGRATION_ALIAS_MAX_LENGTH (import it from
validateAlias.ts) instead of using the literal 60 so the schema and runtime
validation stay in sync.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
52-52: Consider moving alias validation before the permission/DB checks, consistent withvalidateChannels.
validateChannels(line 52) runs before any DB access. Alias validation at lines 75–82 fires after two DB round-trips (permission check + integration fetch). Moving it to the top would fail fast on malformed input at no cost to security.♻️ Suggested reorder
export const updateIncomingIntegration = async (...) => { const channels = validateChannels(integration.channel); + + const aliasValidation = validateIntegrationAlias(integration.alias); + if (aliasValidation === 'too-long') { + throw new Meteor.Error('error-invalid-alias-length', 'Invalid alias length', { method: 'updateIncomingIntegration' }); + } + if (aliasValidation === 'invalid-characters') { + throw new Meteor.Error('error-invalid-alias', 'Invalid alias', { method: 'updateIncomingIntegration' }); + } + let currentIntegration; // ... permission checks ... - const aliasValidation = validateIntegrationAlias(integration.alias); - if (aliasValidation === 'too-long') { ... } - if (aliasValidation === 'invalid-characters') { ... } - const sanitizedAlias = integration.alias?.trim();Also applies to: 75-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts` at line 52, Move the alias validation to run before any DB/permission checks so malformed aliases fail fast (same pattern as validateChannels): call the alias validation function (the code currently at lines 75–82 that validates the incoming integration alias) immediately after validateChannels(integration.channel) at the start of updateIncomingIntegration, before performing permission checks and fetching the integration from the DB; this eliminates the two round-trips on bad input while keeping all existing permission and fetch logic unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.tsapps/meteor/tests/end-to-end/api/incoming-integrations.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/tests/end-to-end/api/incoming-integrations.ts
- apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts
🧰 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/integrations/server/methods/incoming/updateIncomingIntegration.ts
🧠 Learnings (1)
📚 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/integrations/server/methods/incoming/updateIncomingIntegration.ts
🧬 Code graph analysis (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
apps/meteor/app/integrations/lib/validateAlias.ts (1)
validateIntegrationAlias(5-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
84-84: LGTM —sanitizedAliascorrectly handles undefined propagation and whitespace trimming.
🤖 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/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`:
- Line 193: Replace the current behavior that sets alias: sanitizedAlias ||
undefined with explicit $set/$unset logic: when sanitizedAlias is truthy include
alias in the update's $set, and when falsy ensure alias is added to $unset
(similar to how scriptError is handled) so existing aliases are removed instead
of being set to null; update the update object construction that builds $set and
$unset (the block using sanitizedAlias, alias, $set/$unset and scriptError) to
conditionally push alias into $set or $unset accordingly.
---
Duplicate comments:
In
`@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`:
- Around line 75-82: The alias-validation error codes thrown in
updateIncomingIntegration (use of validateIntegrationAlias and the Meteor.Error
instances in updateIncomingIntegration) must match the codes used in
addIncomingIntegration.ts; inspect addIncomingIntegration.ts to confirm it
throws 'error-invalid-alias-length' and 'error-invalid-alias' for the same
validation results and, if they differ, update the Meteor.Error code strings in
updateIncomingIntegration to exactly match addIncomingIntegration.ts so the API
surface is consistent.
---
Nitpick comments:
In
`@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`:
- Line 52: Move the alias validation to run before any DB/permission checks so
malformed aliases fail fast (same pattern as validateChannels): call the alias
validation function (the code currently at lines 75–82 that validates the
incoming integration alias) immediately after
validateChannels(integration.channel) at the start of updateIncomingIntegration,
before performing permission checks and fetching the integration from the DB;
this eliminates the two round-trips on bad input while keeping all existing
permission and fetch logic unchanged.
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 (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
228-232:⚠️ Potential issue | 🟡 MinorPre-existing copy-paste typo in error details:
'updateOutgoingIntegration'→'updateIncomingIntegration'.The error metadata reports the wrong method name, which will mislead any consumer or log that inspects the
methodfield.🐛 Proposed fix
- throw new Meteor.Error('error-invalid-user', 'Invalid user', { - method: 'updateOutgoingIntegration', + throw new Meteor.Error('error-invalid-user', 'Invalid user', { + method: 'updateIncomingIntegration', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts` around lines 228 - 232, The thrown Meteor.Error in the authentication guard uses the wrong method metadata string ('updateOutgoingIntegration'); update the error metadata to use 'updateIncomingIntegration' so the method field matches the actual function name (the error thrown in the block that checks if (!this.userId) inside updateIncomingIntegration.ts). Ensure the thrown Meteor.Error keeps the same error code and message but replaces the method value with 'updateIncomingIntegration'.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
🧰 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/integrations/server/methods/incoming/updateIncomingIntegration.ts
🧠 Learnings (1)
📚 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/integrations/server/methods/incoming/updateIncomingIntegration.ts
🧬 Code graph analysis (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
apps/meteor/app/integrations/lib/validateAlias.ts (1)
validateIntegrationAlias(5-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (2)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (2)
12-12: LGTM.
185-216:setFieldsrefactor +$set/$unsetpattern looks correct.The
setFieldsgrouping cleanly separates the common fields from the conditional alias handling. The conditional$unset: { alias: 1 as const }whensanitizedAliasis falsy correctly prevents the MongoDB driver from serializingundefinedasnull, which would corruptaliasfor any consumer expectingstring | undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`:
- Around line 228-232: The thrown Meteor.Error in the authentication guard uses
the wrong method metadata string ('updateOutgoingIntegration'); update the error
metadata to use 'updateIncomingIntegration' so the method field matches the
actual function name (the error thrown in the block that checks if
(!this.userId) inside updateIncomingIntegration.ts). Ensure the thrown
Meteor.Error keeps the same error code and message but replaces the method value
with 'updateIncomingIntegration'.
---
Duplicate comments:
In
`@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`:
- Around line 75-82: The validation in updateIncomingIntegration (the
validateIntegrationAlias checks and thrown Meteor.Error instances) uses
method-specific error codes for 'too-long' and 'invalid-characters', which
differs from addIncomingIntegration.ts; update updateIncomingIntegration to use
the same shared error code(s) as addIncomingIntegration.ts (e.g., collapse both
branches to throw the common 'error-invalid-alias' code or whatever code
addIncomingIntegration uses), keep the same human-readable message and include
the method metadata ('updateIncomingIntegration') as before, and remove the
existing divergent code strings so API consumers receive a single consistent
error code for alias validation failures; locate the checks around
validateIntegrationAlias in updateIncomingIntegration and adjust the thrown
Meteor.Error code(s) to match addIncomingIntegration.ts.
Author
|
@RocketChat/maintainers @ggazzo Could someone please approve the pending workflows? The required '✅ Tests Done' check appears blocked until those approvals run. Also please add required metadata (stat: QA assured, milestone/project) and help with code-owner review. Thanks. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38855 +/- ##
===========================================
- Coverage 70.55% 70.51% -0.05%
===========================================
Files 3188 3188
Lines 112666 112665 -1
Branches 20395 20442 +47
===========================================
- Hits 79494 79447 -47
- Misses 31113 31158 +45
- Partials 2059 2060 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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.
###Title
fix: validate Incoming WebHook alias input
###Proposed changes (including videos or screenshots)
This PR fixes a gap where Incoming WebHook Alias could be saved with unsupported characters or very long values.
###What I changed:
Added a shared alias validator: validateAlias.ts
Added server-side checks so invalid alias values are rejected on create/update:
addIncomingIntegration.ts
updateIncomingIntegration.ts
Added client-side validation and inline error state in the Incoming WebHook form:
IncomingWebhookForm.tsx
Added REST schema max-length checks for alias:
IntegrationsCreateProps.ts
IntegrationsUpdateProps.ts
Added i18n strings for alias validation errors:
en.i18n.json
Added API test coverage for invalid alias values:
incoming-integrations.ts
Rules now applied to alias:
max length: 60
allowed chars: letters, numbers, spaces, ., _, ', -
empty alias is still allowed
Issue(s)
Incoming WebHook integrations accepted invalid Alias values (unsupported special chars and long strings) and used them in posted messages.
###Steps to test or reproduce
Open Administration -> Workspace -> Integrations -> New Integration -> Incoming WebHook.
Fill required fields.
Try invalid alias values like @#$$ or a 300+ character string.
Click Save.
Confirm save is blocked and validation is shown.
Use a valid alias (example: My Bot_01) and save.
Trigger webhook and confirm messages show expected alias.
API validation:
integrations.create with invalid alias returns 400
integrations.update with invalid alias returns 400
###Further comments
Goal here was consistency and safety:
UI validation gives immediate feedback.
Server/API validation ensures invalid data cannot be persisted even if UI is bypassed.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
Localization