-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
[WIP] Replace validator's xss() with caja's sanitize() #1633
Conversation
Will this work for example with themes that allow code snippets in the content? Tutorial, here's how you put some javascript into your html page:
Would all of that be escaped into html entites and left in, or would all of that be stripped out entirely? |
It depends; if it's sanitized, then it would be stripped out entirely. But the post content is not sanitized at all currently. Handlebars may still escape it afterwards. |
Everything done, feel free to merge. A few notes:
|
I haven't look at this PR yet. Not sure whether it is a good idea to put this in 0.4 or not, will look and make a decision in the next day or so. |
Up to you; The user feedback is not ideal currently, but the sanitizer should work fine. |
Thought I'd come and update my thoughts on this. To do it well I think we need to be able to give proper user feedback, so unless there have been developments in caja, unfortunately I think it may be a dead end? |
I don't think it's related to caja, the sanitize function simply returns the safe html. The question is how to tell the user "we've just deleted your entire title because you put My suggestion would be:
|
@jgillich with the exception of the second part about the editor / preview this sounds like a sensible approach. The problem with the editor is that we do want to support
We possibly need a combination of some or all of the above? |
I think that we need a combination of escaping (as described by @jgillich) and disabling JavaScript to make the Ghost admin safe for multi-user support.
|
@jgillich Is there a special reason why you published your own wrapper for Google-Caja? I'm just curious if there is something wrong with the other wrappers. I had two thoughts that I would like to ask here:
|
The other two wrappers I've checked both included a hacky workaround: https://github.com/theSmaw/Caja-HTML-Sanitizer/blob/master/sanitizer.js#L1083
I think the markdown converter should run first, which would mean code snippets are already escaped when the content is sanitized. |
+1 for enabling javascript only on frontend, and using caja sanitizer for removing scripts on admin |
That depends on your trust in your fellow Ghost users. Since we have no editing history we're unable to tell if someone else has edited a post and disable scripts accordingly. An attack scenario would be that another user adds a script to your blog post which is executed with admin rights when the administrator opens the post. This could be used to install apps or escalate privileges.
You are absolutely right, that will work. |
This conversation has come to a halt again. I think that this issue is something we should tackle with milestone 0.6? We can leave this PR open and add functionality when we have a final concept of how to handle XSS? |
XSS is a big deal, and is going to be a primary focus of 0.6, so I agree, lets leave this PR and tackle this problem head on when we have more time for it in 0.6. |
I'm going to close this now as we are working on adding escaping and also a CSP in order to tackle XSS problems in the admin. |
This is not yet ready (tests fail for some reason), just putting it here for feedback.
As discussed in #1378, this replaces node-validator's xss filter with the one from caja.
When you look at the changes, you will notice that this does not escape or replace the elements, instead it removes them entirely. This means that if you enter something like
<script></script>
in the title, you get told that an empty value isn't allowed. Probably not the best way to handle this.I did some digging and as far as I can see, escaping would require us to modify caja heavily. What we could easily do is white/blacklisting of elements & attributes (sanitize() uses a internal list of elements that it considers unsafe, probably fine for our use case), but escaping instead of removing them is not really what it's been made for.