Closed botandrose closed 8 years ago
+1 from me
cc @elm-community/packages
update comment to note that it doesn't convert them to vdom nodes
@eeue56 Good call. Added, squashed, and force pushed. WDYT?
Seems like a security hole, worth documenting?
Probably worth a caution about "untrusted input" ... that is, you wouldn't want to insert innerHtml
that just anyone can enter.
Struggling with the best way to word this. Perhaps appending "Also note that passing untrusted input to this function could enable XSS attacks."?
I'm not sure it should necessarily fall on this package to document that concern. This function is just a wrapper for something you can already do and folks are already doing with elm-html.
I would even go further and suggest that the property name should be changed to indicate the security risks, naming it dangerouslySetInnerHTML
like React does:
https://facebook.github.io/react/tips/dangerously-set-inner-html.html
On the theory that some action is better than inaction, I'm going to merge this and then add a security warning to the docs.
Looks good to me. Thanks, Max!
One other idea occurs to me -- it may be overkill (in fact, it probably is overkill).
Ruby has this concept of things being "tainted" or "trusted", in order to mark whether they come from.
We could adapt that idea to the type signature. One version would be something like this:
type alias TrustedString = String
innerHtml : TrustedString -> Attribute
This would actually be fairly non-intrusive to the caller, since you could still provide a plain-old-String ... the TrustedString
in the signature would merely be a kind of documentation.
The other approach would be to actually make Trusted
a real type ... something like:
type Trusted a =
Trusted a
trust : a -> Trusted a
trust = Trusted
extract : Trusted a -> a
extract trusted =
case trusted of
Trusted a -> a
Then, the type signature would be something like:
innerHtml : Trusted String -> Attribute
So, this would be more intrusive, in the sense that you'd have to explicitly trust your strings, but in a way that's also it's benefit.
So, I just throw it out there as a possible idea -- as I say, it may well be overkill.
Yeah, I've actually been thinking about the same thing... that Rails' SafeBuffer approach might lend itself really well to Elm, because I think we could leverage the type system to do most of the work of enforcing XSS safety. For reference: http://yehudakatz.com/2010/02/01/safebuffers-and-rails-3-0/
Probably out of the scope of this PR, but it's definitely something I've been thinking about.
On Mon, Apr 11, 2016, 9:31 AM Ryan Rempel notifications@github.com wrote:
One other idea occurs to me -- it may be overkill (in fact, it probably is overkill).
Ruby has this concept of things being "tainted" or "trusted", in order to mark whether they come from.
We could adapt that idea to the type signature. One version would be something like this:
type alias TrustedString = String
innerHtml : TrustedString -> Attribute
This would actually be fairly non-intrusive to the caller, since you could still provide a plain-old-String ... the TrustedString in the signature would merely be a kind of documentation.
The other approach would be to actually make Trusted a real type ... something like:
type Trusted a = Trusted a trust : a -> Trusted atrust = Trusted extract : Trusted a -> aextract trusted = case trusted of Trusted a -> a
Then, the type signature would be something like:
innerHtml : Trusted String -> Attribute
So, this would be more intrusive, in the sense that you'd have to explicitly trust your strings, but in a way that's also it's benefit.
So, I just throw it out there as a possible idea -- as I say, it may well be overkill.
— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/elm-community/elm-html-extra/pull/4#issuecomment-208436377
I wrote up an issue re: using types for trusted string, for further discussion: #5
Howdy!
I've copying and pasting this little gem around for a few projects now, so I think its time to extract it into its own package. I think it might be broadly useful enough to warrant inclusion in this package. What do you think?