-
Notifications
You must be signed in to change notification settings - Fork 685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip CSRF check if a valid JWT is passed in #994
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# frozen_string_literal: true | ||
module ShopifyApp | ||
module CsrfProtection | ||
extend ActiveSupport::Concern | ||
included do | ||
protect_from_forgery with: :exception, unless: :valid_session_token? | ||
end | ||
|
||
private | ||
|
||
def valid_session_token? | ||
request.env['jwt.shopify_domain'] | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
52 changes: 52 additions & 0 deletions
52
test/shopify_app/controller_concerns/csrf_protection_test.rb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
# frozen_string_literal: true | ||
|
||
class CsrfProtectionController < ActionController::Base | ||
include ShopifyApp::LoginProtection | ||
include ShopifyApp::CsrfProtection | ||
|
||
def authenticity_token | ||
render(json: { authenticity_token: form_authenticity_token }) | ||
end | ||
|
||
def csrf_test | ||
head(:ok) | ||
end | ||
end | ||
|
||
class CsrfProtectionTest < ActionDispatch::IntegrationTest | ||
setup do | ||
@authenticity_protection = ActionController::Base.allow_forgery_protection | ||
ActionController::Base.allow_forgery_protection = true | ||
Rails.application.routes.draw do | ||
get '/authenticity_token', to: 'csrf_protection#authenticity_token' | ||
post '/csrf_protection_test', to: 'csrf_protection#csrf_test' | ||
end | ||
end | ||
|
||
teardown do | ||
ActionController::Base.allow_forgery_protection = @authenticity_protection | ||
Rails.application.reload_routes! | ||
end | ||
|
||
test 'it raises an invalid authenticity token error if a valid session token or csrf token is not provided' do | ||
assert_raises ActionController::InvalidAuthenticityToken do | ||
post '/csrf_protection_test' | ||
end | ||
end | ||
|
||
test 'it does not raise if a valid CSRF token was provided' do | ||
get '/authenticity_token' | ||
|
||
csrf_token = JSON.parse(response.body)['authenticity_token'] | ||
|
||
post '/csrf_protection_test', headers: { 'X-CSRF-Token': csrf_token } | ||
|
||
assert_response :ok | ||
end | ||
|
||
test 'it does not raise if a valid session token was provided' do | ||
post '/csrf_protection_test', env: { 'jwt.shopify_domain': "exampleshop.myshopify.com" } | ||
|
||
assert_response :ok | ||
end | ||
end |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're using
ActiveSupport::Concern
, you can justinclude LoginProtection
here. You don't need to do the ancestors check.See https://api.rubyonrails.org/classes/ActiveSupport/Concern.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that's a good idea.
CsrfProtection
should ideally have nothing to do withLoginProtection
. They serve different purposes. If you want, I can haveCsrfProtection
access thejwt_shopify_domain
directly from the request environment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the middleware exposes that interface right now, so it doesn't bother me if you access the
env
directly.My feedback is that under the current approach you have a dependency on
LoginProtection
, and it's better to use the tools thatActiveSupport::Concern
gives you to enforce that dependency than to build your own thing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to do something more duck-typing-esque? Like, put some defensive code around the reference to
jwt_shopify_domain
to raise a meaningful error if it's not present?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, personally. IMO the interface is the keys added to the
env
. We can either access that interface directly or we can pull in a method we know accesses the interface correctly.The
jwt_shopify_domain
method is not the interface and testing that ajwt_shopify_domain
method of unknown origin is present is the wrong approach IMO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is we don't know if the app is using JWT or cookie based auth (Especially due to the fallbacks where it can use either). So we default to the standard Rails CSRF error.