Sanitize model strings

[kingcody]

I've been looking into preforming string sanitization on user input for both the User and Thing models. My goal is to reduce native vulnerabilities to XSS by removing/escaping HTML entities. Besides it being just a good idea in general, I'd like to secure the stack against this further, to better allow for the use of other technologies such as Local Storage; which is completely vulnerable to XSS.

I'm wondering if defining a schema plugin to sanitize string type values would be wise? Or should they be handled on a per key basis. The plugin solution would be easy to use since you could add it to an entire schema. However the second option of defining a pre-save hook to sanitize specific keys could prove to be more performant. Since it would not need to loop (recursively) to find the string values. The plugin option could be useful for maintaining consistency in a project, especially when there are multiple developers. Again however, the pre-save hook could provide more individual control, allowing for more use cases.

If anyone here has any experience with mongoose and model sanitization, or just has an idea related to the subject, I'd love to hear it.

[wolfie82]

Great idea.

Perhaps the focus shouldn't be on the model and more so on an express middleware that will sanitize input? I believe there are a few out there, however I would need to review to get up to speed again. I think this places a developer in a better position of not cluttering the model. Also, I'm not sure of the performance penalty if this route is taken.

[kingcody]

@patbaker82 thanks for the reply! My only issue with the express middleware option is that the models are update from various sources; such as socket.io or perhaps a file reading method.

I can see the value in not wanting to clutter the model. However, my thought was that it would help to have it in a more centralized place in the code.

What are your thoughts on this?

[wolfie82]

@kingcody I guess there could be arguments on both sides of the fence. IE if I request something from an API and use that data to provide information to a user; no model. I wonder if we could apply some similar middleware logic to sockets?

My suggestion only comes out of familiarity with how other frameworks implement this. I'm certainly not opposed to putting something like this in the model. However, as applications get more complicated there will be a human element when adding these XSS security features. It would be nice if an implementation could be crafted that is more under the hood.

[kingcody]

Those are really good points you make @patbaker82. I agree that it would be nice to sanitize input from other sources as well, such as external API's like you suggested. I also couldn't agree more with your point about the human element. My hope was to provide a 'feature' that would work out-of-the-box as well as allow developers to easily extend it and include it in there additional work. If we could manage to do that, while also providing some level of security for users that simple generate and go, that would be awesome.

Thanks so much for entertaining my suggestions @patbaker82. If you have any ideas on an 'under-the-hood' implementation, please do holla :)

[kingcody]

I had this thought just now... Would it be advantageous to include validator.js as a dependency to the generated app? It would provide the lower level validation/sanitation tools that could be used in creating the middleware(?). Also it could benefit the existing Model validations by providing a cleaner and potentially more robust solution to the current checks in place. I only say 'more robust' since validator.js is a very popular module used by over 300 packages on npm. Therefore we would, by proxy, have many more eyes pouring over the validations used in the Fullstack.

Side note: Validator.js has no dependencies of its own and is simply one js file. My point being that if it was included as a dep, it would not bring that much extra bloat to the stack.

[JaKXz]

@kingcody I agree with you, 8.534 kb is not much overhead at all, and I think validator.js would be nice to include.

[wolfie82]

@kingcody @JaKXz FYI - validator removed support for XSS

[wolfie82]

@kingcody Perhaps it would be enough to add validator.js in a middleware. You could use the sanitize implementation in validator to escape outgoing content from the API. Additionally we could employ a white list for input data.

Rails actually has a decent write up on their implementation that I think we could easily mirror.

Rails XSS strategy

[wolfie82]

I may also suggest you checkout Helmet which includes a set of implementations to help improve security.

[thomporter]

+1 to validator.js and Helmet.

I think adding Validator.js is a great idea, being such a well supported module for validation. I think a new user to the stack would appreciate it (I know I would. =)

Additionally, I think using Helmet for XSS is the way to go. Again, I would point to it's wide use and support.

I'm on the fence about these being optional. I cringe to think of making the validator optional, and worse, letting users choose between more than one. I almost think if auth is involved, it should just use Validator.js - no option. For XSS though, I think it's quite possible it might not be needed. If you're not accepting user input, then it would really just be in the way, so I think Helmet should be optional. Down the line we might come up with other options for XSS as well, I don't think the's would be as harsh on the generator code to support.

[kingcody]

@patbaker82 I actually meant to use validator.escape()

[kingcody]

Also, I'm not sure how others feel; but I think that helmet.js might be a bit much for what it offers. Don't get me wrong, it has nice features. However most of them are only partially supported by browsers or would require fine grain control such as whitelists. My opinion is to either borrow a few features that helmet.js offers and include them by default, or add helmet.js as a dependency only if the user agrees to a prompt for additional security middleware/modules. Just my opinion though.

[wolfie82]

@kingcody I think you're right in the assessment of adding sensible defaults and perhaps not the whole middleware. validator.escape() looks good. I think we would need to implement a while list for tags. The Rails guys went through many iterations on security. Their guide is a decent read, aforementioned, perhaps we should mirror some of their implementations.