diff --git a/app/controllers/shopify/callbacks_controller.rb b/app/controllers/shopify/callbacks_controller.rb index a29745550..d5ae6d9f1 100644 --- a/app/controllers/shopify/callbacks_controller.rb +++ b/app/controllers/shopify/callbacks_controller.rb @@ -9,7 +9,6 @@ class Shopify::CallbacksController < ApplicationController end rescue StandardError => e Rails.logger.error("Shopify callback error: #{e.message}") - Rails.logger.error("Shopify callback error backtrace: #{e.backtrace.first(10).join("\n")}") redirect_to error_redirect_url end @@ -38,12 +37,6 @@ class Shopify::CallbacksController < ApplicationController # Shopify will reject any attempt to exchange a code at a different shop's endpoint. @response = oauth_client.auth_code.get_token(params[:code], redirect_uri: redirect_callback_uri) - Rails.logger.info("OAuth Debug - Response present: #{@response.present?}") - Rails.logger.info("OAuth Debug - Response class: #{@response.class}") - Rails.logger.info("OAuth Debug - Parsed body: #{parsed_body.inspect}") - - raise StandardError, 'Failed to parse OAuth response' if parsed_body.blank? - token_key = SecureRandom.hex(16) pending_data = { access_token: parsed_body['access_token'], @@ -71,11 +64,10 @@ class Shopify::CallbacksController < ApplicationController def parsed_body @parsed_body ||= begin parsed = @response.response.parsed - # SnakyHash may not behave like a regular hash for all operations - # Convert to a regular hash to ensure compatibility + # Handle both SnakyHash (production) and regular Hash (tests) { - 'access_token' => parsed.access_token || parsed['access_token'], - 'scope' => parsed.scope || parsed['scope'] + 'access_token' => parsed.respond_to?(:access_token) ? parsed.access_token : parsed['access_token'], + 'scope' => parsed.respond_to?(:scope) ? parsed.scope : parsed['scope'] } end end @@ -141,15 +133,6 @@ class Shopify::CallbacksController < ApplicationController # Compute HMAC-SHA256 computed_hmac = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('SHA256'), client_secret, query_string) - # Debug logging - Rails.logger.info("HMAC Debug - Query params: #{query_params.inspect}") - Rails.logger.info("HMAC Debug - Query string for validation: #{query_string}") - Rails.logger.info("HMAC Debug - Shopify HMAC: #{hmac}") - Rails.logger.info("HMAC Debug - Computed HMAC: #{computed_hmac}") - Rails.logger.info("HMAC Debug - Client secret present: #{client_secret.present?}") - Rails.logger.info("HMAC Debug - Client secret length: #{client_secret&.length}") - Rails.logger.info("HMAC Debug - Client secret starts with: #{client_secret&.first(10)}") - ActiveSupport::SecurityUtils.secure_compare(computed_hmac, hmac) end end diff --git a/spec/controllers/shopify/callbacks_controller_spec.rb b/spec/controllers/shopify/callbacks_controller_spec.rb index 745c1a02e..37d688522 100644 --- a/spec/controllers/shopify/callbacks_controller_spec.rb +++ b/spec/controllers/shopify/callbacks_controller_spec.rb @@ -5,6 +5,7 @@ RSpec.describe Shopify::CallbacksController, type: :request do let(:code) { SecureRandom.hex(10) } let(:state) { SecureRandom.hex(10) } let(:shop) { 'my-store.myshopify.com' } + let(:client_secret) { 'test_secret_key_1234567890' } let(:frontend_url) { 'http://www.example.com' } let(:shopify_redirect_uri) { "#{frontend_url}/app/accounts/#{account.id}/settings/integrations/shopify" } let(:oauth_client) { instance_double(OAuth2::Client) } @@ -17,6 +18,12 @@ RSpec.describe Shopify::CallbacksController, type: :request do ) end + # Helper to compute HMAC for test requests (matches Shopify's algorithm) + def compute_hmac(params, secret) + query_string = params.except(:hmac).sort.map { |k, v| "#{k}=#{v}" }.join('&') + OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('SHA256'), secret, query_string) + end + describe 'GET /shopify/callback' do let(:access_token) { SecureRandom.hex(10) } let(:response_body) do @@ -36,6 +43,7 @@ RSpec.describe Shopify::CallbacksController, type: :request do controller = original.call(*args) allow(controller).to receive(:verify_shopify_token).and_return(account.id) allow(controller).to receive(:oauth_client).and_return(oauth_client) + allow(controller).to receive(:client_secret).and_return(client_secret) controller end allow(Account).to receive(:find).and_return(account) @@ -57,8 +65,11 @@ RSpec.describe Shopify::CallbacksController, type: :request do end it 'creates a new integration hook' do + params = { code: code, state: state, shop: shop } + params[:hmac] = compute_hmac(params, client_secret) + expect do - get shopify_callback_path, params: { code: code, state: state, shop: shop } + get shopify_callback_path, params: params end.to change(Integrations::Hook, :count).by(1) hook = Integrations::Hook.last @@ -82,7 +93,10 @@ RSpec.describe Shopify::CallbacksController, type: :request do end it 'redirects to the shopify_redirect_uri with error' do - get shopify_callback_path, params: { state: state, shop: shop } + params = { state: state, shop: shop } + params[:hmac] = compute_hmac(params, client_secret) + + get shopify_callback_path, params: params expect(response).to redirect_to("#{shopify_redirect_uri}?error=true") end end @@ -104,7 +118,10 @@ RSpec.describe Shopify::CallbacksController, type: :request do end it 'redirects to the shopify_redirect_uri with error' do - get shopify_callback_path, params: { code: code, state: state, shop: shop } + params = { code: code, state: state, shop: shop } + params[:hmac] = compute_hmac(params, client_secret) + + get shopify_callback_path, params: params expect(response).to redirect_to("#{shopify_redirect_uri}?error=true") end end @@ -115,13 +132,17 @@ RSpec.describe Shopify::CallbacksController, type: :request do # Explicit class name and any_instance required for parallel CI stability allow_any_instance_of(Shopify::CallbacksController).to receive(:verify_shopify_token).and_return(nil) allow_any_instance_of(Shopify::CallbacksController).to receive(:oauth_client).and_return(oauth_client) + allow_any_instance_of(Shopify::CallbacksController).to receive(:client_secret).and_return(client_secret) # rubocop:enable RSpec/AnyInstance, RSpec/DescribedClass allow(oauth_client).to receive(:auth_code).and_return(auth_code_strategy) allow(auth_code_strategy).to receive(:get_token).and_return(token_response) end it 'handles as Shopify-initiated install and redirects to login with pending install token' do - get shopify_callback_path, params: { code: code, state: state, shop: shop } + params = { code: code, state: state, shop: shop } + params[:hmac] = compute_hmac(params, client_secret) + + get shopify_callback_path, params: params expect(response).to redirect_to(%r{#{Regexp.escape(frontend_url)}/app/login\?redirect_url=}) expect(CGI.unescape(response.location)).to include('shopify_pending_install=') end