Open WardCunningham opened 7 years ago
Thanks for the heads up, Ward. Did you want to issue a pull, or should I take a look? The latest incarnation of client,js1 already sanitizes the key from this, but I don't believe that helps protect from an attack directly against the API, so something needs to be done, nonetheless.
My inclination would be to (a) remove text as a fallback as it is a legacy that shouldn't arise (0.3.0 also revs old data to use key), and (b) strip toxic characters. I don't feel there's a reason to escape the key given the constraints already in place on the client side.
You can ignore my last comment about key--client.js will parse it and expects [\w\-], but it will save whatever is in the textarea without question (render is parsed, but not save).
This will become a priority once federated wiki is immensely popular. Before then we won't attract much abuse. Still, I make arguments regarding safe sharing so I have to take safety seriously. I'm happy to tend the server side. I'll get a pull request together.
I learned today of what is called "csv function vulnerability".
https://www.contextis.com//resources/blog/comma-separated-vulnerabilities/
Since we pass json freely throughout the federation we have some obligation to insure that our plugins don't misinterpret json in a way that could be dangerous. This line should be more careful:
https://github.com/pragmar/wiki-plugin-fivestar/blob/master/server/server.js#L69
A malicious item.stars field would not get past the parseInt() but the item.text is dangerously transcribed to the csv header without further inspection. This ruby code exhibits sufficient caution: