-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Set dummy app forgery protection to false #3887
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
|
||
# @private | ||
class ApplicationController < ActionController::Base | ||
protect_from_forgery with: :exception | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think this is necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to not include that line, but I ran into an issue in The error was: I think the problem was that
Which works, but seems really hacky and not ideal. Instead, I thought if I added the line:
It would add It's possible I'm misunderstanding, so please let me know what you think. Thanks so much There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this error happen in the test suite? If yes can you please point a spec that is failing for reference? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the error output:
It looks like the error is happening when loading the orders controller before the tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh ok, it makes sense. I think that we need that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah good point about keeping it in the test environment. Thanks for taking a look |
||
end | ||
|
||
# @private | ||
|
@@ -52,8 +53,8 @@ class Application < ::Rails::Application | |
config.public_file_server.headers = { 'Cache-Control' => 'public, max-age=3600' } | ||
config.whiny_nils = true | ||
config.consider_all_requests_local = true | ||
config.action_controller.allow_forgery_protection = true | ||
config.action_controller.default_protect_from_forgery = true | ||
config.action_controller.allow_forgery_protection = false | ||
config.action_controller.default_protect_from_forgery = false | ||
config.action_controller.perform_caching = false | ||
config.action_dispatch.show_exceptions = false | ||
config.active_support.deprecation = :stderr | ||
|
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 feel like this is not necessary since we already set this value when defining the dummy app.
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.
That's a good point. It is redundant, I just pushed a change removing the line. Thanks
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.
Hey!
I have tried to run the solidus_starter_frontend from this branch and they are failing because of the forgery protection.
I think we need to disable the
allow_forgery_protection
configuration here as well.The reason is that there are two dummy apps that can be generated.
1 - A
Dummy::Application
to test extensions that is created by running this task and use this DummyGenerator. This generator creates a full rails application (Dummy::Application) and overrides the railsenvironments/test.rb
file with the above templates/rails/test.rb configuration. This is the reasonallow_forgery_protection
needs to be disabled incore/lib/generators/spree/dummy/templates/rails/test.rb
.2 - A
DummyApp::Application
to test the solidus framework and it is an application initialized in-memory here when the spec/rails_helper.rb file is loaded. This is the reason we need to disable incore/lib/spree/testing_support/dummy_app.rb
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.
You are right @SamuelMartini, we need to re-add it to make it work with Dummy apps generated in extensions.
@FrancescoAiello01 sorry for the wrong suggestion here! 😬
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.
That makes sense, sorry for the confusion! Going to push a fix now.