feat: encode store name + check for fetch
#73
Conversation
✅ Deploy Preview for blobs-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
github-actions
bot
added
the
type: feature
| @@ -33,7 +33,7 @@ export class Store { | |||
|
|
|||
| constructor(options: StoreOptions) { | |||
| this.client = options.client | |||
There was a problem hiding this comment.
Does it make sense to scope options.name as well? I'm wondering what happens when name is set dynamically based on user input, and some attacker gets this to be deploy:foo. Then they can access a different store. If we also prefixed options.name, that wouldn't be possible
There was a problem hiding this comment.
What would be the attack vector there? If you set the store name to deploy:<something>, you end up scoping your store to one of your deploys, which is basically the same as supplying a deploy ID in the constructor. It's not like you'll gain access to someone else's deploy?
I would be more in favour of adding a validation rule that prevents you from getting a store that starts with deploy:, because that's a reserved scope. Not that anything happens if you use it, but because it isn't the right flow.
There was a problem hiding this comment.
What would be the attack vector there?
Imagine a site that stores some deploy-specific assets in deploy:id (don't know why, it's contrived), and then customer-specific assets in a store called :customer_id. Now a customer signs up with the slug deploy:foo, and 💥 their store clashes with the deploy-specific assets.
I would be more in favour of adding a validation rule that prevents you from getting a store that starts with `deploy:``
Yes, that sounds like a good fix!
There was a problem hiding this comment.
Imagine a site that stores some deploy-specific assets in
deploy:id(don't know why, it's contrived), and then customer-specific assets in a store called:customer_id. Now a customer signs up with the slugdeploy:foo, and 💥 their store clashes with the deploy-specific assets.
With this PR, a store can't be called :customer_id, because we're URL-encoding it to %3Acustomer_id.
There was a problem hiding this comment.
:customer_id was meant to be a placeholder for the customer ID
There was a problem hiding this comment.
I don't think I've understood your example, but either way checking for the deploy: prefix is something we should do.
Done in 231386e.
There was a problem hiding this comment.
I've also URL-encoded the deploy ID for good measure. It shouldn't ever be needed, because deploy IDs are alphanumerical, but it just prevents someone doing weird things from triggering a request to a malformed URL.
URL-encodes the store name and throws an error message when a
fetchimplementation hasn't been found.Closes https://github.com/netlify/pillar-runtime/issues/757.
Closes https://github.com/netlify/pillar-runtime/issues/756.