Skip to content
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.

Sign up for GitHub

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

VEN-99 Add brakeman #59

Closed
wants to merge 1 commit into from
Closed

VEN-99 Add brakeman #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,27 @@ jobs:
- vendor/bundle
key: spree-dashboard-bundle-v10-ruby-3-0-{{ checksum "Gemfile.lock" }}

brakeman:
<<: *defaults_3_0
steps:
- checkout
- restore_cache:
keys:
- spree-dashboard-brakeman-{{ .Branch }}
- spree-dashboard-brakeman
- run:
name: Ensure Bundle Install
command: |
bundle check || bundle install
- run:
name: Run Brakeman
command: |
bundle exec brakeman --ignore-config brakeman.ignore --exit-on-warn --exit-on-error
- save_cache:
key: spree-dashboard-brakeman-{{ .Branch }}-{{ checksum "Gemfile.lock" }}
paths:
- ~/vendor/bundle

tests_postgres: &tests_postgres
<<: *run_tests
environment: &postgres_environment
Expand Down Expand Up @@ -188,6 +209,9 @@ workflows:
jobs:
- bundle
- bundle_ruby_3_0
- brakeman:
requires:
- bundle_ruby_3_0
- tests_postgres:
requires:
- bundle
Expand Down
1 change: 1 addition & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ end

group :test, :development do
gem 'awesome_print'
gem 'brakeman', '~> 5.0'
gem 'gem-release'
gem 'redis'
gem 'rubocop', '~> 1.22.3', require: false # bumped
Expand Down
5 changes: 3 additions & 2 deletions app/controllers/spree/admin/payment_methods_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ class PaymentMethodsController < ResourceController
respond_to :html

def create
@payment_method = params[:payment_method].delete(:type).constantize.new(payment_method_params)
provider_name = @providers.map(&:to_s).find { |provider| provider == params[:payment_method][:type] }
@payment_method = provider_name.constantize.new(payment_method_params)
@object = @payment_method
set_current_store
invoke_callbacks(:create, :before)
Expand Down Expand Up @@ -80,7 +81,7 @@ def validate_payment_method_provider
end

def payment_method_params
params.require(:payment_method).permit!
params.require(:payment_method).permit(:type, :name, :description, :display_on, :auto_capture, :active, store_ids: [])
damianlegawiec marked this conversation as resolved.
Show resolved Hide resolved
end

def preferences_params
Expand Down
6 changes: 5 additions & 1 deletion app/controllers/spree/admin/payments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def create
end

def fire
return unless event = params[:e] and @payment.payment_source
return unless (event = find_payment_event) && @payment.payment_source

# Because we have a transition method also called void, we do this to avoid conflicts.
event = 'void_transaction' if event == 'void'
Expand Down Expand Up @@ -107,6 +107,10 @@ def load_payment
def model_class
Spree::Payment
end

def find_payment_event
%w[process authorize purchase capture void cancel].find { |e| e == params[:e] }
end
end
end
end
2 changes: 1 addition & 1 deletion app/controllers/spree/admin/promotion_rules_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,6 @@ def validate_promotion_rule_type
end

