-
Notifications
You must be signed in to change notification settings - Fork 28
feat(pir): captcha providers #1550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for content-scope-scripts ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Temporary Branch UpdateThe temporary branch has been updated with the latest changes. Below are the details:
Please use the above install command to update to the latest version. |
[Beta] Generated file diffTime updated: Wed, 19 Mar 2025 16:17:54 GMT Android
File has changed Integration
File has changed Windows
File has changed Apple
File has changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @madblex for your first contribution!
I like the pattern you're thinking of here, I just noted some small things related to matching the existing codebase's patterns around type-safety etc. Here's a small summary:
-
- try to design richer return types to avoid needing to use exceptions as control flow
-
- favour
implements
overextends
to avoid class inheritance (this is awkward in JSDoc, but I gave an example in the comments)
- favour
-
- remove 'index' files if they only serve to enable re-exports. In this codebase it's preferred to be explicit about import/exports.
-
- prefer
function foo() {
overconst foo = () => {
in module-level functions
- prefer
In terms of number 2, I made this change on a branch just to verify the types would still work correctly, you can take a look here bf391d2
injected/src/features/broker-protection/captcha-services/providers/cloudflare-turnstile.js
Outdated
Show resolved
Hide resolved
injected/src/features/broker-protection/captcha-services/providers/hcaptcha.js
Outdated
Show resolved
Hide resolved
injected/src/features/broker-protection/captcha-services/providers/hcaptcha.js
Outdated
Show resolved
Hide resolved
injected/src/features/broker-protection/captcha-services/providers/index.js
Outdated
Show resolved
Hide resolved
injected/src/features/broker-protection/captcha-services/providers/recaptcha-enterprise.js
Outdated
Show resolved
Hide resolved
injected/src/features/broker-protection/captcha-services/utils/index.js
Outdated
Show resolved
Hide resolved
injected/src/features/broker-protection/captcha-services/captcha.service.js
Outdated
Show resolved
Hide resolved
eed4ba4
to
87d9841
Compare
injected/src/features/broker-protection/captcha-services/captcha.service.js
Outdated
Show resolved
Hide resolved
327c2ea
to
18e3a6c
Compare
injected/src/features/broker-protection/captcha-services/captcha.service.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madblex This looks great to me. I left some comments and did some testing (see below) but nothing blocking from me.
I tested this PR directly in XCode in the following scenarios:
- Using existing Spokeo JSON file (e.g. no
captchaType
ingetCaptchaInfo
) - ✅ Succeeded as expected - Using an invalid
captchaType
in Spokeo's JSON (captchaType: 'bleh'
) - ❌ Failed as expected - Using an explicit
captchaType
in Spokeo's JSON (captchaType: 'recaptcha2'
) - ✅ Succeded as expected - Using a real but mismatched captchaType in Spokeo's JSON (
captchaType: 'cloudflareTurnstile'
) - ✅ Fell back to recaptcha2 and succeeded.
Very excited to see us in a better place captcha-wise. Great job.
injected/src/features/broker-protection/captcha-services/captcha.service.js
Outdated
Show resolved
Hide resolved
injected/src/features/broker-protection/captcha-services/get-captcha-provider.js
Outdated
Show resolved
Hide resolved
injected/src/features/broker-protection/captcha-services/providers/hcaptcha.js
Show resolved
Hide resolved
injected/src/features/broker-protection/captcha-services/providers/registry.js
Outdated
Show resolved
Hide resolved
18e3a6c
to
bae28ee
Compare
injected/src/features/broker-protection/captcha-services/get-captcha-container.js
Show resolved
Hide resolved
Thanks Brian, and thanks for testing as well! Did a very similar test matrix and it all worked well with the new changes. Ready for another round of reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These updated changes look great to me.
injected/src/features/broker-protection/captcha-services/providers/provider.interface.js
Outdated
Show resolved
Hide resolved
100438f
to
94ce4f6
Compare
- replace arrow fn with regular functions - update provider registry - align getSupportingCodeToInject return types
- wire up the remote config setting - replace querying by root with querying by captcha container - remove hcaptcha and cloudflare providers
* feat(pir): captcha providers tests * feat: address PR comments * feat: amend tests * fix: add new tests and unroll for loops * Minor tweaks to wording and removal of hCaptcha mocks * fix: lint --------- Co-authored-by: Brian Hall <[email protected]>
94ce4f6
to
304e4db
Compare
408e7f3
to
12b2e90
Compare
Asana Task/Github Issue: https://app.asana.com/0/0/1209555302496203
Description
This PR:
navigate
action, already augmented with the newinjectCaptchaHandler
property (see associated tech design)resolveActionHandlers
resolver for the action handlers that uses a remote config to determine which version is usedTODO:
resolveActionHandlers
Testing Steps
cd injected
npm run playwright
should passChecklist
Please tick all that apply: