fix: Improve chat message button handling#26249
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Bundle ReportChanges will increase total bundle size by 4.46kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: editor-ui-esmAssets Changed:
|
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="packages/frontend/@n8n/chat/src/components/MessageWithButtons.vue">
<violation number="1" location="packages/frontend/@n8n/chat/src/components/MessageWithButtons.vue:46">
P2: The new domain filter only allows URLs matching `window.location.origin`, but NODE-4374 requires handling cases where the webhook runs on a different domain. This will hide legitimate approval buttons for embedded chats hosted on another domain. Update the check to validate against the chat webhook origin/allowed instance URL instead of the current window origin.
(According to linked Linear issue NODE-4374.)</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/frontend/@n8n/chat/src/components/MessageWithButtons.vue
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| const isSameDomain = (link: string): boolean => { | ||
| try { | ||
| const url = new URL(link, window.location.href); | ||
| return url.origin === window.location.origin; |
There was a problem hiding this comment.
Can those urls include n8n webhook urls? Afaik buttons usually can lead to form-waiting or form endpoint.
Those endpoints can be modified by some env variable(don't remember the name) and it will be different from frontend url. Users may use a proxy for webhooks, which can lead to button not rendering even if it's technically their domain
There was a problem hiding this comment.
They can. I just figured that this can be different from the web ui url apparently and am looking how we can get this value in the chat components
There was a problem hiding this comment.
did run some tests locally to confirm that the links are always generated with the configured webhook url, so there is no need to allow the current origin
This comment has been minimized.
This comment has been minimized.
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="packages/frontend/@n8n/chat/src/components/MessageWithButtons.vue">
<violation number="1" location="packages/frontend/@n8n/chat/src/components/MessageWithButtons.vue:22">
P1: According to linked Linear issue NODE-4374, buttons must render only for URLs that point to the n8n instance. Including `window.location.origin` in the allowlist means any host site embedding the widget still accepts its own origin, so prompt-injected buttons can point outside the n8n instance. Limit the allowlist to the webhook origin only.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/frontend/@n8n/chat/src/components/MessageWithButtons.vue
Outdated
Show resolved
Hide resolved
yehorkardash
left a comment
yehorkardash
left a comment
There was a problem hiding this comment.
LGTM, just two nitpicks
| const chatOptions = useOptions(); | ||
| const clickedButtonIndex = ref<number | null>(null); | ||
|
|
||
| const isValidOrigin = (link: string): boolean => { |
There was a problem hiding this comment.
Would maybe make it computed function
There was a problem hiding this comment.
I did check what the chatOptions are and the result is not reactive, so a computed value would also never be updated
| <template v-for="(button, index) in buttons" :key="button.text"> | ||
| <Button | ||
| v-if="clickedButtonIndex === null || index === clickedButtonIndex" | ||
| v-if=" |
There was a problem hiding this comment.
Would be nice to have clickedButtonIndex === null || index === clickedButtonIndex as a variable with a clear name, because it's not clear what is it for
There was a problem hiding this comment.
This is indeed not optimal. I did extract the check into the new function and renamed it to isButtonVisible. The clickedButtonIndex.value === null || index === clickedButtonIndex.value is still there since a proper description would be "if no buttons clicked or current button is clicked" and converting it to multiple conditions would not make it better, imo
|
Got released with |
2 participants
Summary
Improves chat action button rendering
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/NODE-4374
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)