From 201b97bd16d620e0f1b229dc906e96553717ce9d Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Wed, 7 Feb 2024 16:34:32 +0000
Subject: [PATCH] Raise exception on CSRF failure

This was previously enabled in PR #1375 but then reverted in PR #1383
when it was discovered that enabling it had silently broken the ability
for Release API clients to create deployment records because they don't
(and shouldn't need to) send the CSRF token in the request.

I'm avoiding the problem seen in PR #1375 by skipping forgery protection
if we detect a Bearer token in the Authorization HTTP Header. I'm doing
this in the same way that GDS::SSO checks whether the `gds_sso` Warden
strategy is valid[1] or whether it needs to use the `gds_bearer_token`
strategy instead[2] so I'm fairly confident this is good enough to
distinguish an API request from a browser request.

One of the two new controller tests should fail if we either:

- remove the `with: :exception` option to `protect_from_forgery`
- require the CSRF token when an API client authenticates using a Bearer
  token and creates a deployment

Note that I'm assuming that `Deployments#create` is the only API
endpoint. If it turns out there are more API endpoints then we'll need
to make the same change to those.

I was interested to learn that raising an exception is the default
behaviour in new Rails apps[3] but that we were overriding that default
with our explicit call to `protect_from_forgery` without any arguments.
Adding `protect_from_forgery with: :exception` is the same as removing
`protect_from_forgery` entirely, although I think the former is more
explicit.

[1]: https://github.com/alphagov/gds-sso/blob/8cc1427bfcd3cbfa24904040c8eaccff45434322/lib/gds-sso/warden_config.rb#L38
[2]: https://github.com/alphagov/gds-sso/blob/8cc1427bfcd3cbfa24904040c8eaccff45434322/lib/gds-sso/warden_config.rb#L64
[3]: https://guides.rubyonrails.org/security.html#required-security-token
---
 app/controllers/application_controller.rb     |  2 +-
 app/controllers/deployments_controller.rb     |  6 ++++++
 .../functional/deployments_controller_test.rb | 19 +++++++++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 57b38081c..011728793 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -3,7 +3,7 @@ class ApplicationController < ActionController::Base
 
   before_action :authenticate_user!
 
-  protect_from_forgery
+  protect_from_forgery with: :exception
 
   def error_400
     error 400
diff --git a/app/controllers/deployments_controller.rb b/app/controllers/deployments_controller.rb
index 16f6516a8..6e877b638 100644
--- a/app/controllers/deployments_controller.rb
+++ b/app/controllers/deployments_controller.rb
@@ -1,6 +1,8 @@
 class DeploymentsController < ApplicationController
   class ApplicationConflictError < RuntimeError; end
 
+  skip_forgery_protection if: :api_request_to_create_deployment?
+
   rescue_from ApplicationConflictError do
     head :conflict
   end
@@ -73,4 +75,8 @@ def recent_deployment_params
       :environment_filter,
     )
   end
+
+  def api_request_to_create_deployment?
+    GDS::SSO::ApiAccess.api_call?(request.env) && action_name == "create"
+  end
 end
diff --git a/test/functional/deployments_controller_test.rb b/test/functional/deployments_controller_test.rb
index 3bf2c8ea2..e8343232a 100644
--- a/test/functional/deployments_controller_test.rb
+++ b/test/functional/deployments_controller_test.rb
@@ -37,6 +37,25 @@ class DeploymentsControllerTest < ActionController::TestCase
   end
 
   context "POST create" do
+    context "when forgery protection is enabled" do
+      setup do
+        @controller.allow_forgery_protection = true
+      end
+
+      should "enable forgery protection for non-API requests" do
+        assert_raises(ActionController::InvalidAuthenticityToken) do
+          post :create, params: { repo: "org/app", deployment: { version: "1", environment: "env" } }
+        end
+      end
+
+      should "skip forgery protection for API requests" do
+        request.headers["Authorization"] = "Bearer <token>"
+        post :create, params: { repo: "org/app", deployment: { version: "1", environment: "env" } }
+
+        assert_response :ok
+      end
+    end
+
     setup do
       stub_request(:get, "http://docs.publishing.service.gov.uk/apps.json").to_return(status: 200, body: "", headers: {})
     end