def promotion_rule_params
params[:promotion_rule].permit!
params[:promotion_rule].permit(:type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aleksandarpetrushev what about user_id or product_group_id attributes for this model? or preferences?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@damianlegawiec we have a similar situation such as in payment_methods_controller. promotion_rules_controller is used only for #create and #destroy. When creating a promotion rule, we know the type only. The additional attributes are sent in PromotionsController#update as nested "promotion_rules_attributes", after the promotion rules have been created. Tested for all promotion rules, only the type is sent.

end
end
4 changes: 2 additions & 2 deletions app/views/spree/admin/return_index/return_authorizations.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@
</li>
<li class="nav-item">
<%= link_to Spree.t(:authorized),
params.merge({q: {state_eq: :authorized}}).permit!,
params.permit(q: {}).merge({q: {state_eq: :authorized}}),
class: "nav-link #{'active' if params[:q][:state_eq] == 'authorized'}" %>
</li>
<li class="nav-item">
<%= link_to Spree.t(:canceled),
params.merge({q: {state_eq: :canceled}}).permit!,
params.permit(q: {}).merge({q: {state_eq: :canceled}}),
class: "nav-link #{'active' if params[:q][:state_eq] == 'canceled'}" %>
</li>
<% end %>
Expand Down
104 changes: 104 additions & 0 deletions brakeman.ignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
{
"ignored_warnings": [
{
"warning_type": "Mass Assignment",
"warning_code": 70,
"fingerprint": "2096b83195b858fd16fa7b7139bc84400d108b5f85a8c64ae91da708b5fe21f9",
"check_name": "MassAssignment",
"message": "Specify exact keys allowed for mass assignment instead of using `permit!` which allows any keys",
"file": "app/controllers/spree/admin/resource_controller.rb",
"line": 268,
"link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/",
"code": "params.require(resource.object_name).permit!",
"render_path": null,
"location": {
"type": "method",
"class": "Spree::Admin::ResourceController",
"method": "permitted_resource_params"
},
"user_input": null,
"confidence": "Medium",
"note": ""
},
{
"warning_type": "Mass Assignment",
"warning_code": 70,
"fingerprint": "934f23be465f0d2c5f7278aac4f085771ba22d5e8b56645813a2364313df9561",
"check_name": "MassAssignment",
"message": "Specify exact keys allowed for mass assignment instead of using `permit!` which allows any keys",
"file": "app/helpers/spree/admin/navigation_helper.rb",
"line": 116,
"link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/",
"code": "params.permit!",
"render_path": null,
"location": {
"type": "method",
"class": "Spree::Admin::NavigationHelper",
"method": "per_page_dropdown_params"
},
"user_input": null,
"confidence": "Medium",
"note": ""
},
{
"warning_type": "Mass Assignment",
"warning_code": 70,
"fingerprint": "d45f35a46d0896dcd87099cbae9ce1c77b79ac3c935c0a6317e1a2462186e486",
"check_name": "MassAssignment",
"message": "Specify exact keys allowed for mass assignment instead of using `permit!` which allows any keys",
"file": "app/controllers/spree/admin/prices_controller.rb",
"line": 7,
"link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/",
"code": "params.require(:vp).permit!",
"render_path": null,
"location": {
"type": "method",
"class": "Spree::Admin::PricesController",
"method": "create"
},
"user_input": null,
"confidence": "Medium",
"note": ""
},
{
"warning_type": "Remote Code Execution",
"warning_code": 24,
"fingerprint": "f46c38504b6d7300a6152298cbcd4f65afde1f0b5fd1c642d79908d56ff1aa8c",
"check_name": "UnsafeReflection",
"message": "Unsafe reflection method `constantize` called with parameter value",
"file": "app/controllers/spree/admin/promotion_actions_controller.rb",
"line": 7,
"link": "https://brakemanscanner.org/docs/warning_types/remote_code_execution/",
"code": "params[:action_type].constantize",
"render_path": null,
"location": {
"type": "method",
"class": "Spree::Admin::PromotionActionsController",
"method": "create"
},
"user_input": "params[:action_type]",
"confidence": "High",
"note": ""
},
{
"warning_type": "Mass Assignment",
"warning_code": 70,
"fingerprint": "f5df7c2ded2d794ea955c7ac98e0b5868e4787e56bec481b5e794742f7800308",
"check_name": "MassAssignment",
"message": "Specify exact keys allowed for mass assignment instead of using `permit!` which allows any keys",
"file": "app/controllers/spree/admin/payment_methods_controller.rb",
"line": 91,
"link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/",
"code": "params.require(ActiveModel::Naming.param_key(@payment_method)).permit!",
"render_path": null,
"location": {
"type": "method",
"class": "Spree::Admin::PaymentMethodsController",
"method": "preferences_params"
},
"user_input": null,
"confidence": "Medium",
"note": ""
}
]
}