Raise exception on CSRF failure #1394
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hi @theseanything and @sengi. I spent some time looking into this after @theseanything suggested that we might need to make a change to gds-sso to handle the problem you were seeing. If I've understood the problem correctly then I think this PR demonstrates that we don't need to make a change to gds-sso but I'd appreciate getting your thoughts on it. |
There was a problem hiding this comment.
@chrisroos thanks for the PR! I think this would be okay to go in for now. This implementation might not scale well if we find more apps and places with a mix of APIs and HTML endpoints - (in which case might be useful to have a helper in GDS::SSO).
|
@chrisroos @theseanything is this still needed or being worked on? Can we close it or make it a draft? |
This issue still needs to be resolved, as I said this implementation is fine to fix the immediate problem. @chrisroos is no longer able to work on this. |
|
Created a Trello card for this and reverting to draft. |
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
AgaDufrat
force-pushed
the
raise-exception-on-forged-requests
branch
from
fce2d50 to
201b97b
Compare
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_ssoWarden strategy is valid or whether it needs to use thegds_bearer_tokenstrategy instead 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:
with: :exceptionoption toprotect_from_forgeryNote that I'm assuming that
Deployments#createis 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 but that we were overriding that default with our explicit call to
protect_from_forgerywithout any arguments. Addingprotect_from_forgery with: :exceptionis the same as removingprotect_from_forgeryentirely, although I think the former is more explicit.