openhab / openhab-webui

Web UIs of openHAB
Eclipse Public License 2.0
224 stars 242 forks source link

Enable widgets to use Javascript #626

Open hubsif opened 3 years ago

hubsif commented 3 years ago

Hi!

The problem

While trying to rebuild my "tablet screen" using OH3 Main UI with what I had done so far with HABPanel, I failed right away with my first widget in the upper corner which is a simple digital clock. I couldn't find a clock standard widget, so I started diving into building custom widgets. I then discovered that it seems to not be possible to create a clock since it's not possible to use Javascript in custom widgets (please correct me if I'm mistaken, it would invalidate this whole issue).

Your suggestion

I think a lot of upcoming custom widgets might want to make use of the possibilities and flexiblity that Javascript brings, and I think this is required to have the community build a whole lot of "cool" widgets. Having read that MainUI is meant to be able to replace HABPanel sooner or later, I'm of the opinion this feature might be somewhat crucial for its success.

So, I started experimenting again and came up with the result below (which took a lot more time than it seems 😄 ).

And again, since I'm not sure what's desired for OH3 and being a total newbie in vue and f7, I'm filing this as a suggestion to think about, not a ready to use pull request.

Actually, the only change is this so far:

diff --git a/bundles/org.openhab.ui/web/src/components/widgets/widget-mixin.js b/bundles/org.openhab.ui/web/src/components/widgets/widget-mixin.js
index 148162a..645978c 100644
--- a/bundles/org.openhab.ui/web/src/components/widgets/widget-mixin.js
+++ b/bundles/org.openhab.ui/web/src/components/widgets/widget-mixin.js
@@ -68,6 +68,19 @@ export default {
       return true
     }
   },
