Safe HTML by default #114

Closed
pupeno opened this Issue Jul 18, 2015 · 9 comments

Projects

None yet

3 participants

@pupeno
pupeno commented Jul 18, 2015

It would be nice if Hiccup escaped strings by default, only requiring something special to avoid escaping, as to make it very hard to make apps that are XSS exploitable.

@weavejester
Owner

This is planned, but requires a rewrite of the entire library. The rewrite is currently in the 2.0 branch.

@pupeno
pupeno commented Jul 18, 2015

I started playing around this, modifying hiccup to try to achieve this and I can see it's far from trivial.

@weavejester
Owner

Hiccup 1.0 uses strings for performance. Unfortunately this means that there's no distinction between a safe and unsafe string.

@pupeno
pupeno commented Jul 18, 2015

Selmer considers all string to be unsafe except does inside a vector like this: [:safe string]. Wouldn't something like this work?

@weavejester
Owner

No, because Hiccup compiles into expressions of string concatenation. This makes it fast, but relies on the assumption that it's just dealing with strings. For example:

(html [:span {} (foo "bar")])

Compiles to:

(str "<span>" (foo "bar") "</span>")
@gyim
Contributor
gyim commented Dec 12, 2015

What do you think of this solution? gyim@535282b ?

The basic idea is to introduce a SafeString class that is not escaped, everything else is. This class can be easily plugged into the HtmlRendered protocol. I am not sure why this solution came up before: is it too much of a hack? Or did I overlook an important use case?

I think the most common use case for NOT escaping a string is when someone has multiple partial templates, each "pre-compiled" with the (html) macro for performance. So I introduced a (html {:partial? true}) option which returns a SafeString. This way the user rarely has to deal with enabling/disabling escaping directly, and the :partial? true option suggests that the return value of this function is "not the real thing", just something that is useful inside a "real" template. But this is just a syntactic sugar, using (safe-str (html ...)) would also work.

@weavejester
Owner

@gyim, that solution isn't safe HTML by default, it's safe HTML if you set an option flag. I think if we're going to do this, we should do it properly, instead of trying to come up with partial implementations in an attempt to maintain backward compatibility.

We already know what safe HTML by default looks like:

(html [:p "<foo>"])
=> #hiccup.core/RawString["<p>&lt;foo&gt;</p>"]

(str (html [:p "<foo>"]))
=> "<p>&lt;foo&gt;</p>"

(html [:p (html [:span "foo"])])
=> #hiccup.core/RawString["<p><span>foo</span></p>"]

We just need to implement it. At this point I'd accept a patch that does just that and nothing else.

@gyim
Contributor
gyim commented Dec 12, 2015

Makes perfect sense, I just did not want to introduce any breaking changes. If (html) returns a RawString instead of a String and the caller makes a string operation on it, it could cause a crash in existing code. But I agree that safe HTML by default is the right thing to do, so it could be a reasonable tradeoff.

I will rework the patch and send a PR.

@gyim gyim added a commit to gyim/hiccup that referenced this issue Dec 12, 2015
@gyim gyim Escape all strings by default
This commit implements issue #114 (Safe HTML by default). It introduces
a few breaking changes:

1. Since all strings are escaped by default, adding HTML markup from
string literals or expressions will not work. Use `hiccup.util/raw-str`
to prevent these values from being escaped.

2. `hiccup.util/escape-html`, `hiccup.core/h` and `hiccup.core/html`
returns a RawString object instead of a String. They can be converted to
java.lang.String with the built-in `str` function.

Note that unlike `hiccup.util/html`, macros in `hiccup.page` (`html4`,
`html5`, `xhtml`) *still* return a string so that their result can be
passed directly to Ring. In the rare cases when the output is embedded
to another html page (e.g. into an iframe) it should be also tagged with
`hiccup.util/raw-str`.
0964377
@gyim gyim added a commit to gyim/hiccup that referenced this issue Mar 23, 2016
@gyim gyim Escape all strings by default
This commit implements issue #114 (Safe HTML by default). It introduces
a few breaking changes:

1. All strings that are passed to `html` are escaped. To support raw,
unescaped strings a new object type (RawString) was introduced. Use
`hiccup.util/raw-string` to prevent a string from being escaped.

2. To allow the nested usage of the `html` macro, `html` now returns a
RawString object instead of a string. It can be converted to a string
with the `str` function.

3. The `h` helper function was removed.
db5d39d
@weavejester
Owner

Fixed by #122.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment