From a8887895e0fa2bce365c0c14d995dafdcdee179b Mon Sep 17 00:00:00 2001
From: lurz <lurz@umich.edu>
Date: Fri, 22 Mar 2024 00:29:35 -0700
Subject: [PATCH 1/2] Fix bugs for error response in the form_post and error
 view

Bug
When an error is rendered with the `form_post.html.erb` view, an empty
form is posted instead of the correct error and error_description.

Root cause
The form_post view used the body of `@authorize_response`. However,
the `redirect_or_render` could be called with `authorization.deny` or
`pre_auth.error_response`. In this case, `@authorize_response` would
only contain an empty body.

Fix
Instead of using `@authorize_response`, we pass in a local variable
`auth` to the `form_post.html.erb`, which corresponding to the auth
object passed into the redirect_or_render function. Tests are backfilled
to the authorizations controller spec.

During the test phase, I discovered error.html.erb always renders an
incorrect error description. It turns out that `respond_to(:error_respon
se)` always return false in the view. I changed it to use
`local_assigns` as the correct condition.
---
 .../doorkeeper/authorizations_controller.rb   |   2 +-
 .../doorkeeper/authorizations/error.html.erb  |   2 +-
 .../authorizations/form_post.html.erb         |   2 +-
 .../authorizations_controller_spec.rb         | 135 +++++++++++++++++-
 4 files changed, 137 insertions(+), 4 deletions(-)

diff --git a/app/controllers/doorkeeper/authorizations_controller.rb b/app/controllers/doorkeeper/authorizations_controller.rb
index 7601a5cdd..9403db433 100644
--- a/app/controllers/doorkeeper/authorizations_controller.rb
+++ b/app/controllers/doorkeeper/authorizations_controller.rb
@@ -77,7 +77,7 @@ def redirect_or_render(auth)
             )
           end
         elsif pre_auth.form_post_response?
-          render :form_post
+          render :form_post, locals: { auth: auth }
         else
           redirect_to auth.redirect_uri, allow_other_host: true
         end
diff --git a/app/views/doorkeeper/authorizations/error.html.erb b/app/views/doorkeeper/authorizations/error.html.erb
index 7198ec3ac..3f11f1c6c 100644
--- a/app/views/doorkeeper/authorizations/error.html.erb
+++ b/app/views/doorkeeper/authorizations/error.html.erb
@@ -4,6 +4,6 @@
 
 <main role="main">
   <pre>
-    <%= (respond_to?(:error_response) ? error_response : @pre_auth.error_response).body[:error_description] %>
+    <%= (local_assigns[:error_response] ? error_response : @pre_auth.error_response).body[:error_description] %>
   </pre>
 </main>
diff --git a/app/views/doorkeeper/authorizations/form_post.html.erb b/app/views/doorkeeper/authorizations/form_post.html.erb
index 3dfcfadb6..179a7846b 100644
--- a/app/views/doorkeeper/authorizations/form_post.html.erb
+++ b/app/views/doorkeeper/authorizations/form_post.html.erb
@@ -3,7 +3,7 @@
 </header>
 
 <%= form_tag @pre_auth.redirect_uri, method: :post, name: :redirect_form, authenticity_token: false do %>
-  <% @authorize_response&.body&.compact&.each do |key, value| %>
+  <% auth.body.compact.each do |key, value| %>
     <%= hidden_field_tag key, value %>
   <% end %>
 <% end %>
diff --git a/spec/controllers/authorizations_controller_spec.rb b/spec/controllers/authorizations_controller_spec.rb
index f107b0711..f475191a2 100644
--- a/spec/controllers/authorizations_controller_spec.rb
+++ b/spec/controllers/authorizations_controller_spec.rb
@@ -335,6 +335,10 @@ def query_params
       it "does not issue any access token" do
         expect(Doorkeeper::AccessToken.all).to be_empty
       end
+
+      it "includes the error in the redirect post" do
+        expect(response.body).to include("invalid_scope")
+      end
     end
   end
 
@@ -1100,6 +1104,28 @@ def query_params
       end
     end
 
+    context "invalid scope with form_post response mode" do
+      before do
+        default_scopes_exist :public
+        get :new, params: {
+          client_id: client.uid,
+          response_type: "token",
+          scope: "invalid",
+          redirect_uri: client.redirect_uri,
+          state: "return-this",
+          response_mode: "form_post",
+        }
+      end
+
+      it "renders the form_post page" do
+        expect(response.status).to eq(200)
+      end
+
+      it "includes the error in the redirect post" do
+        expect(response.body).to include("invalid_scope")
+      end
+    end
+
     context "invalid redirect_uri" do
       before do
         default_scopes_exist :public
@@ -1296,10 +1322,117 @@ def query_params
     end
   end
 
+  describe "DELETE #destroy" do
+    context "without form_post response mode" do
+      before do
+        delete :destroy, params: {
+          client_id: client.uid,
+          response_type: "token",
+          redirect_uri: client.redirect_uri,
+        }
+      end
+
+      it "redirects" do
+        expect(response).to be_redirect
+      end
+
+      it "redirects to client redirect uri" do
+        expect(response.location).to match(/^#{client.redirect_uri}/)
+      end
+
+      it "includes error in fragment" do
+        expect(response.query_params["error"]).to eq("access_denied")
+      end
+    end
+
+    context "with form_post response mode" do
+      before do
+        delete :destroy, params: {
+          client_id: client.uid,
+          response_type: "token",
+          redirect_uri: client.redirect_uri,
+          response_mode: "form_post",
+        }
+      end
+
+      it "redirects after authorization" do
+        expect(response.status).to eq(200)
+      end
+
+      it "includes the error in the redirect post" do
+        expect(response.body).to include("access_denied")
+      end
+    end
+
+    context "with invalid params" do
+      before do
+        delete :destroy, params: {
+          client_id: client.uid,
+          response_type: "blabla",
+          redirect_uri: client.redirect_uri,
+        }
+      end
+
+      it "renders the error page correctly" do
+        expect(response.status).to eq(200)
+      end
+
+      it "includes the error in the page" do
+        expect(response.body).to include(
+          translated_error_message(:unsupported_grant_type),
+        )
+      end
+    end
+  end
+
   describe "DELETE #destroy in API mode" do
+    before do
+      allow(Doorkeeper.config).to receive(:api_only).and_return(true)
+    end
+
+    context "without form_post response mode" do
+      before do
+        delete :destroy, params: {
+          client_id: client.uid,
+          response_type: "token",
+          redirect_uri: client.redirect_uri,
+        }
+      end
+
+      it "renders bad request" do
+        expect(response).to have_http_status(:bad_request)
+      end
+
+      it "includes access_denied in the redirect uri" do
+        expect(response_json_body["redirect_uri"].match(/error=(\w+)&?/)[1]).to eq("access_denied")
+      end
+    end
+
+    context "with form_post response mode" do
+      before do
+        delete :destroy, params: {
+          client_id: client.uid,
+          response_type: "token",
+          redirect_uri: client.redirect_uri,
+          response_mode: "form_post",
+        }
+      end
+
+      it "renders bad request" do
+        expect(response).to have_http_status(:bad_request)
+      end
+
+      it "includes the correct redirect uri" do
+        expect(response_json_body["redirect_uri"]).to eq(client.redirect_uri)
+      end
+
+      it "includes access_denied in the body" do
+        expect(response_json_body["body"]["error"]).to eq("access_denied")
+      end
+    end
+
     context "with invalid params" do
       before do
-        allow(Doorkeeper.config).to receive(:api_only).and_return(true)
         delete :destroy, params: {
           client_id: client.uid,
           response_type: "blabla",

From 4cea951dacbdf20e3400062ab6d021b0f2aa3574 Mon Sep 17 00:00:00 2001
From: lurz <lurz@umich.edu>
Date: Fri, 22 Mar 2024 10:08:11 -0700
Subject: [PATCH 2/2] Add CHANGELOG

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f77baf81d..288c50359 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,7 @@ User-visible changes worth mentioning.
 
 - [#1696] Add missing `#issued_token` method to `OAuth::TokenResponse`
 - [#1697] Allow a TokenResponse body to be customized.
+- [#1702] Fix bugs for error response in the form_post and error view
 
 ## 5.6.9