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

Sensitive data exposed via Graphql #3768

Closed
timuric opened this issue Feb 25, 2019 · 11 comments

Comments

@timuric
Copy link

commented Feb 25, 2019

By default Saleor exposes sensitive business information like:

ProductVariant {
quantity: Int!
quantityAllocated: Int!
costPrice: Money
stockQuantity: Int!
margin: Int
quantityOrdered: Int
revenue(...): TaxedMoney
}

I hope nobody is running Saleor out there with theses fields exposed to public.

@dominik-zeglen dominik-zeglen added bug core graphql and removed core labels Feb 25, 2019

@maarcingebala

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Thanks for reporting. Some of these fields are used by the management dashboard and should be available only to authenticated users with proper permissions. I see that some of them are missing those permissions checks. They will remain in the public schema, but shouldn't be accessible by unauthorized users.

@timuric

This comment has been minimized.

Copy link
Author

commented Feb 25, 2019

@maarcingebala I think it would be important to inform current shop owners about this.
Also looks like it is a design flaw since there is no field "inStock" for variants.
At the moment only way the storefront can tell if something is in stock is by reading the public stock property.

This is quite a serious issue for anyone running Saleor in production.

@maarcingebala

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Fix has been merged to master and released in Saleor 2.3.1 version.

@timuric

This comment has been minimized.

Copy link
Author

commented Feb 26, 2019

Looks like #3773 doesn't cover the stock quantity

@maarcingebala

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

I don't consider stock quantity to be as sensitive as financial data, I think it depends more on the use-case. Also, it seems that this field is currently used by Storefront 2.0. I'd like to discuss if we should restrict this field to privileged users or expose a boolean flag inStock instead. Another idea is to limit the maximal value that we show to unauthorized users to some arbitrary value.

@timuric

This comment has been minimized.

Copy link
Author

commented Feb 26, 2019

I don't consider stock quantity to be as sensitive as financial data, I think it depends more on the use-case.

@maarcingebala If you multiply stock by price you can easily track the revenue. It is almost the same as exposing revenue. The only noise in data would be stock transfers, which is also not hard to account for.

Another idea is to limit the maximal value that we show to unauthorized users to some arbitrary value.

That would be nice, since it will allow to create "low in stock" notifications. It would be great to be able to adjust the threshold.

@jxltom

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

I think revenue = (quantity - stock) * price? Maybe Saleor should limit the access to total quantity of a product variant?

@maarcingebala

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

I agree that quantity and quantityAllocated should be private, but it's not exactly the same as exposing revenue. Allocated quantity tells how many items are currently allocated in open orders, but doesn't tell you anything about past orders. You would have to regularly fetch quantities to track that but still - the impact is not that heavy as directly exposing revenue.

@timuric

This comment has been minimized.

Copy link
Author

commented Feb 27, 2019

@maarcingebala would be good to notify existing store owners about the issue so they can temporarily patch it. Some people might get fired for a data leak like that :)

@maarcingebala

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I'll mention that on Gitter and on Spectrum. This will be also mentioned in the incoming release notes on our blog.

@maarcingebala

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Oh, and we're waiting to get the CVE vulnerability ID for that bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.