Skip to content

Sets Gon variables in response to requests with invalid reCAPTCHA

Bogdan Denkovych requested to merge set-gon_variables-after-requiest-with-invalid-reCAPTCHA into master

What does this MR do and why?

before_action :add_gon_variables, if: :html_request? callback sets Gon variables.

prepend_before_action :check_recaptcha in

check whether reCAPTCHA was verified.

prepend_before_action :check_recaptcha callback is executed before before_action :add_gon_variables.

When a user submits requests with invalid reCAPTCHA, check_recaptcha builds HTML page in response by render action: 'new'. This page is rendered without Gon variables since add_gon_variables is not executed before check_recaptcha. This leads to !85127 (merged) and !87857 (closed) issues.

We decided to use redirect_to action: :new instead of render action: 'new'. A drawback of this approach is that it leads to 1 extra request to GitLab server. To eliminate such requests this MR suggest to execute add_gon_variables in check_recaptcha before rendering HTML page that requires that. Also, this MR reverts !85127 (merged).

I considered the following fix:

- prepend_before_action :check_recaptcha
+ before_action :check_recaptcha

but turns out that in https://gitlab.com/gitlab-org/gitlab/-/blob/a434a913378c7a715866a32eed81622ab049dccc/app/controllers/sessions_controller.rb#L24 it is important to execute check_recaptcha at the begginning of callback chain to prevent a user be authenticated with invalid reCAPTCHA.

Screenshots or screen recordings

These are strongly recommended to assist reviewers and reduce the time to merge your change.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Bogdan Denkovych

Merge request reports