ModelAdmin executes init code even when there is no authenticated user

lekoala commented 2 months ago

Module version(s) affected

5.x

Description

I recently discovered this unexpected error in my logs...

RuntimeException
ModelAdmin::init(): Invalid Model class lib

for the following url: /admin/events/lib/external/responsive_filemanager/filemanager/dialog.php

That's expected, "lib" is not a valid model class. But what's really odd, is that this is the case for anonymous users... meaning any bot can basically hammer your website and create tons of errors logs due to this.

The issue is that the auth check does not interrupt the init() process in subclasses (there is simply a return statement in the init parent class)

How to reproduce

Visit any /admin/security|modeladmin_segment/invalid_modal/xxx url on a ss website and get a server error

eg: https://some.domain.com/admin/security/lib/external/responsive_filemanager/filemanager/dialog.php

Possible Solution

Always add a redirectedTo check in any ModelAdmin subclasses... (not great, because you have to think about it)

<?php
    protected function init()
    {
        parent::init();

        if ($this->redirectedTo()) {
            return;
        }
?>

Better long term solution:

Throw a RedirectException (this does not exist, but I think it really should be added to the core) in the init method to avoid any further processing instead of what's currently in place. This would make the whole thing much simpler and avoid issues for unsuspecting developers.

Additional Context

No response

Validations

[X] Check that there isn't already an issue that reports the same bug
[X] Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

https://github.com/silverstripe/silverstripe-admin/pull/1830

GuySartorelli commented 2 months ago

A RedirectException (similar to the existing ValidationException) sounds like a good long-term solution that could be introduced in CMS 6. Would you be interested in implementing that?

I'll review the CMS 5 bugfix PR in the meantime.

lekoala commented 2 months ago

A RedirectException (similar to the existing ValidationException) sounds like a good long-term solution that could be introduced in CMS 6. Would you be interested in implementing that?

I don't have much time at the moment :-) I can easily draft up some basics, but if it needs to be a proper PR (ie : adding tests, documentation etc etc) then it's clearly to time consuming

GuySartorelli commented 2 months ago

No worries then, a PR that wouldn't be ready to be merged (with tests etc) probably isn't worth doing. It's a good idea though, maybe someone will pick it up eventually. In the meantime the PR you've done for CMS 5 resolves this specific bug so that's a win!

lekoala commented 1 month ago

@GuySartorelli I had a quick look for this redirectException thing and actually I think all that is needed is to do something similar to this

https://github.com/silverstripe/silverstripe-framework/blob/b02ac10fc8495e429d017664ee8ed5efba38357f/src/Control/RequestHandler.php#L193-L205

but also inside the init method

https://github.com/silverstripe/silverstripe-framework/blob/b02ac10fc8495e429d017664ee8ed5efba38357f/src/Control/Controller.php#L121

that could become something like

    try {
          $this->init();
      } catch (HTTPResponse_Exception $e) {
          $this->setResponse($e->getResponse());
      } catch (PermissionFailureException $e) {
           $this->setResponse(Security::permissionFailure(null, $e->getMessage()));
      }

This way, any exception PermissionFailureException or HTTPResponse_Exception thrown during init would behave just like a beforeHandleRequest redirect And it would be really easy also to create a RedirectException that extends HTTPResponse_Exception and overrides the getResponse() to provide the redirect response

I didn't try that yet but i don't see why it wouldn't work

that or add the try catch block not in handleRequest, but inside whatever calls the handleRequest method which would probably be even better

GuySartorelli commented 1 month ago

The call to init() essentially originates from the call to beforeHandleRequest() in Controller::handleRequest() I think, so I'd put the second try/catch around that.

But yup that sounds like a good solution to me.

that or add the try catch block not in handleRequest, but inside whatever calls the handleRequest method which would probably be even better

I'd probably put it in handleRequest() for now, given that's what's catching the other various response exceptions.

GuySartorelli commented 1 month ago

PR for CMS 5 merged. @lekoala if you want to make a PR for the CMS 6 exception, feel free to use this issue for it - otherwise close the issue as done.

lekoala commented 1 month ago

will close for now

silverstripe / silverstripe-admin