Regression #13757
closed
Circular dependency issue in ``auth.inc``/``authgui.inc``
100%
Description
Some parts of auth.inc use a check for a function before doing some GUI-specific checks:
if (function_exists("display_error_form")) {
This function is defined in authgui.inc, but authgui.inc includes auth.inc before this function is defined.
I don't see how this could be working correctly unless something happened to include auth.inc twice.
The checks here used to work but perhaps something is behaving more appropriately in PHP 8.x which makes it (correctly) fail.
Updated by Jim Pingle over 1 year ago
Looks like this may have broken in 746f30e3ce1ff39c226a73bf87c86dd370ef239c with the added includes changing the order in which things were initially loaded.
Moving the authgui.inc include up to the top of guiconfig.inc fixes the behavior, but it's still awkward.
Updated by Jim Pingle over 1 year ago
- Status changed from New to Feedback
- % Done changed from 0 to 100
Applied in changeset 2a24c162e0a8e69d176c54b5a7be09b23cb233f8.
Updated by Jim Pingle over 1 year ago
- Status changed from Feedback to In Progress
- % Done changed from 100 to 0
That fix attempt ended up not incomplete, it could break CSRF in certain cases.
Still experimenting and checking into alternative solutions.
Updated by Jim Pingle over 1 year ago
Draft MR: https://gitlab.netgate.com/pfSense/pfSense/-/merge_requests/984
At the moment the least disruptive way to correct the behavior is to hardcode the CSRF timeout so it is not read from the configuration, which allows us to reorder includes so that authgui.inc can load auth.inc first.
I'm still looking into alternate solution that will allow reading in the CSRF timeout value from the configuration but it's a tricky chicken-and-egg scenario it ends up in because loading config.inc loads in numerous other includes, but we can't load authgui before initializing CSRF because it can output things that need CSRF protection.
Updated by Jim Pingle over 1 year ago
- Status changed from In Progress to Feedback
- % Done changed from 0 to 100
Applied in changeset db6dd2d2d288fdd64b9e741db0900c5eb15ba9fb.
Updated by Jim Pingle over 1 year ago
- Status changed from Feedback to Resolved
Closing this for now as it appears to be working as expected given the current limitations for the moment.
I can trigger the DNS Rebind and/or HTTP_REFERER error by using an unknown hostname as expected depending on the context.
Updated by Jim Pingle over 1 year ago
- Private changed from Yes to No
- Release Notes changed from Default to Force Exclusion