Reaction: Security: Shop ids sent from client are published

[Akarshit]

Expected behavior

Client should not be able to control what gets published

Actual behavior

The publisher publishes the shop whose id id sent from the client.

Cause

Here, the shopIds are being sent from client to the publisher to publish.

There is a Shops publication that should publish the shops that user has "admin" permission for.
For this we use something like
```Meteor.publish("MerchantShop", () => {
// This part is not run again as server side publish function are not reactive.
const query = { _id: { $in: Reaction.getShopsOfUser(this.userId) } };
return Shop.find(query);
})

When a new shop is added the code inside the publisher is not run again and hence the new shop is not published to the user.
Now one solution to this would be to observe the whole Shops collection and then check when a new shop is added and then refresh this publication.
Something like
```Shops.find().observe({
  added() {
    // refresh MerchantShops publication
  }
})

But the problem with this is the way we add shops, we first add the shop to the Shops collection and then later add permission to the user.
So what will happen is when we run the publish code again Reaction.getShopsOfUser(this.userId) will not contain the new shop(as user still hasn't receive permission for it) and therefore the new shop will not be published.
A simple solution is to change the publisher to
Meteor.publish("MerchantShop", (shopsOfUser) => { // This part is not run again as server side publish function are not reactive. const query = { _id: { $in: shopsOfUser} }; return Shop.find(query); })
and pass Reaction.getShopsOfUser(this.userId) as shopsOfUser from the client, but this introduces the security bug that I mentioned.
A quick solution for this would be to again check inside the publisher if Reaction.getShopsOfUser(this.userId) === shopsOfUser. This should fix all the security issues.
But do you think that is OK to do.
Also all this would run only when a shop is added for a user(basically only when the user click the Become a seller button on profile) or when the user changes.

Versions

release-1.8.0

[Akarshit]

I am working with @jshimko to find a good solution for this problem.

[Akarshit]

@jshimko Could you take a look at this when you are around.

[zenweasel]

@jshimko Please help Akarshit out with this or alternately just take it over and fix it.

[spencern]

@Akarshit I see that @aaronjudd has assigned this to me, what's the status of this right now? Has any work been done on it?

[zenweasel]

@spencern I know that no work has been done. He was looking to get technical direction on how to solve this problem and still make the publication reactive.

[spencern]

Reading through the issue, I'm not sure what the problem is, is the concern that we're publishing the _ids of shops or is there something more?

[zenweasel]

Any client could provide an array of shopIds and get data about arbitrary shops for which they don't have permissions. At least that's what it looked like to me.

[spencern]

👍we should probably patch that gap.

A quick solution for this would be to again check inside the publisher if Reaction.getShopsOfUser(this.userId) === shopsOfUser. This should fix all the security issues.
But do you think that is OK to do.
Also all this would run only when a shop is added for a user(basically only when the user click the Become a seller button on profile) or when the user changes.

@Akarshit
I think that checking permissions on the publication is the appropriate way to do this. If a shop is active, then it may need to be published to the client for any user visiting that shop, though that may be partially determined by the marketplace settings.

This is not a big concern, however, because there's not really any sensitive data stored on the shop document.

[Akarshit]

Great! I will make a PR with the patch I proposed.