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.

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

wants to merge 1 commit into from

Conversation

aleksandarpetrushev
Copy link

No description provided.

@aleksandarpetrushev
Copy link
Author

@damianlegawiec Does not run at all

@aleksandarpetrushev
Copy link
Author

aleksandarpetrushev commented Nov 29, 2021

For https://codeclimate.com/github/spree/spree_backend/app/controllers/spree/admin/promotion_actions_controller.rb?from_sha=31a4e70c&to_sha=fc8404a7 we need to eager load all promotion actions (in development environment) for being able to search Spree::PromotionAction.descendants, and not .constantize the :action_type parameter directly. In other environments they will be loaded automatically.

@@ -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?

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.

@damianlegawiec
Copy link
Member

@aleksandarpetrushev I see there are still 7 issues listed by CC - what's the plan for them?

@aleksandarpetrushev
Copy link
Author

aleksandarpetrushev commented Nov 30, 2021

@aleksandarpetrushev I see there are still 7 issues listed by CC - what's the plan for them?

@damianlegawiec
for the critical one: #59 (comment)

  • 2 of them are related to sending variable parameters (such as payment provider settings)
  • one is in the resource_controller, so I assumed (maybe wrongly) that we should not touch that one
  • first(".icon-#{type}").click is a possible SQL injection, although we are always passing in args like :edit, I can whitelist those
  • I need to look into the last two

@aleksandarpetrushev
Copy link
Author

@damianlegawiec So to sum it up we have 5 issues left:

  1. and 2. are because of many different preferences we can send
  2. whitelisting all params in resource_controller
  3. not really possible sql injection (the method name is first) it is actually a test helper
  4. the most critical one - we need to eager load some models in spree/spree

So how should we proceed?

@aleksandarpetrushev
Copy link
Author

Continues at #106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants