From 68dcbcb7bf2cceb181a9b82d604d2c2829c0c78b Mon Sep 17 00:00:00 2001
From: Claire <claire.github-309c@sitedethib.com>
Date: Wed, 18 Jan 2023 16:47:56 +0100
Subject: Add more specific error messages to HTTP signature verification
 (#21617)

* Return specific error on failure to parse Date header

* Add error message when preferredUsername is not set

* Change error report to be JSON and include more details

* Change error report to differentiate unknown account and failed refresh

* Add tests
---
 .../concerns/signature_verification_spec.rb        | 107 +++++++++++++++++++--
 1 file changed, 100 insertions(+), 7 deletions(-)

(limited to 'spec/controllers/concerns/signature_verification_spec.rb')

diff --git a/spec/controllers/concerns/signature_verification_spec.rb b/spec/controllers/concerns/signature_verification_spec.rb
index 6e73643b4..13655f313 100644
--- a/spec/controllers/concerns/signature_verification_spec.rb
+++ b/spec/controllers/concerns/signature_verification_spec.rb
@@ -16,6 +16,8 @@ describe ApplicationController, type: :controller do
   controller do
     include SignatureVerification
 
+    before_action :require_actor_signature!, only: [:signature_required]
+
     def success
       head 200
     end
@@ -23,10 +25,17 @@ describe ApplicationController, type: :controller do
     def alternative_success
       head 200
     end
+
+    def signature_required
+      head 200
+    end
   end
 
   before do
-    routes.draw { match via: [:get, :post], 'success' => 'anonymous#success' }
+    routes.draw do
+      match via: [:get, :post], 'success' => 'anonymous#success'
+      match via: [:get, :post], 'signature_required' => 'anonymous#signature_required'
+    end
   end
 
   context 'without signature header' do
@@ -118,6 +127,37 @@ describe ApplicationController, type: :controller do
       end
     end
 
+    context 'with request with unparseable Date header' do
+      before do
+        get :success
+
+        fake_request = Request.new(:get, request.url)
+        fake_request.add_headers({ 'Date' => 'wrong date' })
+        fake_request.on_behalf_of(author)
+
+        request.headers.merge!(fake_request.headers)
+      end
+
+      describe '#signed_request?' do
+        it 'returns true' do
+          expect(controller.signed_request?).to be true
+        end
+      end
+
+      describe '#signed_request_account' do
+        it 'returns nil' do
+          expect(controller.signed_request_account).to be_nil
+        end
+      end
+
+      describe '#signature_verification_failure_reason' do
+        it 'contains an error description' do
+          controller.signed_request_account
+          expect(controller.signature_verification_failure_reason[:error]).to eq 'Invalid Date header: not RFC 2616 compliant date: "wrong date"'
+        end
+      end
+    end
+
     context 'with request older than a day' do
       before do
         get :success
@@ -140,6 +180,13 @@ describe ApplicationController, type: :controller do
           expect(controller.signed_request_account).to be_nil
         end
       end
+
+      describe '#signature_verification_failure_reason' do
+        it 'contains an error description' do
+          controller.signed_request_account
+          expect(controller.signature_verification_failure_reason[:error]).to eq 'Signed request date outside acceptable time window'
+        end
+      end
     end
 
     context 'with inaccessible key' do
@@ -171,6 +218,7 @@ describe ApplicationController, type: :controller do
 
     context 'with body' do
       before do
+        allow(controller).to receive(:actor_refresh_key!).and_return(author)
         post :success, body: 'Hello world'
 
         fake_request = Request.new(:post, request.url, body: 'Hello world')
@@ -189,21 +237,66 @@ describe ApplicationController, type: :controller do
         it 'returns an account' do
           expect(controller.signed_request_account).to eq author
         end
+      end
 
-        it 'returns nil when path does not match' do
+      context 'when path does not match' do
+        before do
           request.path = '/alternative-path'
-          expect(controller.signed_request_account).to be_nil
         end
 
-        it 'returns nil when method does not match' do
+        describe '#signed_request_account' do
+          it 'returns nil' do
+            expect(controller.signed_request_account).to be_nil
+          end
+        end
+
+        describe '#signature_verification_failure_reason' do
+          it 'contains an error description' do
+            controller.signed_request_account
+            expect(controller.signature_verification_failure_reason[:error]).to include('using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)')
+            expect(controller.signature_verification_failure_reason[:signed_string]).to include("(request-target): post /alternative-path\n")
+          end
+        end
+      end
+
+      context 'when method does not match' do
+        before do
           get :success
-          expect(controller.signed_request_account).to be_nil
         end
 
-        it 'returns nil when body has been tampered' do
+        describe '#signed_request_account' do
+          it 'returns nil' do
+            expect(controller.signed_request_account).to be_nil
+          end
+        end
+      end
+
+      context 'when body has been tampered' do
+        before do
           post :success, body: 'doo doo doo'
-          expect(controller.signed_request_account).to be_nil
         end
+
+        describe '#signed_request_account' do
+          it 'returns nil when body has been tampered' do
+            expect(controller.signed_request_account).to be_nil
+          end
+        end
+      end
+    end
+  end
+
+  context 'when a signature is required' do
+    before do
+      get :signature_required
+    end
+
+    context 'without signature header' do
+      it 'returns HTTP 401' do
+        expect(response).to have_http_status(401)
+      end
+
+      it 'returns an error' do
+        expect(Oj.load(response.body)['error']).to eq 'Request not signed'
       end
     end
   end
-- 
cgit