opnsense / core

OPNsense GUI, API and systems backend
https://opnsense.org/
BSD 2-Clause "Simplified" License
3.36k stars 753 forks source link

Pressing Cancel in Static DHCPv6 Mapping shouldn't return to the DHCPv6 config page #4406

Closed wget closed 3 years ago

wget commented 4 years ago

Describe the bug

To Reproduce Steps to reproduce the behavior:

  1. Ensure the checkbox Allow manual adjustment of DHCPv6 and Router Advertisements is unchecked.
  2. Go to the DHCPv6 lease page (/status_dhcpv6_leases.php)
  3. Click on the '+' Screenshot_20201010_211637
  4. Press the Cancel button Screenshot_20201010_212050
  5. You are being redirected to the DHCPv6 configuration page (/services_dhcpv6.php?if=lan) (notice the absence of title to this page) Screenshot_20201010_212149

Expected behavior

We shouldn't be allowed.

Screenshots

See above.

Relevant log files

N/A

Additional context

I led an additional test:

It looks like the config is properly reset when the aforementioned checkbox is unchecked. So this bug is bug is not /that/ bad as this configuration reset process seems not impacted :)

Environment

Software:

Hardware:

fichtner commented 3 years ago

@wget thanks! so we are avoiding to add the add/delete button now if these things shouldn't be manually configured according to the (most likely) correct menu logic, see 4dbc220490. If you want to try you can do so via:

# opnsense-patch 4dbc220490

Cheers, Franco

wget commented 3 years ago

@fichtner Will definitely check! Thanks :)

wget commented 3 years ago

@fichtner I tried the logic on 21.1 and unfortunately we still can bypass it.

When disabling the Allow manual adjustment of DHCPv6 and Router Advertisements option, reaching a page like:

/services_dhcpv6_edit.php?if=lan&duid=00:03:00:01:6c:c2:17:9b:4e:e0&hostname=

should NOT be allowed. The end user should be redirected to the main (dashboard page).

This actually happens for all the other pages as well. If we are being denied, we should be prevented accessing and be redirected.

fichtner commented 3 years ago

Right, it should be looking for the parent (likely "wan"), but it is looking for "lan"

fichtner commented 3 years ago

No wait that was silly. I'll try to fix it this morning in any case :)

fichtner commented 3 years ago

Did you try the page link manually? The add button should already disappear, no?

wget commented 3 years ago

@fichtner Yes the + button link has already disappeared following your patch :)

And yes, I reached the page manually. This is quite common when you use your browser history a lot. I often see SMB companies documenting things using direct links to the WebUI 😱

What we really want is a patch to avoid configuring things we shouldn't have access to. 🤗

fichtner commented 3 years ago

Ah ok. Now I have all the context :) The question is where would we want to redirect? It's already some sort of parent page. We could move to the dashboard but someone else might interpret it as a bug, too. What do you think?

wget commented 3 years ago

@fichtner In all CMS I have been using like Magento, WordPress, etc., they have implemented this as two solutions: report an in context 404 or a redirection to the dashboard. So maybe this page would be the more common choice and be less perceived as a bug? :) ⤵️

Screenshot_20210225_183311