Roll20 uses cookies to improve your experience on our site. Cookies enable you to enjoy certain features, social sharing functionality, and tailor message and display ads to your interests on our site and others. They also help us understand how our site is being used. By continuing to use our site, you consent to our use of cookies. Update your cookie preferences .
×
Create a free account

sendChat needlessly mangles CSS

1462716051
Lucian
Pro
API Scripter
It appears that sendChat removes CSS properties that it doesn't recognise from HTML that is sent via sendChat. It does this even when using /direct. For example, I tried using display:flex, and ended up with 'display:' in the resulting chat output. This is in a campaign-specific script, and I know all the players have modern browsers. I get why you want to restrict the available HTML tags to avoid security problems, but I'm struggling to see why you need to mess with the CSS. Granted, arbitrary CSS could mess up the display somewhat, but then again, an API script could wipe your entire campaign, so that's hardly a particularly serious risk! 
1462791717
Lucian
Pro
API Scripter
For what it's worth, this also applies to handouts and other things with notes fields. The Redactor editor, sketchy though it sometimes may be, does actually cope ok with some decent HTML content, and it's possible to make some nice looking layouts in the handouts either by pasting in from outside.... except that the stylesheet uses a lot of !important, which I have to override using my own !important, and your sanitize_html function (from what I can make out behind the minification) strips all/most of these out of my style attributes. It's kinda frustrating that I can make something nice-looking in the editor window and then it gets gutted when I click save :-( Is there any way you can tone down the CSS mangling on this? Obviously it will let people mess up the display of various things, but if they're pasting pre-written HTML/CSS in they probably know that already, right? 
1462827568

Edited 1462827600
Phil B.
Forum Champion
Sheet Author
Hey Lucian, sorry for the inconvenience but this isn't something we can safely get rid of. You give the average internet user far too much credit, thinking that "they know better than to copy and run random code they find". Allowing a script to override the sites CSS and HTML can lead to all kinds of security issues, like making the page look like a login page and say that the player's session expired to steal passwords or any infinite number other bad ideas. I know you and 99% of the script writers just want to make cool stuff, but that last 1% ruins it for everyone and makes us require this kind of extra security.  As for your issue with the text editor, we know the current version is bad. I get a bug report at least once a week about a different piece that's broken. It's on our radar, and I'm going to push for it to be my next big project to update. (it's not as easy as just updating the code, it's tied to pretty much every part of the site and last time we tried everything broke)
1462831736
Lucian
Pro
API Scripter
Hey Phil, Thanks for the reply. I understand where you are coming from, and I applaud you for taking security seriously, but I'm not convinced that you've got the balance quite right. I'm not for one moment suggesting that you allow arbitrary HTML, or the injection of generic stylesheets into the site. I'm merely suggesting that you allow people to put style attributes onto the limited HTML that you support within tightly controlled contexts without messing with them unduly. The handout dialog has overflow:hidden on it; the worst a malicious agent could conceivably achieve would be to reskin the handout dialog to look like a login box of some description. This would be really very challenging to achieve and would at worst save the supplied information to the user's campaign data somewhere. It's a pretty poor attack for a lot of effort and almost no gain. I think part of the problem is that there isn't a one-size-fits-all answer to this across the site. Campaign handouts and the campaign chat window seem like they are much lower risk than, for example, forum posts - they're closely monitored by a very limited set of users who have a close relationship to the creation of the content in them and are going to be much more likely to notice something out of place.  I'd reiterate my original assertion - I strongly suspect that for most Roll20 users, having their whole campaign trashed by an API script is likely to be worse than any of the other credible attack vectors - and I'd follow it up with a challenge: can you describe, even in rough outline, a way in which allowing arbitrary style attributes (not elements) on controlled HTML inside a handout could result in some harm to your users that is worse than what is already possible? Since you can't send data anywhere (no scripts, no forms, no external links without warning), I'm struggling to see how making things look different (and only within the outline of the handout dialog) would really make much difference to the risk level. Hell, I'm sure you can already do *much* worse using a character sheet, which supports form controls! If you're really still adamant that this is too risky, then I'd encourage you to give some serious thought to the idea of some sort of simple code-signing scheme in the future. In the end, trying to prevent people from Doing Bad Things On The Internet is a losing battle; it's a lot more effective to help build trust networks rather than trying to police the untrusted. It wouldn't be hard to accept a digital signature along with a script (perhaps in exchange for permission to do more), and that way you'd remove most of the risk of scripts from unknown sources.
1462902999
Lucian
Pro
API Scripter
Sorry to flog a dead horse, but I'm wondering if there's any room for even a little flexibility here. Currently the redactor editor CSS won't even let me change the font-size or font-family of most of the elements thanks to its rather aggressive use of !important. Is this *really* necessary? Even some small concessions here would make a big difference to how much can be achieved with handouts.
So I think that there are two different issues you're discussing here. One is the overall CSS/Javascript sanitization that we are using across the site; it's the same whether you're using a handout, /direct in sendChat, sending a chat message in the client, etc. That sanitization is backed by Google's Caja sanitization library. It's big, it's complicated, and we didn't write it. It's based in large part on a whitelisting approach, and I think the version that we are using is pretty old at this point (like, maybe 2 years?) At the time they weren't actively developing it anymore but I just looked and it looks like at least someone is now maintaining a version of it on Github, so we'll see if we can get it updated to a new version which might add whitelisting for some of the newer CSS properties. But in general, that sanitization isn't going anywhere. The second thing it sounds like you are talking about is the built-in CSS rules that are part of the Redactor editor that we use to power our rich-text editing. I'm assuming you are trying to set HTML with the API and the CSS rules for the redactor are overriding some of it? I know that looking into upgrading or replacing it is on our list of stuff to do so we'll take a look at that CSS when we do that. Obviously we have Roll20CON coming up so that is where our focus is at right now but I think the next update after that is going to be looking at some of these things in a related way so we can look at it then.
1462909873
Lucian
Pro
API Scripter
Hey Riley, Ok, great, that sounds like a plan. I think there will be celebrations all round if you do manage to upgrade/replace redactor - I can't imagine that there are many users who haven't been perplexed by it at some point or other! :-) Lucian