From b0aa844a3266604c589dff0943399a8ad31bf656 Mon Sep 17 00:00:00 2001
From: ramalau <71857041+ramalau0@users.noreply.github.com>
Date: Mon, 27 Apr 2026 21:44:51 +0200
Subject: [PATCH] fix(portals): handle integer blob_id in process_attached_logo
 without 500 (#14274)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Updating portal settings (name, header text, page title, homepage link)
on a portal that already has a logo attached returns 500. The error is
\`NoMethodError: undefined method 'valid_encoding?' for an instance of
Integer\`. The fix is a two-character change in
\`process_attached_logo\`.

Closes #13300

## Root cause

\`ActiveStorage::Blob.find_signed\` expects a signed ID string (e.g.
\`"eyJfcmFpbH..."\`). Internally it calls \`valid_encoding?\` on the
argument to validate the signature payload — a method that exists on
\`String\` but not \`Integer\`.

When a portal already has a logo, the frontend includes the blob's raw
database integer ID (e.g. \`blob_id: 170\`) in the update request
payload. The controller passes this integer directly to \`find_signed\`,
which immediately raises \`NoMethodError\` before any database query is
made.

\`\`\`ruby
# before, crashes when blob_id is an Integer
blob_id = params[:blob_id]
blob = ActiveStorage::Blob.find_signed(blob_id)  # NoMethodError here
@portal.logo.attach(blob)
\`\`\`

## What changed

\`\`\`ruby
# after, safe for any input type
blob = ActiveStorage::Blob.find_signed(params[:blob_id].to_s)
@portal.logo.attach(blob) if blob
\`\`\`

\`.to_s\` on an Integer produces a plain decimal string (\`"170"\`),
which is not a valid signed ID. \`find_signed\` returns \`nil\` for any
invalid signature rather than raising, so the nil guard prevents a
broken \`attach\` call. The existing logo remains attached and the
settings update succeeds.

## Trade-offs considered

| Option | Decision |
|---|---|
| \`find(blob_id)\` when input is an Integer | Bypasses signature
verification — any authenticated user knowing a blob ID could attach
arbitrary files to a portal. Security risk. Rejected. |
| Raise a 422 for non-string blob_id | Overly strict — the frontend
sending an integer is pre-existing behaviour this PR shouldn't break. |
| Silently no-op for invalid blob_id (chosen) | Correct product
behaviour: if no valid signed upload is provided, leave the logo
unchanged. The settings update still succeeds. |

## Known limitation

The correct long-term fix is also on the frontend: it should only send
\`blob_id\` when attaching a **new** upload (using the signed ID from
the direct-upload flow), not when re-submitting the existing logo's raw
database integer ID. This PR makes the server robust against the current
frontend behaviour without requiring a coordinated frontend change.

## How to reproduce

1. Create a Help Center portal and upload a logo
2. Update any text field via \`PUT /api/v1/accounts/:id/portals/:slug\`
while including \`blob_id: <integer>\` in the payload
3. Observe 500 with \`NoMethodError: undefined method 'valid_encoding?'
for an instance of Integer\`

After this fix, the request returns 200, settings are updated, and the
existing logo is preserved.

Co-authored-by: Ramalau Debeila <rdebeila@datacentrix.co.za>
---
 .../api/v1/accounts/portals_controller.rb            |  5 ++---
 .../api/v1/accounts/portals_controller_spec.rb       | 12 ++++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/app/controllers/api/v1/accounts/portals_controller.rb b/app/controllers/api/v1/accounts/portals_controller.rb
index 972b244fa..ade83d8ec 100644
--- a/app/controllers/api/v1/accounts/portals_controller.rb
+++ b/app/controllers/api/v1/accounts/portals_controller.rb
@@ -61,9 +61,8 @@ class Api::V1::Accounts::PortalsController < Api::V1::Accounts::BaseController
   end
 
   def process_attached_logo
-    blob_id = params[:blob_id]
-    blob = ActiveStorage::Blob.find_signed(blob_id)
-    @portal.logo.attach(blob)
+    blob = ActiveStorage::Blob.find_signed(params[:blob_id].to_s)
+    @portal.logo.attach(blob) if blob
   end
 
   private
diff --git a/spec/controllers/api/v1/accounts/portals_controller_spec.rb b/spec/controllers/api/v1/accounts/portals_controller_spec.rb
index 19bc795c3..9c780c00a 100644
--- a/spec/controllers/api/v1/accounts/portals_controller_spec.rb
+++ b/spec/controllers/api/v1/accounts/portals_controller_spec.rb
@@ -180,6 +180,18 @@ RSpec.describe 'Api::V1::Accounts::Portals', type: :request do
         expect(portal.archived).to be_truthy
       end
 
+      it 'does not raise when blob_id is an integer (existing logo re-sent by frontend)' do
+        portal.logo.attach(io: Rails.root.join('spec/assets/avatar.png').open, filename: 'avatar.png', content_type: 'image/png')
+
+        put "/api/v1/accounts/#{account.id}/portals/#{portal.slug}",
+            params: { portal: { name: 'updated_name' }, blob_id: portal.logo.blob.id },
+            headers: admin.create_new_auth_token
+
+        expect(response).to have_http_status(:success)
+        expect(response.parsed_body['name']).to eq('updated_name')
+        expect(portal.reload.logo).to be_attached
+      end
+
       it 'clears associated web widget when inbox selection is blank' do
         web_widget_inbox = create(:inbox, account: account)
         portal.update!(channel_web_widget: web_widget_inbox.channel)