[Bug] Problem with settings cache in chrome

[derpicknicker1]

What were you doing?

You (Gina) did a pull request on telegram plugin fabianonline/OctoPrint-Telegram#43
This update should prevent non admin users to see restricted settings.
I merged it and it seemed to work well.

What did you expect to happen?

When i'm not logged in, i expect not to see the restricted settings. When i'm logged in as admin, i expect to see the restricted settings.

What happened instead?

On Chrome:

• When i'm logged in and load the site , i can see the settings as desired. But when i logout and reload the page, i also can see the restricted settings.

• When i am not logged in at site load, i can not see the restricted settings, and when i log in as admin after that, i can not see them. I did page reload, but the restricted settings will not appear.

• When i clear the browser cache, it will work again until next logoff/login. So it seems to be a caching problem.

This only happens in Chrome, not in IE (FF not tested)

Branch & Commit or Version of OctoPrint

Tested with 1.2.16 (master) and 1.3.0rc1

Printer model & used firmware incl. version

Marlin 1.0.2

Browser and Version of Browser, Operating System running Browser

Chrome Version 54.0.2840.71 m (here the caching issue appears)
IE Version 11.0.9600.18499 (here there are no problems)

Link to octoprint.log

https://gist.github.com/derpicknicker1/8b73a9806b1c7bc909bb5abcd335e249

Screenshot(s) showing the problem:

Logged out and the red marked data should not appear (should be null).

Logged in as admin. The red marked settings values should not be null.

I have read the FAQ.

[derpicknicker1]

I tested this again. The same problem appears in FF. If i get no response on this issue, i will revert the PR and the user has to live wit the security issue. It's better than not beeing able to use the plugin. Also i get more and more requests on this starnge behavior.

[foosel]

I was sick the whole of last week and am only now catching up on everything that happened in the meantime. Give me some time please.

[derpicknicker1]

It's ok. A little note would have been nice.
Gute Besserung!

[foosel]

You can pretty much always assume I'm either out sick or otherwise away from the keyboard for similar pressing reasons when I don't react on stuff like this where I know I'll have to run some tests and analyse a potentially tricky situation before I can add anything meaningful to the conversation ;)

On the issue, I cannot reproduce this on 1.2.16 or 1.2.17rc4.

Logged out, did a force refresh. Verified settings did not contain sensitive data. Logged in. Opened settings (which triggers a new fetch of the settings backend, indicated by the spinner next to the Settings title). Verified that the response to THAT requests DID contain the sensitive data. Also verified that the settings pane of the Telegram plugin was populated correctly.

Now that is with 1.2.x (which you reported as also showing an issue here). With 1.3.0rc1, which - contrary to 1.2.x - in fact DOES set caching headers on /api/settings, I can reproduce. Here I needed to take changes in the user's login state into account which was so far missing. That's been fixed in 970880d

1.2.x in fact still does set "never ever dare to cache this" headers on the API responses, so if you (and your users) are seeing caching behaviour here, I'm a bit at a loss as to why. So if you are seeing this issue with 1.2.x (1.2.16 or 1.2.17rc4) then there must be something different between your steps and mine, or your setup and mine. Can you provide a screenshot of the Network tab without a request in focus, so that I can see the response code and potentially any "from cache" messages of the browser?

[derpicknicker1]

You are right. It's only in 1.3.0. I tested again with 1.2.16. Maybe i had some caching side effects. I also was a bit in a hurry when i tested it. Maybe i mixed up the VMs.
Do you need any other informations?

[foosel]

No, nothing more needed then.

In that case, nothing to do for 1.2.x and already fixed on devel/1.3.0. Will be part of 1.3.0rc2. Users encountering this issue can always roll back to the current stable release of OctoPrint (or switch to the devel branch) until 1.3.0rc2 goes live.

[derpicknicker1]

I tested again with 1.2.16 and got this issue again.
It seems like there is also some error in this version.

When i am not logged in and load the page the first time, i will get the "null-settings" correct. When i then login, and open the settings dialog, it will re-request the correct settings (without null values) but something seems not to be correct with bindings.
Everything the plugin will process on "settingsDialogOpen" will be displayed correct because it will be done after the correct settings are loaded on opening the settings dialog. But everything which is done by bindings from the jinja file will not work because the bindings will be done on pageload before logged in. At this time only the "null-settings" are available.

