Skip to content

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.


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.

3 participants