lionleaf / dwitter

Social network for short js demos
https://www.dwitter.net
Apache License 2.0
766 stars 69 forks source link

Error display let's you get outside the iframe and affect the main site #373

Open lionleaf opened 6 years ago

lionleaf commented 6 years ago

While I'm on the security side; it is possible to post dweets that, while maybe not a real security threat, can really affect the flow of the website by throwing exceptions with interesting strings. For instance hundreds of newlines.

One way to solve it is to deal with those specific instances, ie. don't allow it to grow with newlines etc.

But I suggest we simply disable the viewing of errors when you are not editing the code. Now this breaks some old dweets that relied on throw "text" as a cheap way to print text, but there aren't too many of them. How do people (especially authors of such dweets) feel about this change? As far as I have seen, most of those dweets are fun small ones; not big efforts with hours of golfing (but I could be wrong).

You'd still be able to show of clever throw tricks, you would just have to have the users edit the dweet first.

MiraR commented 6 years ago

I think it is a good idea, and the trade-off is worth it. However, that might be easy for me to say since I don't personally have any dweets with throw.

joeytwiddle commented 6 years ago

The error message medium has gained some popularity in the last week, and if it isn't shut down I imagine it will eventually be explored in depth(*). I appreciate it was not in the original concept of Dwitter, just a quirky side-effect. I won't mind much if it is muted, but somebody should advocate for it, so...

Here are some downsides to muting the errors:

Here is why I like the error messages:

Suggestion:

The proposal to hide error messages is a simple solution, and simplicity is a virtue. But on the other hand, it should be relatively simple to limit the error output too:

var sanitizedErrorMessage = String(error).split('\n').slice(0,12)
                              .map(line => line.substring(0, 80)).join('\n');

That should limit error messages to at most 80 x 12. But that still might not fix the issue yonatan raised or the kipkat attack: throw '\n'.repeat(6+6*S(t*9))

So I haven't even fully convinced myself we should keep the errors visible. 😢

(*) It appears I was wrong about that. Nobody has touched it for over a week!

lionleaf commented 6 years ago

Alright, I've made up my mind. Even though there's been a good amount of fun error-dweets lately, I want to hide the error display until you edit (you can still view the dweets if you edit it).

If anyone is interested in implementing that fix, just assign yourself to this issue. If not I'll do it whenever I have some time to sit down properly with dwitter again. (I'm basically busy for the next 3-4 weeks, so you'll have some time)

That would also fix #389