Action Mailbox Mandrillインgressで署名ヘッダー欠落時にNoMethodErrorが発生する問題を修正

Action MailboxのMandrillインgressにおいて、X-Mandrill-Signatureヘッダーが欠落したリクエストがNoMethodErrorを引き起こしていた問題を修正しました。署名の存在チェックを追加することで、ヘッダー未送信のリクエストも偽造署名と同様に401 Unauthorizedで確実に拒否されるようになります。

背景

Mandrillインgressの認証ロジックは、リクエストの正当性をX-Mandrill-Signatureヘッダーと期待されるHMACの比較によって検証しています。しかし、このヘッダーが存在しない場合に問題が発生していました。

authenticated?メソッドはActiveSupport::SecurityUtils.secure_compareを呼び出す前に署名の存在チェックを行っておらず、ヘッダーが欠落しているとgiven_signatureがnilになります。secure_compareは内部で引数の.bytesizeを呼び出すため、nilが渡された際に以下のエラーが発生していました。

NoMethodError: undefined method `bytesize' for nil
    activesupport/lib/active_support/security_utils.rb:34:in `secure_compare'
    actionmailbox/app/controllers/action_mailbox/ingresses/mandrill/inbound_emails_controller.rb:69:in `authenticated?'

この挙動は#57329として報告されており、期待される動作（401 Unauthorizedを返す）と実際の動作（例外の発生）の乖離が問題の本質です。

技術的な変更

修正はauthenticated?メソッドへの1行の変更で完結しており、署名の存在確認を比較処理の前置条件として追加しています。

変更前:

def authenticated?
  ActiveSupport::SecurityUtils.secure_compare given_signature, expected_signature
end

変更後:

def authenticated?
  given_signature.present? && ActiveSupport::SecurityUtils.secure_compare(given_signature, expected_signature)
end

given_signature.present? によるガード節が追加され、nilや空文字列の場合は&&の短絡評価によりsecure_compareが呼ばれることなくfalseを返します。これにより、署名欠落のリクエストは認証失敗として扱われ、コントローラーが401 Unauthorizedを返す通常のフローに乗ります。

テスト側には、署名ヘッダーなしでリクエストを送信するリグレッションテストが追加されています。

test "rejecting an inbound email from Mandrill without a signature" do
  assert_no_difference -> { ActionMailbox::InboundEmail.count } do
    post rails_mandrill_inbound_emails_url, params: { mandrill_events: @events }
  end

  assert_response :unauthorized
end

このテストは「署名なしのリクエストでInboundEmailレコードが作成されないこと」と「レスポンスが401であること」の両方をアサートしており、再発防止の観点から適切なカバレッジとなっています。

設計判断

present?による存在チェックをsecure_compareの呼び出し前に置く という設計が選ばれています。

nilガードとしてgiven_signature && ...ではなくgiven_signature.present? && ...を使用している点が注目されます。present?はnilだけでなく空文字列（""）もfalsy扱いにするため、空の署名ヘッダーが送信された場合にも同様に認証を拒否できます。空の署名をsecure_compareに渡した場合は例外は発生しないものの、認証通過の余地を残さないという一貫した防御的実装になっています。

また、secure_compareの呼び出しに括弧を追加する形で引数リストが明示化されており、コードの読みやすさも改善されています。

まとめ

本修正は1行の変更ながら、例外による500エラーを意図した401エラーに切り替えるセキュリティ上重要な修正です。present?を使った防御的な存在チェックにより、署名ヘッダーの欠落・空文字・nilのいずれのケースでも認証フローが正しく機能するようになりました。