expose icon indicating copy to clipboard operation
expose copied to clipboard

Expose client authentication results into runtime error in some cases

Open FlyingDR opened this issue 3 years ago • 1 comments

There is a case when upgrade from Expose 1.x to Expose 2.x results into runtime error during client authentication process, effectively resulting into unusable service.

Issue is triggered by upgrading Expose 1.x server which had previously published configuration file (via expose publish) to Expose 2.x without re-publishing of the configuration file. It is especially likely because upgrade guide explicitly mentions:

Your existing authentication token and configuration file will still be valid after updating to the latest version.

which is not completely true, see below.

There are several components which causes issue to arise under certain conditions.

Issue itself is caused by generation of response to authenticate: https://github.com/beyondcode/expose/blob/97993318e737a422075444cfe8d180415ed43180/app/Server/Http/Controllers/ControlMessageController.php#L170-L179

As can be seen - value for message is generated by function which is taken from config(). This behavior is added by https://github.com/beyondcode/expose/commit/774265852710bb21ac14c13e73d88c2339393e7d and this commit adds function to admin.messages configuration: https://github.com/beyondcode/expose/blob/97993318e737a422075444cfe8d180415ed43180/config/expose.php#L340-L344

Having a function inside configuration is not quite expected case by itself. But it became especially strange when there is an array of strings and one of them turns to be required to be a function. Pretty strange decision which may be worth to reconsider.

Anyway, it is first piece of the puzzle. Second one is Expose 1.x config which, of course, have no such function at this place in configuration. It is being loaded and merged here: https://github.com/beyondcode/expose/blob/fb05f23124443d32352e59ba46465209f343bd94/app/Providers/AppServiceProvider.php#L49-L67 but, as you can see, simple array_merge() is used which does not perform deep merge. Since new function is located in deeper level of the configuration - it, of course, get silently replaced by information from old configuration file.

It was especially hard to find a reason because issue seem to be silently swallowed, and not reported anywhere, even in PHP errors log.

Visual behavior is very similar to one, described in #264, but actually there is an error: Value of type null is not callable in ControlMessageController.php:173

I would propose to reconsider adding functions in configuration files and to add is_callable() check before trying to run the code from the configuration. It may also be worth to mention requirement for configuration upgrade into upgrade guide and to implement proper configuration merging.

FlyingDR avatar Aug 18 '21 18:08 FlyingDR

For those who may be affected by this issue: there is two simple workarounds for it:

  1. If you have nothing special into your Expose configuration - either remove ~/.expose/config.php and run expose publish or just run expose publish --force
  2. If you have some useful options in your configuration file - copy these lines and paste them into your configuration file.

Don't forget to restart Expose server afterwards.

FlyingDR avatar Aug 18 '21 19:08 FlyingDR

Closing this issue because it's old. Please feel free to open a new one if it's still relevant.

sschlein avatar Dec 21 '23 13:12 sschlein