From f49694f8c91e37f55cf11e6af884e7f2500c13d0 Mon Sep 17 00:00:00 2001
From: Shivam Mishra <scm.mymail@gmail.com>
Date: Thu, 28 May 2026 16:40:46 +0530
Subject: [PATCH] feat: harden hmac binding

---
 .../api/v1/inboxes/contacts_controller.rb     | 21 +++++++-
 .../api/v1/inbox/contacts_controller_spec.rb  | 48 +++++++++++++++++++
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/app/controllers/public/api/v1/inboxes/contacts_controller.rb b/app/controllers/public/api/v1/inboxes/contacts_controller.rb
index 835c2596b..a78d2d465 100644
--- a/app/controllers/public/api/v1/inboxes/contacts_controller.rb
+++ b/app/controllers/public/api/v1/inboxes/contacts_controller.rb
@@ -30,8 +30,9 @@ class Public::Api::V1::Inboxes::ContactsController < Public::Api::V1::InboxesCon
   def process_hmac
     return if params[:identifier_hash].blank? && !@inbox_channel.hmac_mandatory
     raise StandardError, 'HMAC failed: Invalid Identifier Hash Provided' unless valid_hmac?
+    raise StandardError, 'HMAC failed: Identifier does not match contact' if hmac_identifier_conflict?
 
-    @contact_inbox.update(hmac_verified: true) if @contact_inbox.present?
+    @contact_inbox.update(hmac_verified: true) if @contact_inbox.present? && hmac_identifier_matches?
   end
 
   def valid_hmac?
@@ -42,6 +43,24 @@ class Public::Api::V1::Inboxes::ContactsController < Public::Api::V1::InboxesCon
     )
   end
 
+  # Rejects the request when the selected contact is already bound to a different identifier,
+  # so a valid (identifier, identifier_hash) pair cannot be replayed against another contact.
+  def hmac_identifier_conflict?
+    return false if @contact_inbox.blank?
+
+    contact_identifier = @contact_inbox.contact.identifier.to_s
+    contact_identifier.present? &&
+      !ActiveSupport::SecurityUtils.secure_compare(contact_identifier, params[:identifier].to_s)
+  end
+
+  # Only grants verified trust when the proven identifier matches the contact's own identifier.
+  # A blank contact identifier never grants trust, preventing replay onto anonymous contacts.
+  def hmac_identifier_matches?
+    contact_identifier = @contact_inbox.contact.identifier.to_s
+    contact_identifier.present? &&
+      ActiveSupport::SecurityUtils.secure_compare(contact_identifier, params[:identifier].to_s)
+  end
+
   def permitted_params
     params.permit(:identifier, :identifier_hash, :email, :name, :avatar_url, :phone_number, custom_attributes: {})
   end
diff --git a/spec/controllers/public/api/v1/inbox/contacts_controller_spec.rb b/spec/controllers/public/api/v1/inbox/contacts_controller_spec.rb
index 244d31c4d..83e99a829 100644
--- a/spec/controllers/public/api/v1/inbox/contacts_controller_spec.rb
+++ b/spec/controllers/public/api/v1/inbox/contacts_controller_spec.rb
@@ -48,4 +48,52 @@ RSpec.describe 'Public Inbox Contacts API', type: :request do
       expect(data['name']).to eq 'John Smith'
     end
   end
+
+  describe 'HMAC identifier binding' do
+    let!(:owner_contact) { create(:contact, account: api_channel.account, identifier: 'owner-identifier') }
+    let!(:owner_inbox) { create(:contact_inbox, contact: owner_contact, inbox: api_channel.inbox) }
+    let!(:victim_contact) { create(:contact, account: api_channel.account, identifier: 'victim-identifier', name: 'Victim') }
+    let!(:victim_inbox) { create(:contact_inbox, contact: victim_contact, inbox: api_channel.inbox) }
+    let(:valid_owner_hash) { OpenSSL::HMAC.hexdigest('sha256', api_channel.hmac_token, 'owner-identifier') }
+
+    it 'does not promote the victim contact inbox to hmac_verified when a valid pair is replayed against the victim source_id' do
+      begin
+        get "/public/api/v1/inboxes/#{api_channel.identifier}/contacts/#{victim_inbox.source_id}",
+            params: { identifier: 'owner-identifier', identifier_hash: valid_owner_hash }
+      rescue StandardError
+        # the fix fails closed by raising; the security invariant is asserted below
+      end
+
+      expect(victim_inbox.reload.hmac_verified).to be(false)
+    end
+
+    it 'does not mutate the victim contact when a valid pair is replayed against the victim source_id' do
+      begin
+        patch "/public/api/v1/inboxes/#{api_channel.identifier}/contacts/#{victim_inbox.source_id}",
+              params: { identifier: 'owner-identifier', identifier_hash: valid_owner_hash, name: 'Hacked' }
+      rescue StandardError
+        # the fix fails closed by raising; the security invariant is asserted below
+      end
+
+      expect(victim_contact.reload.name).to eq('Victim')
+    end
+
+    it 'still verifies the contact inbox when the owner presents a matching identifier pair' do
+      get "/public/api/v1/inboxes/#{api_channel.identifier}/contacts/#{owner_inbox.source_id}",
+          params: { identifier: 'owner-identifier', identifier_hash: valid_owner_hash }
+
+      expect(response).to have_http_status(:success)
+      expect(owner_inbox.reload.hmac_verified).to be(true)
+    end
+
+    it 'does not verify an anonymous contact inbox when a valid pair is replayed against its source_id' do
+      anonymous_contact = create(:contact, account: api_channel.account)
+      anonymous_inbox = create(:contact_inbox, contact: anonymous_contact, inbox: api_channel.inbox)
+
+      get "/public/api/v1/inboxes/#{api_channel.identifier}/contacts/#{anonymous_inbox.source_id}",
+          params: { identifier: 'owner-identifier', identifier_hash: valid_owner_hash }
+
+      expect(anonymous_inbox.reload.hmac_verified).to be(false)
+    end
+  end
 end