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

Multi-shop permission fixes #4872

Merged
merged 13 commits into from
Dec 19, 2018
Merged

Multi-shop permission fixes #4872

merged 13 commits into from
Dec 19, 2018

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Dec 12, 2018

Impact: minor
Type: bugfix

Issues

  • "products/updateVariant" Meteor method had questionable security and was unused
  • "products/updateVariantsPosition" Meteor method did not check permissions for correct shop. Someone with permission to change variant order for any shop could also do it for all other shops.
  • "products/getpublishedProductHash" Meteor method was not secured and was unused
  • "shop/changeLayouts" Meteor method was not secured and was unused
  • "shop/fetchCurrencyRate" Meteor method was not secured. Anybody, even if not logged in, could trigger a query of the currency rate exchange API and subsequent update of currencies property of the primary shop.
  • "shop/flushCurrencyRate" Meteor method was not secured. Anybody, even if not logged in, could trigger the clearing of all rate properties in the currencies property of the primary shop.
  • "shop/hideHeaderTag" Meteor method did not properly check shopId and was unused.
  • "shop/updateHeaderTags" Meteor method did not properly check shopId. If you knew a tag ID and had proper permissions for your own shop, you could change the name of that tag even if it belonged to a different shop.
  • "shop/removeHeaderTag" Meteor method did not check permissions for correct shop and was unused.
  • "shop/togglePackage" Meteor method did not check permissions for correct shop. If you had permission for your shop, you could toggle plugins enabled status for another shop.
  • "registry/update" Meteor method did not check permissions for correct shop. If you had permission for your shop, you could edit plugin settings for another shop.

Solutions

  • "products/updateVariant" Meteor method is removed
  • "products/updateVariantsPosition" Meteor method checks permissions for the correct shop.
  • "products/getpublishedProductHash" Meteor method is removed
  • "shop/changeLayouts" Meteor method is removed
  • "shop/fetchCurrencyRate" Meteor method is removed. The code is now in a private server method because it is only called from background job workers.
  • "shop/flushCurrencyRate" Meteor method is removed. The code is now in a private server method because it is only called from background job workers.
  • "shop/hideHeaderTag" Meteor method is removed.
  • "shop/updateHeaderTags" Meteor method now checks permissions for the correct shop.
  • "shop/removeHeaderTag" Meteor method is removed.
  • "shop/togglePackage" Meteor method now checks permissions for the correct shop.
  • "registry/update" Meteor method now checks permissions for the correct shop.

Breaking changes

Custom code relying on the removed Meteor methods will break.

Testing

  1. Verify that the removed Meteor methods can no longer be called from a connected browser.
  2. Verify that the listed permission issues with the not-removed Meteor methods are fixed. You will need to create multiple shops to test some fixes, for example by enabling and using the Marketplace plugin.

To call Meteor methods from a browser, connect to localhost:3000, open the browser console, and enter Meteor.call("methodName", /* all necessary args */, console.log.bind(console)). By passing console.log as the callback function, it will be sure to log the results and/or error when it's done.

@aldeed aldeed self-assigned this Dec 12, 2018
@aldeed aldeed requested a review from kieckhafer December 14, 2018 19:58
@aldeed aldeed added this to the 🏔 Shavano milestone Dec 14, 2018
@aldeed aldeed changed the base branch from release-2.0.0-rc.8 to develop December 14, 2018 20:10
if (!Reaction.hasPermission("createProduct")) {
throw new ReactionError("access-denied", "Access Denied");
}

// Check first if Product exists and then if user has the right to alter it
Copy link
Member

Choose a reason for hiding this comment

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

Why do we check for the Product first, and then permissions? Should a user without permissions be allowed to know if a product exists or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. It's definitely best practice to avoid giving clues to existence for something like querying if a user with a certain email exists. In this case, though, the product ID is public information so we're not really revealing anything. The reason we look up the product first, is because we need product.shopId in order to do the permission check for the correct shop. We can't even do a permission check if we don't have a product. We could still choose to throw a less informative error, but as I said in this case we're not really revealing anything.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Makes sense, thanks for the clarification.

@kieckhafer
Copy link
Member

@aldeed This is pretty much good to go, just had one question, see above.

@kieckhafer kieckhafer merged commit c4a2306 into develop Dec 19, 2018
@kieckhafer kieckhafer deleted the fix-aldeed-security-3 branch December 19, 2018 05:47
@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