Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luci-base: menu and i18n plays very BADLY with sysauth=false in master #3563

Closed
hmh opened this issue Jan 25, 2020 · 7 comments
Closed

luci-base: menu and i18n plays very BADLY with sysauth=false in master #3563

hmh opened this issue Jan 25, 2020 · 7 comments

Comments

@hmh
Copy link
Contributor

hmh commented Jan 25, 2020

LuCI pages with sysauth=false (e.g. status pages that should work even for an user that is not logged in) are broken on LuCI master.

The symptom is that when you access the page with "sysauth=false" attribute, the menu disappears, as in: not even nodes that are also sysauth=false show up.

What should happen: the menu should list all pages, always, and require authentication if you try to access one of them while lacking the required credentials.

Translations are also broken, and doubly so:

  1. They try to XHR something from a node that is below /admin/, even if you are on a node on an entirely different subtree (like say, a /status node that should work unauthenticated and be the first page the user gets when he connects to the modem).
  2. Whatever the translation stuff tries to fetch is hiding behind a sysauth=true node, which renders it useless for any sysauth=false page [if the user is unauthenticated]

Maybe the /l10n/ or /i18n/ namespace (or even wherever luci-static ends up) should be used for anything the i18n mechanism needs? It really doesn't make sense to have it tied to /admin/.

jow- added a commit that referenced this issue Jan 25, 2020
Ref: #3563
Signed-off-by: Jo-Philipp Wich <jo@mein.io>
@jow- jow- closed this as completed in 8978aaf Jan 25, 2020
@hmh
Copy link
Contributor Author

hmh commented Jan 25, 2020

I am extremely impressed with the response time! THANK YOU!

@hnyman
Copy link
Contributor

hnyman commented Jan 26, 2020

This change apparently makes the whole menu structure browseable even before logging in.
I am not sure if that is wise, as that exposes knowledge about the installed package universe to the casual browser. (E.g, in netcafe context)

image

@jow-
Copy link
Contributor

jow- commented Jan 26, 2020

I had the same thoughts but eventually the entirety of LuCI will be moved to the client side and then this knowledge can be easily obtained by checking for specific files in /luci-static/ even when not authenticated.

@hmh
Copy link
Contributor Author

hmh commented Jan 26, 2020

Well, if you want to hide pages unavailable to the current authentication state (which MUST NOT depend on whether the user is in a page that requires authentication or not!!!!), you MUST have a way to access the authentication page.

The fact that it was working before was just accident, but it made the design defect less obvious: it would hit a page that requires authentication, with no way out (which is already broken: the menu would NOT show anything, not even pages that don't require authentication).

I am not sure what is the best design fix for this, but it isn't difficult to fix, here's an example:

  1. Whether a menu node is show or not would depend on the intersection of that node's auth requirements and the current auth state. If non-empty, show it. Otherwise, don't show it.

  2. There would be a menu node type that is always available when logged out, and which allows one to log on by redirecting to whatever landing page is desired (which would trigger the logon page). This node would get hidden when the user is logged in.

alternatively, just have the /admin node be an active node that doesn't require authentication, always show up on the menu, and redirects to firstnode() -- this would trigger the authenticaton.

That should be enough.

PS: edited with a simpler, better solution than the first one I wrote in the comment. twice :(

hmh added a commit to simetnicbr/simetbox-openwrt-feed that referenced this issue Mar 2, 2020
We want to:
* Require sysauth to change configuration and trigger actions.
* Require sysauth on SIMETBox "status" page, because it reveals the
  virtual label and other sensitive information.
* NOT require sysauth to view results / SIMET landing page

Note: only change the sysauth field to set it to false, otherwise it
breaks things on OpenWRT 20.x (at least as of 2020-01-26).  Leave it
untouched to require authentication.

Refer to:
* openwrt/luci#3573
* openwrt/luci#3563
@sanitariu
Copy link

I think closed but unlocked door is much much better than open and unlocked door.
Hnyman there is nothing wrong when you admit that the commit is more bad than without it.
You can easy fix this.

@sanitariu
Copy link

.... and most important it is UGLY.....

@hmh
Copy link
Contributor Author

hmh commented Apr 13, 2020

The real issue you guys have seems to be a want for "privacy" of the menu tree.

The only way to do this is to filter the menu tree on the "embedded-device-side". Nothing else will work on openwrt-20 (trunk): it renders the menu client-side, by sending all menu notes to the browser and running some javascript to change it into html5.

Reverting this patch is NOT going to change anything re. privacy of the menu tree in openwrt-20.

As for filtering the menu tree "server-side" (in the router) before sending it to the java-script, that was my proposal for properly fixing the design behind this issue -- but I don't have the time nor the inclination to implement that, so if nobody steps up to do it, you cannot complain that the main LuCI devs don't care.

And there WILL be side-channels that will leak that information. Someone mentioned they wanted to hide that package foo was installed. You have to either disable, or redesign the i18n entirely, or it will leak the presence of translated strings for that package. And rpcd list. And...

NOTE: I am not one of the "main LuCI devs".

hmh added a commit to simetnicbr/simetbox-openwrt-feed that referenced this issue Jul 13, 2020
We can hit a very nasty OpenWRT-19.07.x+ issue with LuCI should we set
sysauth=false on a *page* (not a XHR node, etc).  Basically, the menu
diappears, and you cannot navigate anywhere.  Refer to
openwrt/luci#3563

It was somewhat fixed on LuCI master (for OpenWRT 20), but not in a way
that made people happy -- since it will always show the entire menu
tree regardless of auth state.  But OpenWRT 19.07 doesn't have this
change, it is too controrversial to request a backport, and we don't
want to fork LuCI if we can help it.

While it is really nice to have a decent status page that does not
require auth (something OpenWRT sorely lacks is user friendliness), we
will have to address that directly later.

While at it, remove other stray code that is currently unused (and has
been unused for a looong time).

Note that we do not have the "user_data" functionality active for
*years*, but remants of its code are still everywhere...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants