Skip to content

Commit

Permalink
Raise exception on CSRF failure
Browse files Browse the repository at this point in the history
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
  • Loading branch information
chrisroos authored and AgaDufrat committed May 17, 2024
1 parent 97e6717 commit 201b97b
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 1 deletion.
2 changes: 1 addition & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/deployments_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
class DeploymentsController < ApplicationController
class ApplicationConflictError < RuntimeError; end

skip_forgery_protection if: :api_request_to_create_deployment?

Check failure

Code scanning / CodeQL

CSRF protection weakened or disabled High

Potential CSRF vulnerability due to forgery protection being disabled or weakened.

rescue_from ApplicationConflictError do
head :conflict
end
Expand Down Expand Up @@ -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
19 changes: 19 additions & 0 deletions test/functional/deployments_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 201b97b

Please sign in to comment.