Bug 6873 - normal web settings usage & namespace collisions
: normal web settings usage & namespace collisions
Status: CLOSED FIXED
Product: Logitech Media Server
Classification: Unclassified
Component: Web Interface
: unspecified
: PC All
: P2 minor (vote)
: 7.x
Assigned To: Michael Herger
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-30 19:02 UTC by Peter Watkins
Modified: 2009-07-31 10:16 UTC (History)
1 user (show)

See Also:
Category: ---


Attachments
prepending "pref_" to settings (4.91 KB, text/plain)
2008-04-23 04:00 UTC, Michael Herger
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Watkins 2008-01-30 19:02:19 UTC
The normal technique for web UIs for settings in SC7 is to have HTML controls with a name matching the setting name within the namespace -- but not including the full namespace path. For instance, the "port" setting in plugins.BottleRocket has an HTML control with a name of "port". This can lead to problems if the setting name conflicts with names meaningful to SC7, e.g. "command", "subcommand", "p0", etc.

The web settings ought to use fully qualified preference names, e.g. "plugins.BottleRocket.port".

To make this backwards-compatible, SC7 could 
 - issue something like a deprecation warning if the settings method encounters names that appear to be unqualified
 - but actually discard everything to the left of the rightmost period
 - and not take action on names known to be meaningful (command, subcommand, pN, ...)

The settings pages that ship with SC7 would then need to use qualified names like "server.screensaver" in order to avoid the deprecation warnings, and encourage plugin developers to follow suit.

For more discussion, see http://forums.slimdevices.com/showthread.php?t=40665
Comment 1 Blackketter Dean 2008-01-31 08:02:25 UTC
Michael: do we need this for 7.0?
Comment 2 Michael Herger 2008-01-31 08:11:39 UTC
No. That's not a change I would do for 7.0. This requires quite a few changes. Even if they're simple, we risk to break things because they're many. I target for 7.0.1. Ok?
Comment 3 Michael Herger 2008-03-07 17:28:51 UTC
bigger changes don't go into the 7.0 branch
Comment 4 Michael Herger 2008-04-23 04:00:31 UTC
Created attachment 3278 [details]
prepending "pref_" to settings

Peter - did you mean something like this patch? It's not using the ull pref name with namespace and everything, but simple prepending a prefix "pref_". And as TT uses the dot to access hash values, I've chosen the underscore. Using the full namespace and dots as separators would result in a lot of changes and code which is hard to read. prefs.language would have become prefs.${"$namespace.language"} or similar.

Also: this modification only protects pages calling the default pagehandler. Plugins implementing their own pagehandler wouldn't even be checked at all.
Comment 5 Peter Watkins 2008-04-23 17:20:50 UTC
Michael, that looks quite good to me.  
Comment 6 Michael Herger 2008-04-25 05:09:53 UTC
change 19149 - prevent use of SC internal keywords in settings pages
- prefix preferences in web page with "pref_"
- print error message if a page (3rd party plugin) doesn't use this prefix
- compatibility code left in, 3rd party plugins should still be compatible between 7.0.x and 7.1+
Comment 7 Michael Herger 2008-04-25 06:11:19 UTC
change 19155 - the plugins

QA - please test all of them thoroughly ;-)
Comment 8 Adrian Smith 2008-04-29 14:20:27 UTC
Michael,

As per email - I tend to think we are fixing a symptom of the main problem not the root cause here.  I think the root cause is that we accept command parameters whatever the web page is - if we restricted this to only certain urls, then we could allow any settings page or plugin to use any parameter other than perhaps one or two for player etc.

Views?
Comment 9 Chris Owens 2008-07-30 15:30:28 UTC
This bug has now been fixed in the 7.1 release version of SqueezeCenter!  Please download the new version from http://www.slimdevices.com if you haven't already.  

If you are still experiencing this problem, feel free to reopen the bug with your new comments and we'll have another look.
Comment 10 Chris Owens 2009-07-31 10:16:34 UTC
Reduce number of active targets for SC