aurelia / templating-binding

An implementation of the templating engine's Binding Language abstraction which uses a pluggable command syntax.
MIT License
32 stars 26 forks source link

innerhtml.bind does not work #7

Closed AshleyGrant closed 9 years ago

AshleyGrant commented 9 years ago
<div innerHTML.bind=“some.expression.here”></div>

Does not currently work.

plwalters commented 9 years ago

As a basic start to cleaning the HTML from script injection we could use something like this (pseudo-code) -

var SCRIPT_REGEX = /<script\b[^<]*(?:(?!<\/script>)<[^<]*)*<\/script>/gi;
while (SCRIPT_REGEX.test(text)) {
    text = text.replace(SCRIPT_REGEX, "");
}
element.innerHtml = text;

Taken from here - http://stackoverflow.com/questions/6659351/removing-all-script-tags-from-html-with-js-regular-expression

This should effectively remove scripts, but I still think in the documentation we should be very clear that you should never trust user input by displaying it this way without better security measures.

EisenbergEffect commented 9 years ago

Need to think about this a bit. We want to have a default behavior but also want to make it easy to:

  1. Replace the sanitization for your entire app.
  2. Use custom sanitization on a case-by-case basis

An attached behavior can work this way.....using the withOptions metadata to embed multiple properties. But, we may need a core improvement so that the default case is a bit easier to use.

goranobradovic commented 9 years ago

Just a thought, when sanitization is done, I have not tried, but it might be possible to inject script without tag on event of html element (i.e. onclick=eval(....)), so it might be needed to sanitize too.

AshleyGrant commented 9 years ago

Why can't we just add an attached behavior for this as part of the default behaviors? That way if the user needs something other than the basic naive sanitization they can override it?

EisenbergEffect commented 9 years ago

That's what I'm thinking. I'm just pondering how to write that behavior so it can look nice for default use:

inner-html.bind="some.prop"

and for custom one-offs

inner-html="value.bind: some.prop; sanitizer.call: mySanitizationFunction"

We can do one or the other today...but not sure the syntax will support both with the same attached behavior. I might have to make the syntax for options a bit smarter somehow.

devzsolt commented 9 years ago

@EisenbergEffect when do you plan to fix this bug at least with the basic sanitization recommended above? Until then do we have any workaround to inject HTML? I want to integrate CKEditor into an admin app.

EisenbergEffect commented 9 years ago

@miu-zsolt You can easily work around this by implementing a custom Attached Behavior.

devzsolt commented 9 years ago

Thank you @EisenbergEffect, after an hour of trying (lots of new concepts for me) I was able to implement one-way data binding via a custom attached behavior. Could you please give me a hint, how can I detect changes in the innerHTML? My element is a DIV contenteditable="true"

Btw. Aurelia docs uses in the examples this import:

import {Behavior} from 'aurelia-templating';

however using your skeleton app as a base, it worked for me only this way:

import {Behavior} from 'aurelia-framework';
EisenbergEffect commented 9 years ago

To detect changes in the html itself, you would need to use a DOMMutationObserver.

devzsolt commented 9 years ago

Thanks for the tip, but I found a more elegant solution: attaching an on change event handler to the CKEditor instance. Nice to see both that Aurelia is so customizable and you are so helpful. Keep up the great work Rob!

AshleyGrant commented 9 years ago

This has been resolved by https://github.com/aurelia/templating-resources/pull/19

EisenbergEffect commented 9 years ago

Going out in the next release.