+  mounted () {
+    if (this.context.component.script) {
+      let script = document.createElement("script")
+      script.text = this.context.component.script;
+      let oldscript = this.$el.getElementsByTagName('script')[0]
+      if (oldscript)
+        this.$el.replaceChild(script, oldscript)
+      else
+        this.$el.appendChild(script);
+    } else if (this.$el.getElementsByTagName('script')[0]) {
+      this.$el.removeChild(script);
+    }
+  },
   methods: {
     evaluateExpression (key, value, context) {
       if (value === null) return null

which enables widgets to have an additional script: parameter defining some javascript. Example:

uid: widget_ca6fb002bd
component: f7-card
config:
  content: clock
  class: myclass
script: '
  setInterval(function() {
    document.querySelector(".myclass .card-content").innerHTML = new Date().toLocaleTimeString();
  }, 1000);'

Issues with this I've found so far:

With injected Javascript the first thought always is "security issues". I'm not a JS guru, but to my understanding this feature here is not, since the JS is run on client side only.

Your environment

OH3 MainUI, master branch of this repository at 9f0e43b

Please let me know what you think.

hubsif commented 3 years ago

Sorry, I seem to have used the wrong template and to not be able to adjust the labels. Please change "bug" to "enhancement". Thanks!

ghys commented 3 years ago

Interesting, but I can't help seeing this as "remote code injection/XSS vulnerability as a feature" :) This seems dangerous, and the fact that it's "only" client code doesn't mean we can brush that off. If you're not careful you could end up with something that makes malicious requests to the API, or even to external services, stealing sensitive info from e.g. your things' configuration, logging what you do, etc. Note that the expression parsing is not completely immune from this either, there are probably ways to exploit it too to make it execute Javascript (https://github.com/donmccurdy/expression-eval#security), but at least it's not that blatant and presented as a feature. I'd be more willing to consider a feature that can load code from .js files from the server, in /static/ for example (and restricted to the server with a CSP), so the administrator can better control it.

hubsif commented 3 years ago

With my poor knowledge of JS security I certainly don't want to start a general discussion, but I'd appreciate if you (or someone else) could help me understand better:

  1. the code will only be executed client side and I can run whatever code I like client side anyway Meaning, with code only being on client side, I can't see how that's different from opening developer tools in my browser and run whatever else code I like to.
  2. the UI is independent of/using the API of the core With that (and the code not being run in the core) I would think that all security lies within the API.
  3. custom JS is served by the core That's actually the one issue I can see here: The core will serve the JS to all connected clients and it could e.g. contain code that collects data and sends it to somewhere. I think that this is something users just have to trust for two reasons:
    • openHAB itself could also contain such code - as a user I already trust that it's clean
    • I somewhat would trust the community to discover such things, since the widgets will be shared open source on the marketplace (actually that's the same with HABPanel right now, where people install the JS of e.g. the popular openweathermap widget)
    • additionally, the marketplace could warn me that the widget contains potentially malicious Javascript code
  4. Difference to .js files At first js files in /static/ also sounded more secure to me. On second thought: Where's the actual difference? When users installed a shared widget (e.g. from the marketplace) it wouldn't matter if the (malicious) JS code is part of the yaml file or if it's in an extra file, does it? Actually, I wouldn't mind if it works by putting js files in /static/ just like it is with HABPanel now. Without knowing the difference in security, I would just think that having it as part of the yaml definition would it make easier for users to share and for users to install.

Reading through my own post makes me realize that I actually might "discuss" already 😄 , though my primary goal is to understand the security issue (technically) (with my second goal being of the opinion that sooner or later widget authors might call for JS support in some way) (and the third not wanting to give up my clock 😜 )

rgrollfitz commented 3 years ago

I would agree on the additional possibilities that the usage of javascript inside of widgets would have and like the idea with the external *.js file.

  1. Difference to .js files At first js files in /static/ also sounded more secure to me. On second thought: Where's the actual difference? When users installed a shared widget (e.g. from the marketplace) it wouldn't matter if the (malicious) JS code is part of the yaml file or if it's in an extra file, does it?

I could imagine that an external *.js would make it easier (and quicker) to detect malicious code - If I write a widget with ~2k lines of YAML 'code' it could be burried between the lines (as it might be partly possible with the expressions already) and would take some time to detect.

hubsif commented 3 years ago

Thanks Rainer. I'm not of the opinion that external js would make it easier. I think it mainly depends on how the js code is formatted. It can still be pretty formatted in the yaml file, with normal newlines like in my tiny example above. On the other hand, a js file can be hard to read, e.g. when minified (or even obfuscated).

I'm actually thinking a lot of the ease of exchange. I think an "all-in-one" file would make the process of sharing widgets so much easier, as there would be no need for the user to download extra files and for him to be able to put it at the right location. That's what it is with HABPanel right now and I think a lot of people have had issues e.g. where to put the js file, also because locations are different and dependent on the way OH has been installed. Instead a user could just search (hopefully from within mainUI someday) for widgets and click "install" and that's it - including js. Same for widget developers: On the one hand they could just code right away using the editor and wouldn't have to edit an extra file located at the server. On the other hand they could just click on "share/upload" and would not have to collect files and pack them to share their work.

And from all this it might not seem like but I'm actually a big friend of security! Yet I also think there's a definite need for JS widgets and that people will always like those "fancy" widgets and if not available directly, get them from github or somewhere else and install them by just following the instructions there. If those widgets are managed centralized (by the marketplace) they can be reviewed much easier. And the user can be given a warning and at least be made aware of potential risks.

rgrollfitz commented 3 years ago

On the other hand, a js file can be hard to read, e.g. when minified (or even obfuscated).

Fair point - this was only a first thought, what the benefits of an external js file could be. So maybe @ysc have other things in mind here, which I'm not aware of.

I'm actually thinking a lot of the ease of exchange. I think an "all-in-one" file would make the process of sharing widgets so much easier, as there would be no need for the user to download extra files and for him to be able to put it at the right location.

You're right - one click soloutions would be the best case scenario - especially from a UX perspective (for users as well as developers, as you already said). The same holds true for all the images that comes with widgets and the custom path that a user/dev has to define...

Maybe both (external js as well as images) could be packed into a sort of archive, which extracts automatically into a widget-specific folder with a default structure - so a dev could work with relative paths then, which follows a well documented pattern?!

One downside might be, that this approach would it make even harder to detect 'dirty code'. But as you said, this isn't much different from HabPanel right now and users would be able to discover such things anyway. A user-rating feature might help here too.

Instead a user could just search (hopefully from within mainUI someday) for widgets and click "install" and that's it - including js.

I'm hoping this kind of widget-browser will come one day. I remind me, that @ghys mentioned that some time ago and referred me to a discussion which I can't find anymore :-P

hubsif commented 3 years ago

Argh, I totally forgot about images! That means that "all-in-one" wouldn't be possible anyway (unless images could get (base64) embedded, which is probably unlikely). And since you then have to "pack" it all anyway, it doesn't matter much if the javascript also goes to an extra file (though the advantage of "real-time editing" would still speak for js embedding 😏 ).

There might be another obstacle though: I don't know if openHAB is currently designed to be able to put files into a static folder - though that can certainly be overcome.

And there's another thing that came to my mind which I think might be problematic for a "one-click-userexperience": Item creation. Items must perfectly match to the widgets and it can be quite an effort to create them all. As you know, to make this a little easier, right now widget developers provide item files, e.g. for the openweathermap widget. I would love to see this included into a one-click-solution some day! 😎

It seems we've become a little off-topic here, perhaps this ticket would be better placed in the community forums after all ... 😄

Nadahar commented 2 years ago

It's a shame that this seems to have stalled or ended nowhere, it seems clear to me that the current "widget regime" is too limited to do a lot of things that you'd want to do. I'll not pretend to know how, it's just clear to me that they have to be able to contain "custom logic" in some way or form to be really versatile and powerful.

As always, "with power comes responsibility" and every extra possibility usually also adds some possibility for abuse. But, letting that scare us into just restricting everything isn't the solution, it will just mean that people won't be able to do what they want to do, and might end up choosing other ("less secure") solutions instead. Instead, one must try to find reasonable compromises. This isn't really thought through, but one thing that could potentially help a little bit would for example be to scan all code and impose some basic restrictions, like disallowing non-relative URLs etc. I the code don't pass the test, it won't be allowed to run.

flo-mic commented 2 years ago

@ghys is there any plan to enable the second option to allow js loading from an static folder on the file system. Could be either static or also automation from the openhab-conf directory.

I'd be more willing to consider a feature that can load code from .js files from the server, in /static/ for example (and restricted to the server with a CSP), so the administrator can better control it.

ghys commented 2 years ago

No plans unless there are clearly identified extension points for which custom JS would be useful and allowed. This could be adding functions to the expressions' context, defining entire widgets/Vue SFCs with custom logic, etc. Simply injecting random user JS code doesn't always lead to anything useful, and it may even disrupt or harm the normal behavior.

ThaDaVos commented 1 year ago

TLDR; My opinion on a solution as I am also running into the limited widget functionality

I am also interested in the above but more on the side of opening the Vue SFC feature set to the yaml code - for example, if one could define a computed and access this inside the widgets expressions, one can eliminate repeated code - this is just an example.

If it's all about preventing remote code injection/XSS vulnerability - then one be better of removing the custom widget system - especially through the Community Marketplace - as currently one can easily add a widget which through methods, can send data externally already as certain api's - like document.* - aren't prevented either...

Coming back to the issue itself - one could look into opening up certain VueJs based features, like computed and data (or vue3, not sure if it's used yet, setup) - and add a security layer which checks the code server side and prevent loading the widget if it contain anything possible malicious.

Secondly, in my opinion - it's always also the user's task to prevent adding something malicious, like the Developer Console in Mozilla currently does by preventing Copy/Paste into it and you have to confirm, after confirmation it's the user's responsibility

I would suggest, if the above gets added - it is done through an opt-in (so by default you cannot) - maybe through a checkbox per widget or an plugin which needs to be installed.

I am willing to in my free time look into opening more API's to the front-end - so widgets can become easier to write, easier to maintain and more powerful in general - I just need someone to give me a little guide on how to start on extending or modifying this inside Openhab (I know Java etc, so that's not the thing)

Or an alternative - keep the widget editor in the front-end as is - and only open up more advanced widgets to bindings/addons - so it can't just simply be copy/pasted - adding to that a way to define the Vue Component/widget inside Java make it easier to check for security issues as you could just not allow certain api's to be called.