Allow passing a string directly to dangerouslySetInnerHTML

[syranide]

In response to #1370 and to move the discussion along.

As far as I understand it, {__html: X} is not really intended as another safe-guard, but a remnant of how XHP worked/works.

Here's a fully backwards-compatible implementation, with a bunch of test-cases added to ensure correct functioning.

   raw     gz Compared to master @ 32b84a4c5ea32835b93a857cf00f0db86d6c755a
  -290    -34 build/JSXTransformer-previous.js
-19584  -3777 build/JSXTransformer.js
     =      = build/react-previous.min.js
 +3227   +434 build/react-test.js
  +421    +68 build/react-with-addons.js
  +170    +32 build/react-with-addons.min.js
  +421    +71 build/react.js
  +170    +23 build/react.min.js

[sophiebits]

Instead of checking innerHTML.__html === undefined, maybe it makes sense to check typeof innerHTML === "string"?

[syranide]

I thought about that and the primary reason I don't want to do that is; if you pass an object with a toString()-method (or number/boolean, not sure why you would, but w/e), this way it would actually work out correctly, which it wouldn't if you explicitly tested only for a string.

So apart from passing an object with an uintended __html property, this exactly matches the behavior of {__html: anything}.

[petehunt]

I think if we're going to do this we should just break it now rather than continue to support __html. This fix is pretty easy to automatically update as well so I don't think we need to do a deprecation and then removal. @zpao?

[sophiebits]

+1 for dropping __html if it's feasible from Facebook's end.

[zpao]

I'm down. Will be a bit of work to update internally but it's worth it. I think we should probably support both for a release though.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[sebmarkbage]

__html does provide another safe-guard. It allows you to have whitelisted functions that passed a blessed response through indirections.

function frameworkLevelBlessedFunction(name) {
  return { __html: 'I\'m sure this is safe, ' + encode(name) + '!' };
}

Now you can lint every other caller...

function indirection(data) {
  return frameworkLevelBlessedFunction(data.name);
}

and components...

render() {
  return <div dangerouslySetInnerHTML={indirection(this.props.data)} />;
}

to make sure that none of them declare __html but it's still safe for everyone to use dangerouslySetInnerHTML which doesn't have to be lint:ed against. It's really not dangerous. The dangerous part is actually the __html property.

However, you ALWAYS have to lint against people setting innerHTML on anything.

I would be fine with allowing innerHTML accepting a string and we will just forbid the use of any innerHTML identifier at FB.

[sophiebits]

That logic makes sense to me. I'm in favor of keeping the current situation because it's too hard for most people to set up lint rules to forbid innerHTML and people will end up just writing insecure code instead.

[syranide]

@sebmarkbage Interesting perspective, but if __html is the "safe-guard" (I'm inclined to agree that it's a good thing now), then dangerouslySetInnerHTML is quite redundant no? Anyway, it has always bugged me that it's called dangerously_Set_InnerHTML, no other property is named like that. I would assume that proper naming should be dangerousInnerHTML, unsafeInnerHTML or w/e.

[sebmarkbage]

@syranide agreed. It's misnamed. It always bothered me as well. I'd be happy to change the name. What do you think @zpao ?

[zpao]

Is there a pattern for this in other frameworks / libraries / languages to indicate it's unsafe?

"dangerous" is slightly more annoying to type than "unsafe", so let's do that.

dangerousInnerHTML={__html: ""}

(We should probably also deprecate nicely with a warning and letting both work for a release)

[syranide]

@zpao I personally prefer the sound of dangerous myself, unsafe sounds relatively kind of safe, whereas innerHTML really is bite-your-face-off dangerous. As for other frameworks, I believe .NET uses "unsafe" for C++ calls.

PS. We should also document the purpose of {__html: ''}, when explained correctly it makes a ton of sense. Writing dangerousInnerHTML={{__html: ''}} doesn't make as much sense, but dangerousInnerHTML={wrappedSafeString} does.

[syranide]

Follow-up; perhaps there should be a dangerousDefaultInnerHTML or such too (default as that's what we use elsewhere).

[syranide]

I split the naming discussion into #2134

So this PR should probably be closed. I'm leaving it open for now until another issue/PR is opened to address the "lack of documented justification" for {__html: ...}. Feel free to close it otherwise.

EDIT: I meant "lack of documented justification", not "lack of justification" :).

[sebmarkbage]

Not sure what you mean about a new issue/PR. Are there still concerns about it's justification?

[syranide]

@sebmarkbage Not in my opinion, but AFAIK the justification is not documented anywhere.

[sebmarkbage]

Good point. Let's add an explanation to the docs before closing this.

On Sep 4, 2014, at 1:06 AM, Andreas Svensson [email protected] wrote:

@sebmarkbage Not in my opinion, but AFAIK the justification is not documented anywhere.

—
Reply to this email directly or view it on GitHub.

[syranide]

Opened #2256 for tracking documentation of justification and #2257 for renaming of dangerouslySetInnerHTML, closing this.