LG-14216: Assign reCAPTCHA A/B test bucket for missing user#11210
Merged
Conversation
changelog: Upcoming Features, Fraud Prevention, Assign reCAPTCHA A/B test bucket for missing user
aduth
commented
Sep 6, 2024
| user.uuid | ||
| else | ||
| user&.uuid | ||
| SecureRandom.gen_random(1) |
Contributor
Author
There was a problem hiding this comment.
Thought process with the selection of method here:
- We do want this to be resilient against timing attacks (
SecureRandom) - We don't care much about the actual value.
SecureRandom.hexdoes a lot more work than we need
Benchmarks, since I was curious:
require 'benchmark/ips'
require 'securerandom'
Benchmark.ips do |x|
x.report('gen_random') do
SecureRandom.gen_random(1)
end
x.report('hex') do
SecureRandom.hex
end
x.compare!
endruby 3.1.4p223 (2023-03-30 revision 957bb7cb81) [arm64-darwin21]
Warming up --------------------------------------
gen_random 486.807k i/100ms
hex 169.039k i/100ms
Calculating -------------------------------------
gen_random 4.867M (± 0.9%) i/s - 24.340M in 5.001117s
hex 1.679M (± 1.9%) i/s - 8.452M in 5.035533s
Comparison:
gen_random: 4867370.6 i/s
hex: 1679161.7 i/s - 2.90x slower
Contributor
There was a problem hiding this comment.
this is not exactly a fair test, they'll pulling different numbers of bytes from random? we can close the gap a bit by using SecureRandom.hex(1)
irb(local):005:0> require 'benchmark/ips'
irb(local):006:0> require 'securerandom'
irb(local):007:0>
irb(local):008:1* Benchmark.ips do |x|
irb(local):009:2* x.report('gen_random') do
irb(local):010:2* SecureRandom.gen_random(1)
irb(local):011:1* end
irb(local):012:1*
irb(local):013:2* x.report('hex') do
irb(local):014:2* SecureRandom.hex(1)
irb(local):015:1* end
irb(local):016:1*
irb(local):017:1* x.compare!
irb(local):018:0> end
irb(local):019:0>
Warming up --------------------------------------
gen_random 692.696k i/100ms
hex 415.078k i/100ms
Calculating -------------------------------------
gen_random 6.852M (± 1.7%) i/s - 34.635M in 5.055886s
hex 4.109M (± 3.7%) i/s - 20.754M in 5.059781s
Comparison:
gen_random: 6852411.5 i/s
hex: 4109274.2 i/s - 1.67x slower
Contributor
Author
There was a problem hiding this comment.
That's fair. I think I opted to use it without arguments since the expected pushback I expected for going for something like gen_random(1) is that it's very common and "simple" for us to use SecureRandom.hex (unparameterized) elsewhere in code.
zachmargolis
approved these changes
Sep 6, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎫 Ticket
Addendum for LG-14216 (implemented in #11148)
🛠 Summary of changes
Updates the discriminator evaluation for reCAPTCHA at sign-in to randomly assign requests which aren't associated with a user. This provides some resiliency against user existence probing, though there are other mitigations which already provide protection (ping me on Slack if interested).
This test is not currently live in production.
This would likely be very incompatible with #11202 since it would result in persisting records for many random values (although only within the domain of 1 bytes worth of random data), so I'd plan to avoid opting-in reCAPTCHA for persistence if moving forward with these changes.
📜 Testing Plan
Verify that reCAPTCHA is performed based on percent tested when trying to authenticate with a user that doesn't exist:
config/application.yml:abc@example.com, assigning a failing reCAPTCHA score (e.g.0.1)