Relying on the request in the constructor of RegisterForm prevents proper access checking outside of the expected route context

[FeyP]

Problem/Motivation

In the constructor of Drupal\civiremote_event\Form\RegisterForm the form tries to get the route parameters from the current request. This prevents proper access checking outside the context of the route, if the access checks require an initialization of the RegisterForm, which is currently the case due to the custom access callbacks on the form object. Throwing 404 or other exceptions on error there may also result in the 404 or other error leaking back to the request that tried to initialize the form object, e.g. you might see a 404 error just for rendering a link to the registration route on an overview page, if the link is access checked as part of the rendering process.

Steps to reproduce

Try something like this somewhere outside of the expected route request context:

$url = \Drupal\Core\Url::fromRoute('civiremote_event.register_form', [
  'event' => $crm_id,
  'profile' => NULL,
]);
$url->access();

Proposed resolution

As a quick fix I moved the access checks of RegisterForm and RegistrationUpdateForm to RegisterFormController and used the parameters passed in instead of the properties.

A better fix would be to refactor the form code so that relying on the request in the constructor is no longer needed.

See also #9 , which is related.

[jensschuppe]

Thanks for raising this issue.

As a quick fix I moved the access checks of RegisterForm and RegistrationUpdateForm to RegisterFormController and used the parameters passed in instead of the properties.

Would you be able to provide that fix/workaround as a PR?

A better fix would be to refactor the form code so that relying on the request in the constructor is no longer needed.

Agreed, do you plan working on that?

[FeyP]

Would you be able to provide that fix/workaround as a PR?

Yes, although I didn't do much testing yet. I'll prepare a PR, but maybe let it sit for at least a few days in case something comes up.

Agreed, do you plan working on that?

I could certainly do that, but I'd need to check with my project management. I'm not sure we have budget left for this, so for now I'd say we don't plan on doing that, unfortunately. I'll let you know, if that will change.

[jensschuppe]

#19 is another approach and should cover errors during out-of-context access checks.