I think after login, the settings should be loaded again and bindings for settings dialog (or whole page?) should be done again. Or is this a thing, the plugin developer should do? But as this is a "security feature" of octoprint itself, i think this should be done by octoprint.

This is the error i get in java console:

[derpicknicker1]

Same in 1.2.17

[foosel]

I think after login, the settings should be loaded again and bindings for settings dialog (or whole page?) should be done again.

I can't do that since a) as far as I know knockout doesn't even support that (yes, cleanNode does exist, however it doesn't remove everything) and b) it would be way to resource intensive, especially since I'd have no way to know if it's even necessary (I can't distinguish between bindings failing because there's an implementation error or because a plugin did not code defensively in case of protected settings). It also wouldn't help here, since your issue arises in a piece of HTML you generate programmatically from within your plugin's view model and inject into the page, so OctoPrint wouldn't be able to rebind that even if there existed such a mechanism since it wouldn't know about it.

I spent some time trying to debug this (and for the record, for me it does NOT happen on page load, but only after I try to open either the chat editor or the notification editor, both which have HTML injected & bound at runtime, long after the settings have been refreshed from the backend with the full set of data). And it looks like the reason is NOT that data is undefined but that the structure of the data differs between an initial call on page load and an updated call after a settings refresh. self.settings.settings.plugins.telegram.chats is an observable if called without admin rights first, because null gets mapped to an observable instead of an object by the knockout json mapper plugins. Consequent updates (a full replace of the settings structure would kill bindings, so I HAVE to update here) then fill that observable with an object with more observables in turn. If on the other hand the settings are first loaded as admin, self.settings.settings.plugins.telegram.chats is an object and the binding works just fine. So the issue is not that the binding target is unset here (chats is actually filled when opening the dialog) but that the path to the property you are attempting to bind against varies. That of course is a bug too, just a different one than anticipated.

I've now modified the server code such that the restrictor will guess the data type of the restricted value first by taking a look at whether its a dict, list or other value, and only replace other values with None. dicts and lists will be replaced with empty versions. That makes the binding work again even on subsequent updates of the settings. I'll push that with the upcoming 1.2.18 release. The commit is already available on the maintenance branch and I'll also naturally push it up to devel to also be part of 1.3.0rc2. Also made me find another bug in the restriction functionality, potentially causing settings defaults to be altered during runtime. That's also fixed now.

However, your plugin still has the pre-1.2.16 code in place inside on_settings_load, which sets chats to None (I know that was from my own PR, and had I realized that binding issue earlier I'd of course done that PR differently). That needs to be changed so that chats gets replaced with an empty dict in case of restrictions:

diff --git a/octoprint_telegram/__init__.py b/octoprint_telegram/__init__.py
index 41c4e5f..66ecb23 100644
--- a/octoprint_telegram/__init__.py
+++ b/octoprint_telegram/__init__.py
@@ -762,10 +762,10 @@ class TelegramPlugin(octoprint.plugin.EventHandlerPlugin,
                data = octoprint.plugin.SettingsPlugin.on_settings_load(self)

                # only return our restricted settings to admin users - this is only needed for OctoPrint <= 1.2.16
-               restricted = ("token", "tracking_token", "chats")
-               for r in restricted:
+               restricted = (("token", None), ("tracking_token", None), ("chats", dict())
+               for r, v in restricted:
                        if r in data and (current_user is None or current_user.is_anonymous() or not current_user.is_admin()):
-                               data[r] = None
+                               data[r] = v

                return data

Otherwise, while the core implementation will return stuff correctly, it will then be nuked by that piece of code. I've prepared a PR with the above patch against the plugin repo, see fabianonline/OctoPrint-Telegram#52. Since 1.2.18 first has to go through at least one release candidate cycle, it might be faster to push out a new plugin release if you want to see that solved ASAP. I gave it a quick test against 1.2.17 and I could no longer reproduce the issue.

As a general note: You as the plugin author know which bindings you depend on in your view models that are marked as restricted in the backend. There's no way for OctoPrint to figure that out in the frontend side of things without attempting a full fledged source code analysis and I'd be doubtful even then this would be reliable. No issue should arise with single value restrictions, but if you depend on restricted hierarchical data, code defensively. Personally I would have created a couple of custom observables for my editor properties and synched those with the settings data on dialog open and close, instead of dynamically creating new elements on the DOM and constantly adding new data bindings to individual spans.

[foosel]

1.2.18rc1 and 1.3.0rc2 are out