Skip to content

Improve race condition handling for slow reCAPTCHA load#11451

Merged
aduth merged 5 commits intomainfrom
aduth-recaptcha-load-delay
Nov 13, 2024
Merged

Improve race condition handling for slow reCAPTCHA load#11451
aduth merged 5 commits intomainfrom
aduth-recaptcha-load-delay

Conversation

@aduth
Copy link
Copy Markdown
Contributor

@aduth aduth commented Nov 5, 2024

🛠 Summary of changes

Updates CaptchaSubmitButton to better handle a scenario where reCAPTCHA is slow to load.

Currently, if reCAPTCHA is not loaded by the time the user submits the form, the form will submit immediately, which may produce a failing result from the backend if reCAPTCHA is being enforced.

This is rather unlikely to happen, since the initial reCAPTCHA script is small (roughly 2kb in size), but still technically possible, particularly if someone is using browser extensions or system features to autofill the sign-in form.

The proposed changes here take advantage of documented reCAPTCHA features to "queue" callbacks prior to reCAPTCHA being loaded†, with an additional fallback to submit the form if reCAPTCHA fails to load within a reasonable timeframe (currently 5 seconds).

†: While this feature is not explicitly documented in the reCAPTCHA Enterprise documentation, I've manually confirmed the presence of this queue behavior in the enterprise.js script.

📜 Testing Plan

Verify that reCAPTCHA validation is not impacted by these changes:

Configure reCAPTCHA for local development. You can use any credentials, they don't need to be valid.

# config/application.yml
development:
  sign_in_recaptcha_score_threshold: 0.3
  sign_in_recaptcha_percent_tested: 100
  recaptcha_site_key: 'test'
  recaptcha_enterprise_api_key: 'test'
  recaptcha_enterprise_project_id: 'test'
  recaptcha_mock_validator: false
  1. Go to http://localhost:3000
  2. Enter email and password
  3. Click "Sign in"
  4. Confirm that you are redirected to either MFA or directly to your account dashboard

Verify that the reCAPTCHA script failing to load doesn't prevent form submission:

You can simulate this by removing the reCAPTCHA script from CaptchaSubmitButtonComponent's markup:

diff --git a/app/components/captcha_submit_button_component.html.erb b/app/components/captcha_submit_button_component.html.erb
index 88e7033c4a..b6a7699b10 100644
--- a/app/components/captcha_submit_button_component.html.erb
+++ b/app/components/captcha_submit_button_component.html.erb
@@ -36,5 +36,2 @@
       ).with_content(content) %>
-  <% if recaptcha_script_src.present? %>
-    <%= content_tag(:script, '', src: recaptcha_script_src, async: true) %>
-  <% end %>
 <% end %>
  1. Go to http://localhost:3000
  2. Enter email and password
  3. Click "Sign in"
  4. After 5 seconds of the button showing as a spinner button, confirm that you are redirected to either MFA or directly to your account dashboard

@aduth aduth marked this pull request as ready for review November 5, 2024 14:53
@aduth aduth requested a review from a team November 5, 2024 14:53
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean we wait an additional 5 seconds? That seems like a very long time

Copy link
Copy Markdown
Contributor Author

@aduth aduth Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is meant to be a race: Either reCAPTCHA finishes loading in the next 5 seconds (and we cancel this timeout), or the form submits (without the reCAPTCHA token). From a user's perspective, the form will submit as soon as reCAPTCHA is finished its assessment.

We could also just remove this logic, but then it risks reCAPTCHA failing to load altogether and the submission appearing to hang indefinitely. They'll be unsuccessful if they submit without a reCAPTCHA token, but at least it gives them a path forward.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important to keep things moving, but my guess is that if the page is responsive enough for the user to submit, but the reCAPTCHA hasn't loaded yet, it probably won't? I would have added like a 1 second delay

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a few other optimizations in the pipeline to drive the load time down further that we could be more aggressive, but I'm hoping this is exceedingly unlikely as it is to not worry too much about it.

@aduth aduth force-pushed the aduth-recaptcha-load-delay branch from 7271f15 to 69fb875 Compare November 13, 2024 19:56
@aduth
Copy link
Copy Markdown
Contributor Author

aduth commented Nov 13, 2024

I did an extra test locally to double-check that it works as expected with a real-world scenario of reCAPTCHA being slow, but eventually loading:

Diff, for posterity:

diff --git a/app/components/captcha_submit_button_component.html.erb b/app/components/captcha_submit_button_component.html.erb
index 88e7033c4a..f8f10909da 100644
--- a/app/components/captcha_submit_button_component.html.erb
+++ b/app/components/captcha_submit_button_component.html.erb
@@ -36,4 +36,8 @@
       ).with_content(content) %>
-  <% if recaptcha_script_src.present? %>
-    <%= content_tag(:script, '', src: recaptcha_script_src, async: true) %>
+  <%= javascript_tag(nonce: true) do %>
+  setTimeout(() => {
+    const script = document.createElement('script');
+    script.src = "<%= recaptcha_script_src %>";
+    document.body.appendChild(script);
+  }, 3000);
   <% end %>

@aduth aduth merged commit 6203407 into main Nov 13, 2024
@aduth aduth deleted the aduth-recaptcha-load-delay branch November 13, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants