fix(integrations): Add client-side validation for Incoming Webhook Avatar URL#37432
fix(integrations): Add client-side validation for Incoming Webhook Avatar URL#37432MrKalyanKing wants to merge 4 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 3bcbd46 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
WalkthroughExpanded incoming and outgoing webhook form data models; added client-side validators and error feedback for incoming webhook avatar and emoji fields; updated form initial values and save payloads; added changeset entries documenting the integration validation fix. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form as IncomingWebhookForm
participant Validators as Avatar/Emoji Validators
participant Hook as react-hook-form
participant Editor as EditIncomingWebhook
participant API as Server
User->>Form: Fill fields & Submit
Form->>Hook: handleSubmit
Hook->>Validators: validate avatar & emoji
Validators-->>Hook: validation result
alt validation passes
Hook->>Editor: onSubmit(formValues)
Editor->>API: create/update payload (includes expanded fields)
API-->>Editor: success
Editor-->>User: success UI
else validation fails
Hook->>Form: return field errors
Form-->>User: show FieldError(s) (aria-live)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx (1)
48-56: Consider a more specific emoji format validation.The current regex
/^:.+:$/accepts any non-empty content between colons, including whitespace-only strings like": :"or unusual patterns. Standard emoji shortcodes typically use alphanumeric characters, hyphens, and underscores.Apply this diff for stricter validation:
- if (/^:.+:$/.test(value)) { + if (/^:[a-z0-9_+-]+:$/i.test(value)) { return true; }apps/meteor/client/views/admin/integrations/incoming/EditIncomingWebhook.tsx (2)
76-86: LGTM! Error handling logic is appropriate.The
handleInvalidcallback correctly extracts validation error messages and forwards them to the error handler. The else-if chain ensures only one error is displayed at a time, which is standard for toast notifications.If you want to display all validation errors simultaneously, consider this alternative:
- const handleInvalid = (errors: { - avatar?: { message?: string }; - emoji?: { message?: string }; - }) => { - - if (errors.avatar && errors.avatar.message) { - handleError(errors.avatar.message); - } else if (errors.emoji && errors.emoji.message) { - handleError(errors.emoji.message); - } + const handleInvalid = (errors: { + avatar?: { message?: string }; + emoji?: { message?: string }; + }) => { + const errorMessages = [ + errors.avatar?.message, + errors.emoji?.message, + ].filter(Boolean); + + if (errorMessages.length > 0) { + handleError(errorMessages.join(' ')); + } };
138-138: Add space after comma for consistency.Minor formatting issue in the function call.
Apply this diff:
- <PageScrollableContentWithShadow id={formId} is='form' onSubmit={handleSubmit(handleSave,handleInvalid)}> + <PageScrollableContentWithShadow id={formId} is='form' onSubmit={handleSubmit(handleSave, handleInvalid)}>apps/meteor/client/views/admin/integrations/outgoing/EditOutgoingWebhook.tsx (1)
162-162: Remove extra whitespace in JSX tag.Minor formatting inconsistency with double space before the self-closing tag.
Apply this diff:
- <OutgoingWebhookForm /> + <OutgoingWebhookForm />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/client/views/admin/integrations/incoming/EditIncomingWebhook.tsx(4 hunks)apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx(3 hunks)apps/meteor/client/views/admin/integrations/outgoing/EditOutgoingWebhook.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🔇 Additional comments (2)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx (1)
37-46: LGTM! URL validation approach is sound.Using the native
URLconstructor is the standard and appropriate method for validating URLs.apps/meteor/client/views/admin/integrations/incoming/EditIncomingWebhook.tsx (1)
8-8: The import path and hook usage are correct—no changes needed.The
useErrorHandlerhook is located inviews/admin/import/but is a generic error handler (it dispatches toast messages and logs errors), not import-specific functionality. The hook is actively used in EditIncomingWebhook.tsx at line 59, and the import path correctly resolves. This is the standard error handling pattern used in this part of the codebase.
| // | ||
|
|
There was a problem hiding this comment.
Remove incomplete error handling scaffolding.
The code references useErrorHandler (line 75) without importing it, and the resulting handleError is never used. This appears to be incomplete implementation, possibly copied from EditIncomingWebhook.tsx. The comments at lines 15, 16, 76, 96, and 97 also appear to be leftover debugging artifacts.
Apply this diff to remove the incomplete code:
-//
-
-
type EditOutgoingWebhookFormData = { const EditOutgoingWebhook = ({ webhookData }: EditOutgoingWebhookProps) => {
const t = useTranslation();
- //
- const handleError = useErrorHandler();
- //
const setModal = useSetModal(); const deleteIntegration = useDeleteIntegration(OUTGOING_TYPE);
const createIntegration = useCreateIntegration(OUTGOING_TYPE);
const updateIntegration = useUpdateIntegration(OUTGOING_TYPE);
-
-Also applies to: 74-76, 96-97
🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/integrations/outgoing/EditOutgoingWebhook.tsx
around lines 15-16, 74-76 and 96-97, remove the leftover debugging comments and
the incomplete error-handling scaffolding: delete the stray comment lines at
15-16 and 96-97, remove the useErrorHandler reference and the unused handleError
variable at/around line 75 (and remove its import from the top of the file if
present), and clean up any now-unused imports caused by that removal; ensure no
other code references handleError after deletion.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/funny-islands-run.md(1 hunks).changeset/light-singers-join.md(1 hunks)apps/meteor/client/views/admin/integrations/incoming/EditIncomingWebhook.tsx(3 hunks)apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/funny-islands-run.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/admin/integrations/incoming/EditIncomingWebhook.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx (1)
packages/livechat/src/components/Form/TextInput/index.tsx (1)
TextInput(38-38)
🪛 ast-grep (0.39.7)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx
[warning] 316-316: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx
[error] 317-317: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (4)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx (3)
45-53: LGTM!The emoji validation correctly enforces the colon format (
:emoji:) and allows empty values. The permissive regex is appropriate for client-side validation, with server-side validation handling emoji existence.
267-283: Validation and accessibility properly implemented.The avatar field now includes proper validation rules, error handling, and accessibility attributes. The FieldError component with
aria-live='assertive'ensures screen readers announce validation errors, addressing previous review feedback.
295-319: Validation and accessibility properly implemented.The emoji field validation and error handling follow the same pattern as the avatar field. Accessibility is properly configured with FieldError and aria attributes, addressing previous review feedback.
.changeset/light-singers-join.md (1)
1-5: LGTM!The changeset properly declares a patch-level version bump with an accurate description that aligns with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx (3)
48-56: Consider tightening the emoji validation regex.The current regex
/^:.+:$/accepts any characters between colons, including spaces and special characters (e.g.,:hello world:or:!@#$:). Standard emoji shortcodes typically contain only alphanumeric characters, underscores, hyphens, and plus signs.Apply this diff for stricter validation:
const validateEmoji = (value: string) => { if (!value) return true; - if (/^:.+:$/.test(value)) { + if (/^:[a-z0-9_+-]+:$/i.test(value)) { return true; } return t('Invalid_emoji_format_Must_be_in_colon_format', { example: ':ghost:' }); };
263-289: Great work on accessibility! Consider minor consistency improvement.The validation and accessibility features are properly implemented. All previous review concerns have been addressed.
For consistency, since you're already destructuring
errorfromfieldState(line 270), you could use it for the conditional rendering as well:Apply this diff for consistency:
</FieldRow> - {errors?.avatar && ( + {error && ( <FieldError aria-live='assertive' id={`${avatarField}-error`}> - {errors.avatar.message} + {error.message} </FieldError> )}
291-321: Great work on accessibility! Consider minor consistency improvement.The validation and accessibility features are properly implemented. All previous review concerns have been addressed.
For consistency, since you're already destructuring
errorfromfieldState(line 298), you could use it for the conditional rendering as well:Apply this diff for consistency:
</FieldRow> - {errors?.emoji && ( + {error && ( <FieldError aria-live='assertive' id={`${emojiField}-error`}> - {errors.emoji.message} + {error.message} </FieldError> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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
🪛 ast-grep (0.39.7)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx
[warning] 318-318: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx
[error] 319-319: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (1)
apps/meteor/client/views/admin/integrations/incoming/IncomingWebhookForm.tsx (1)
35-46: LGTM! URL validation is secure and well-implemented.The validation properly allows empty values, restricts to safe http/https schemes (preventing XSS via javascript: or data: URLs), and provides clear error feedback.
1 participant
[x] I have read the Contributing Guide
[x] I have signed the CLA
[x] Lint and unit tests pass locally with my changes
[ ] I have added tests that prove my fix is effective or that my feature works (if applicable)
[ ] I have added necessary documentation (if applicable)
[ ] Any dependent changes have been merged and published in downstream modules
Proposed changes (including videos or screenshots)
This PR fixes a bug where the "Avatar URL" field on the Incoming Webhook form was not being validated. It allowed a user to save any invalid string (like "jjjj"), which would result in a broken avatar.
This field is optional, so this fix correctly allows an empty string, but validates the input if a value is provided.
The Fix:
EditIncomingWebhook.tsx(Parent):useErrorHandler.validateAvatarUrlfunction (using the nativeURLconstructor).handleInvalidfunction to catch the validation error from theavatarfield and trigger the pop-up toast.handleSubmitis updated to callhandleSubmit(handleSave, handleInvalid).IncomingWebhookForm.tsx(Child):validateAvatarUrlfunction as a prop from the parent.rules={{ validate: validateAvatarUrl }}prop to theavatarfield's<Controller>.Issue(s)
Closes **fix(integrations): Incoming Webhook 'Avatar URL' field has no validation #37431
Steps to test or reproduce
Administration > Integrations.Further comments
This is my second contribution. I found this bug while testing the integration forms. This PR uses the parent/child form validation pattern with
useErrorHandler(as discussed in my previous PR).Ready for any feedback!
Summary by CodeRabbit
New Features
Bug Fixes