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

fix: check permissions for order workflow methods #4863

Merged
merged 1 commit into from
Dec 18, 2018

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Dec 12, 2018

Impact: minor
Type: bugfix

Issue

Permissions were not checked by three Meteor methods, such that anybody with enough information could set an order or order item status to anything.

Solution

  • Check for "orders" permission for the order shop in "workflow/pushOrderWorkflow" and "workflow/pushItemWorkflow" Meteor methods
  • Removed the "workflow/pullOrderWorkflow" Meteor method because it isn't used

Breaking changes

A custom plugin that is calling the "workflow/pullOrderWorkflow" Meteor method will break

Testing

Verify that you get access denied when calling "workflow/pushOrderWorkflow" method in an incognito tab, or in a tab where you are logged in as a customer. Example to run in the browser console, replacing with IDs that exist in your DB:

Meteor.call("workflow/pushOrderWorkflow", "coreOrderWorkflow", "processing", { _id: "aHEm8LriEFSnmndbx", shopId: "J8Bhq3uTtdgwZx3rz" }, console.log.bind(console));

Verify that you get access denied when calling "workflow/pushItemWorkflow" method in an incognito tab, or in a tab where you are logged in as a customer. Example to run in the browser console, replacing with IDs that exist in your DB:

Meteor.call("workflow/pushItemWorkflow", "coreOrderItemWorkflow/picked", { _id: "aHEm8LriEFSnmndbx", shopId: "J8Bhq3uTtdgwZx3rz" }, ["27gYhd4rMCX37jGzH"], console.log.bind(console));

@aldeed aldeed self-assigned this Dec 12, 2018
@aldeed aldeed requested a review from mikemurray December 12, 2018 13:51
@aldeed aldeed added this to the 🏔 Shavano milestone Dec 12, 2018
@aldeed aldeed changed the base branch from release-2.0.0-rc.8 to develop December 14, 2018 20:08
@aldeed aldeed requested review from impactmass and removed request for mikemurray December 14, 2018 20:09
@impactmass
Copy link
Contributor

I had this on my TODO for today, but I could not get to it. I'll review tomorrow

Copy link
Contributor

@impactmass impactmass left a comment

Choose a reason for hiding this comment

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

LGTM while testing. Access denied errors thrown as appropriate.

One question: I noticed this workflow/pushCartWorkflow in the workflow.js file. Do we need to have a hasPermission check there too?

@aldeed
Copy link
Contributor Author

aldeed commented Dec 18, 2018

@impactmass Good question about cart workflow methods. The security audit did identify pushCartWorkflow and revertCartWorkflow as insecure, but they are used only by the classic UI checkout, which we are removing soon. So the fix will be to remove those methods when the corresponding UI is removed.

@impactmass
Copy link
Contributor

@aldeed 👍

@impactmass impactmass merged commit 9664a1d into develop Dec 18, 2018
@impactmass impactmass deleted the fix-aldeed-security-1 branch December 18, 2018 16:26
@spencern spencern mentioned this pull request Jan 18, 2019
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