Skip to content
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

shakespeare-js does not escape #{...} interpolated strings #55

Closed
joeyadams opened this issue Apr 6, 2012 · 7 comments
Closed

shakespeare-js does not escape #{...} interpolated strings #55

joeyadams opened this issue Apr 6, 2012 · 7 comments

Comments

@joeyadams
Copy link

I wrote a simple "hello world" style program for shakespeare-js:

{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE QuasiQuotes #-}

import Data.Text    (Text)
import Text.Julius

import qualified Data.Text.Lazy.IO as TLIO

shouldBeEscaped :: Text
shouldBeEscaped = "\" + alert('XSS') + \""

foo :: JavascriptUrl url
foo = [js|
    var s = "#{shouldBeEscaped}";
|]

main :: IO ()
main = TLIO.putStrLn $ renderJavascriptUrl (\_ _ -> undefined) foo

It turns out that no string escaping happens at all! This is what the program outputs:

var s = "" + alert('XSS') + "";

Is this a bug, or does shakespeare-js not automatically escape interpolated strings? I'm guessing the latter, based on this sentence buried in the Shakespearean Templates documentation:

Julius allows the three forms of interpolation we've mentioned so far, and otherwise applies no transformations to your content.

This might be intended behavior, but it is extremely worrisome:

  • The documentation doesn't make it obvious that no escaping is done. Rather, it suggests the opposite, by vaunting principles of type safety and well-formed content.
  • It is inconsistent with Hamlet's #{...} syntax, which does perform the appropriate escaping.
  • What if a future version of shakespeare-js does perform escaping of some sort? That would silently break any programs that do their own escaping.
@meteficha
Copy link
Member

It looks like a bug for me.

@snoyberg
Copy link
Member

snoyberg commented Apr 6, 2012

This is the intended behavior. Whether we should change it, or how we should document it, are good questions. Can you raise them on the mailing list?

@snoyberg snoyberg closed this as completed Apr 6, 2012
@gregwebs gregwebs reopened this Apr 7, 2012
@gregwebs
Copy link
Member

gregwebs commented Apr 7, 2012

I also think this behavior is problematic. We should be documenting how to avoid any hassles from automatic safety, not how to make things safe.

@snoyberg
Copy link
Member

snoyberg commented Apr 7, 2012

This is far from a simple matter, which is why I closed the issue in favor of discussing it on the mailing list. The presumption underlying it is that Julius interpolation is fixated on values which will be included in JS string literals. I'm not convinced that this is what people are expecting: it may be quite disconcerting for a user to try and interpolate some JS code and have to escaped as if it were appearing inside of string.

@gregwebs
Copy link
Member

gregwebs commented Apr 7, 2012

Yeah, so from a security standpoint it matters little whether it will be a string literal or something else. To be secure we shouldn't allow any code whatsoever, only JS values.
This is plausible: if someone wants to insert JS code, they can insert a widget.
It would also be a major change: instead of ToJavascript we would probably need to use ToJSON.

@snoyberg
Copy link
Member

snoyberg commented Apr 8, 2012

So should we only interpolate Data.Aeson.Value values? When we
interpolate a Text, should it have quotes automatically added? Should we
apply entity escaping as well to properly avoid XSS attacks? There are a
lot of questions, and I think this is anything but obvious. That's why I'd
rather close the issue and discuss it on the list.

@gregwebs
Copy link
Member

gregwebs commented Apr 8, 2012

@joeyadams do you want to ask this on the mail list?

@gregwebs gregwebs closed this as completed Apr 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants