From 36a05097fa44b716a8d489d0aa40e188c5d4cc30 Mon Sep 17 00:00:00 2001 From: ramalau <71857041+ramalau0@users.noreply.github.com> Date: Tue, 2 Jun 2026 19:58:13 +0200 Subject: [PATCH 1/3] fix(webhooks): strip trailing newlines from webhook message content (#14272) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TipTap/ProseMirror editor stores agent messages with trailing paragraph nodes that produce trailing newlines (e.g. \`\n\n\n\`) in the \`content\` field. While Chatwoot's native channel delivery already handles this, webhook payloads and API responses were returning raw content with trailing whitespace — causing visible blank space below messages in every external integration that consumes Chatwoot webhooks (WhatsApp via Evolution API, Telegram bots, custom webhook consumers). Closes #13459 ## Root cause \`Messages::WebhookContentNormalizer\` already strips CommonMark hard line breaks (\`\\\` + newline) for webhook consumers, but it did not strip trailing whitespace. All webhook and API responses flow through this normaliser, so it is the single correct place to apply the fix without touching stored data. ## What changed Added \`.rstrip\` to \`Messages::WebhookContentNormalizer.normalize\`: \`\`\`ruby # before text.gsub(/\\\r?\n/, "\n") # after text.gsub(/\\\r?\n/, "\n").rstrip \`\`\` ## Trade-offs considered | Option | Decision | |---|---| | \`before_save\` on \`Message\` model | Would clean stored data but is a broader change affecting all message creation paths and would require a data migration for existing records. Out of scope for this bug. | | Trim in each channel's send path | DRY violation — many channels, each would need the same patch. | | Fix at normaliser level (chosen) | Single location, only affects webhook/API output, zero risk to stored data or native channel delivery. | **Known limitation:** existing messages in the database still have trailing newlines in storage. They will be delivered correctly through webhooks after this fix, but a follow-up migration could clean stored content if needed. ## How to reproduce 1. Send an agent reply from the Chatwoot UI 2. Inspect the \`content\` field of the outgoing \`message_created\` webhook payload 3. Observe trailing \`\n\n\n\` after the message text After this fix, the \`content\` field is trimmed before delivery. --------- Co-authored-by: Ramalau Debeila Co-authored-by: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> --- .../messages/webhook_content_normalizer.rb | 3 +- .../webhook_content_normalizer_spec.rb | 41 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 spec/services/messages/webhook_content_normalizer_spec.rb diff --git a/app/services/messages/webhook_content_normalizer.rb b/app/services/messages/webhook_content_normalizer.rb index b45f83ad0..5fa66fe9c 100644 --- a/app/services/messages/webhook_content_normalizer.rb +++ b/app/services/messages/webhook_content_normalizer.rb @@ -1,10 +1,11 @@ # Strips CommonMark hard line breaks from stored markdown source (backslash before newline). # ProseMirror / the dashboard editor emits this form so soft breaks survive as markdown; # webhook consumers expect plain newlines without a visible backslash (e.g. WhatsApp gateways). +# Also strips trailing newlines introduced by TipTap/ProseMirror trailing paragraph nodes. class Messages::WebhookContentNormalizer def self.normalize(text) return text if text.blank? - text.gsub(/\\\r?\n/, "\n") + text.gsub(/\\\r?\n/, "\n").sub(/(\r?\n)+\z/, '') end end diff --git a/spec/services/messages/webhook_content_normalizer_spec.rb b/spec/services/messages/webhook_content_normalizer_spec.rb new file mode 100644 index 000000000..ca5023b9d --- /dev/null +++ b/spec/services/messages/webhook_content_normalizer_spec.rb @@ -0,0 +1,41 @@ +require 'rails_helper' + +RSpec.describe Messages::WebhookContentNormalizer do + describe '.normalize' do + it 'returns nil unchanged' do + expect(described_class.normalize(nil)).to be_nil + end + + it 'returns blank string unchanged' do + expect(described_class.normalize('')).to eq('') + end + + it 'strips trailing newlines added by TipTap/ProseMirror' do + expect(described_class.normalize("hello\n\n\n")).to eq('hello') + end + + it 'preserves intentional trailing spaces' do + expect(described_class.normalize("hello \n\n")).to eq('hello ') + end + + it 'replaces CommonMark hard line breaks (backslash-newline) with plain newlines' do + expect(described_class.normalize("hello\\\nworld")).to eq("hello\nworld") + end + + it 'replaces CommonMark hard line breaks with CRLF with plain newlines' do + expect(described_class.normalize("hello\\\r\nworld")).to eq("hello\nworld") + end + + it 'preserves intentional internal newlines' do + expect(described_class.normalize("line one\nline two")).to eq("line one\nline two") + end + + it 'strips trailing CRLF newlines without leaving dangling carriage returns' do + expect(described_class.normalize("hello\r\n\r\n")).to eq('hello') + end + + it 'handles both hard line breaks and trailing newlines together' do + expect(described_class.normalize("hello\\\nworld\n\n\n")).to eq("hello\nworld") + end + end +end From b791d75b30dc6478faed54b7f65939dc0ee85ec2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Fitzner?= Date: Wed, 3 Jun 2026 03:35:25 -0300 Subject: [PATCH 2/3] fix(microsoft): prevent OAuth admin consent loop (#13962) Fixes #9775 ## Description This fixes a repeated admin consent loop in the Microsoft OAuth flow when connecting a Microsoft email inbox. Chatwoot was always sending `prompt=consent` in the Microsoft authorization URL. In the current code path, this parameter is only used when building the authorization URL and is not required by the callback, token exchange, token persistence, or refresh flow. By removing the forced consent prompt, the OAuth flow can proceed normally without repeatedly sending users back through the admin consent screen. ## What changed - removed `prompt: 'consent'` from the Microsoft authorization URL - added a regression assertion to ensure `prompt` is not included in the generated URL ## Why this is safe - `redirect_uri`, `scope`, and `state` remain unchanged - callback and token exchange flow remain unchanged - refresh token flow remains unchanged - no other part of the current Microsoft inbox flow depends on forcing a consent screen ## Testing - updated controller spec to assert that the generated authorization URL does not include `prompt` --- .../api/v1/accounts/microsoft/authorizations_controller.rb | 3 +-- .../api/v1/accounts/microsoft/authorization_controller_spec.rb | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/v1/accounts/microsoft/authorizations_controller.rb b/app/controllers/api/v1/accounts/microsoft/authorizations_controller.rb index a300b5f59..c65a3031d 100644 --- a/app/controllers/api/v1/accounts/microsoft/authorizations_controller.rb +++ b/app/controllers/api/v1/accounts/microsoft/authorizations_controller.rb @@ -6,8 +6,7 @@ class Api::V1::Accounts::Microsoft::AuthorizationsController < Api::V1::Accounts { redirect_uri: "#{base_url}/microsoft/callback", scope: scope, - state: state, - prompt: 'consent' + state: state } ) if redirect_url diff --git a/spec/controllers/api/v1/accounts/microsoft/authorization_controller_spec.rb b/spec/controllers/api/v1/accounts/microsoft/authorization_controller_spec.rb index 60b05b36c..18a26a393 100644 --- a/spec/controllers/api/v1/accounts/microsoft/authorization_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/microsoft/authorization_controller_spec.rb @@ -43,6 +43,7 @@ RSpec.describe 'Microsoft Authorization API', type: :request do ] expect(params['scope']).to eq(expected_scope) expect(params['redirect_uri']).to eq(["#{ENV.fetch('FRONTEND_URL', 'http://localhost:3000')}/microsoft/callback"]) + expect(url).not_to match(/(?:\?|&)prompt=/) # Validate state parameter exists and can be decoded back to the account expect(params['state']).to be_present From 7acbe8b3ff154ceee410462cf25c213501747c27 Mon Sep 17 00:00:00 2001 From: JoseGrdar <168092473+JoseGrdar@users.noreply.github.com> Date: Wed, 3 Jun 2026 09:20:21 +0200 Subject: [PATCH 3/3] fix(whatsapp): truncate location fallback_title to 255 chars to avoid silent message drop (#14517) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `Whatsapp::IncomingMessageBaseService#attach_location` builds a `fallback_title` by concatenating `location['name']` and `location['address']` with no length cap, then stores it directly into `Attachment#fallback_title`. `ApplicationRecord` enforces a generic 255-character limit on string columns, so any WhatsApp location whose `"#{name}, #{address}"` exceeds 255 chars (a common case for Google Places that include a long full address) raises `ActiveRecord::RecordInvalid` deep inside the Sidekiq job. The message and attachment INSERTs are part of the same transaction, so the whole thing rolls back. Sidekiq retries once; the retry dedup-skips the wamid silently and exits without an error. **Result: the message is irrecoverably lost — no row in `messages`, no entry in the UI, no outgoing webhook, no clue for the operator.** Confirmed in `v4.13.0`, `v4.14.0`, and `develop` (commit `f33e469`, 2026-05-20). No upstream issue found before opening this PR. ## How to reproduce 1. From WhatsApp, share a Google Place whose `name + ", " + address` is > 255 chars. The Spanish business address `Gremi de Fusters, 33, Edificio VIP Asima, Piso 2, Local 2, Norte, 07009 Polígon industrial de Son Castelló, Illes Balears, España` (132 chars) used as both `name` and `address` is enough. 2. Sidekiq logs: ``` ERROR ActiveRecord::RecordInvalid: Validation failed: Attachments fallback title is too long (maximum is 255 characters) ``` 3. The `messages` table has no row. The conversation UI shows nothing for that timestamp. 4. The first retry "Performed" successfully but creates nothing — the dedup-by-source-id silently swallows the failure. ## Fix Cap the existing concatenated title at 255 chars via `.first(255)`. Minimal change, no behavioural difference for any message shorter than the limit, prevents the silent data loss for any longer ones. ```diff - location_name = location['name'] ? "#{location['name']}, #{location['address']}" : '' + location_name = (location['name'] ? "#{location['name']}, #{location['address']}" : '').first(255) ``` ## Alternatives considered - **Increase the validation limit on `Attachment#fallback_title`**: more invasive; would touch other inbound channels and possibly require a DB column change. - **Use `name` alone (no concat)**: cleaner semantically (in many real payloads `name == address`), but changes user-visible behaviour. Left as a follow-up if desired. - **Truncate with ellipsis**: cosmetic only; deferred. This PR is intentionally minimal so it can be merged on its own. --------- Co-authored-by: Sony Mathew Co-authored-by: Sony Mathew <2040199+sony-mathew@users.noreply.github.com> --- .../whatsapp/incoming_message_base_service.rb | 2 +- .../whatsapp/incoming_message_service_spec.rb | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/app/services/whatsapp/incoming_message_base_service.rb b/app/services/whatsapp/incoming_message_base_service.rb index 722ac3e4d..9e0720f74 100644 --- a/app/services/whatsapp/incoming_message_base_service.rb +++ b/app/services/whatsapp/incoming_message_base_service.rb @@ -147,7 +147,7 @@ class Whatsapp::IncomingMessageBaseService def attach_location location = messages_data.first['location'] - location_name = location['name'] ? "#{location['name']}, #{location['address']}" : '' + location_name = (location['name'] ? "#{location['name']}, #{location['address']}" : '').first(255) @message.attachments.new( account_id: @message.account_id, file_type: file_content_type(message_type), diff --git a/spec/services/whatsapp/incoming_message_service_spec.rb b/spec/services/whatsapp/incoming_message_service_spec.rb index fe6b179c1..430aa1561 100644 --- a/spec/services/whatsapp/incoming_message_service_spec.rb +++ b/spec/services/whatsapp/incoming_message_service_spec.rb @@ -381,6 +381,32 @@ describe Whatsapp::IncomingMessageService do expect(location_attachment.coordinates_long).to eq(-122.3895553) expect(location_attachment.external_url).to eq('http://location_url.test') end + + it 'truncates long fallback titles to avoid dropping location messages' do + long_place_name = [ + 'Gremi de Fusters, 33, Edificio VIP Asima, Piso 2, Local 2, Norte', + '07009 Poligon industrial de Son Castello, Illes Balears, Espana' + ].join(', ') + source_id = 'wamid.long-location-fallback-title' + params = { + 'contacts' => [{ 'profile' => { 'name' => 'Sojan Jose' }, 'wa_id' => '2423423243' }], + 'messages' => [{ 'from' => '2423423243', 'id' => source_id, + 'location' => { 'id' => 'b1c68f38-8734-4ad3-b4a1-ef0c10d683', + :address => long_place_name, + :latitude => 37.7893768, + :longitude => -122.3895553, + :name => long_place_name, + :url => 'http://location_url.test' }, + 'timestamp' => '1633034394', 'type' => 'location' }] + }.with_indifferent_access + + expect { described_class.new(inbox: whatsapp_channel.inbox, params: params).perform } + .to change { Message.where(source_id: source_id).count }.from(0).to(1) + + location_attachment = Message.find_by!(source_id: source_id).attachments.first + expect(location_attachment.fallback_title).to eq("#{long_place_name}, #{long_place_name}".first(255)) + expect(location_attachment.fallback_title.length).to eq(255) + end end context 'when valid contact message params' do