fix(portals): handle integer blob_id in process_attached_logo without 500 (#14274)

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>
This commit is contained in:
ramalau
2026-04-28 01:14:51 +05:30
committed by GitHub
co-authored by Ramalau Debeila
parent c8e551820b
commit b0aa844a32
2 changed files with 14 additions and 3 deletions
@@ -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
@@ -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)