-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: improve Cloudflare challenge handling with clickPositionCallback and configurable waitPeriod #3247
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
feat: improve Cloudflare challenge handling with clickPositionCallback and configurable waitPeriod #3247
Conversation
| } | ||
|
|
||
| if (options.clickPositionCallback) { | ||
| let pos = await options.clickPositionCallback(page); |
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.
| let pos = await options.clickPositionCallback(page); | |
| const pos = await options.clickPositionCallback(page); |
| /** Allows overriding how the checkbox click position is calculated. */ | ||
| clickPositionCallback?: (page: Page) => Promise<{ x: number; y: number } | null>; | ||
| /** Optional delay (in milliseconds) before the first click attempt on the challenge checkbox. */ | ||
| waitPeriod?: number; |
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.
| waitPeriod?: number; | |
| waitPeriodMillis?: number; |
or maybe we should use seconds here, it feels weird to have one time based option in millis and the other in seconds. also, since we wait for one second on each iteration, what about making that time configurable instead of introducing another sleep option in the same loop?
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.
Yes, I will change waitPeriod from milliseconds to seconds.
Also, regarding the initial delay, if we apply the delay before verifying the challenge page, then even non challenge requests will wait unnecessarily. So it would be better to apply the extra delay after we detect the challenge (i.e., only when required).
If you prefer that we still keep a configurable delay before the first click regardless of detection, I can update the code accordingly, just let me know.
Also as per the issue description and after studying the issue #3124 it seems the delay is required only for the first iteration to allow the challenge UI to finish rendering, can you please confirm it.
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.
The idea is that the 1s delay might not be enough, so we try it 10 times in a loop and escape early once we see we can. That's why I said that one sleep might be enough, since it happens again and again till we pass the challenge.
So it would be better to apply the extra delay after we detect the challenge (i.e., only when required).
You need to wait to be able to verify if it is the challenge page, I dont think there is a way to make this pass immediately for non challenge pages.
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.
Got it, thanks for the clarification.
My initial understanding was that we would wait 1 second to detect whether it’s a challenge page, and then after confirming the challenge, we would wait an additional options.waitPeriod seconds to allow the button position shift to occur before detection.
But understood now that the initial wait itself will be configured and is applied in each iteration, so adding a second delay isn’t necessary. The repeated loop sleeps already give enough time for both detection and passing the challenge.
| /** Allows overriding how the checkbox click position is calculated. */ | ||
| clickPositionCallback?: (page: Page) => Promise<{ x: number; y: number } | null>; | ||
| /** Optional delay (in seconds) before the first click attempt on the challenge checkbox. */ | ||
| waitPeriodSecs?: number; |
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.
This should have a more descriptive name, having sleepSecs and waitPeriodSecs next to each other is quite confusing. Also the comment should mention the default of 1s.
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.
changed the name to initialWaitSecs
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.
i'd prefer sleep over wait, it means the same and we already use sleep in those options. also not entirely sure if this should be called initial since it happens in each iteration
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.
ok, preChallengeSleepSecs might be a good name in this case.
This PR enhances the Cloudflare challenge handling logic by making the click behaviour more flexible and reliable.
Two new options are introduced in postNavigationHooks.ts:
clickPositionCallbackAllows users to manually specify the exact coordinates (x, y) where the click should occur.
This is useful when the default DOM-based position detection fails, or when the challenge layout varies across sites.
preChallengeSleepSecsOptional delay (in milliseconds) inserted before the first click attempt.
This helps ensure the Cloudflare button and related elements are fully rendered before detection or clicking begins.
Closes #3127