-
Notifications
You must be signed in to change notification settings - Fork 3
feat: encode store name + check for fetch
#73
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
Conversation
✅ Deploy Preview for blobs-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:customer_id was meant to be a placeholder for the customer ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.