-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(geoip): warn when proxy is set per-context (geoip resolves at launch only)#229
fix(geoip): warn when proxy is set per-context (geoip resolves at launch only) #229mvanhorn wants to merge 1 commit into
Conversation
@Cloak-HQ
Cloak-HQ
left a comment
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 for the PR and for documenting this limitation clearly.
The monkey-patching approach adds more complexity than needed for a warning. Two new functions (sync + async), duplicated _has_context_proxy helpers, and it still misses browser.new_page(proxy=...) which bypasses the patched new_context internally.
Simpler alternative: warn at launch time when geoip=True but no proxy is passed to launch. That's the actual misconfiguration moment from #160.
# In launch() / launch_async(), after maybe_resolve_geoip: if geoip and not proxy: logger.warning( "geoip=True without proxy= at launch — timezone/locale will default to " "UTC/en-US. Per-context proxies don't trigger geoip resolution. Pass " "proxy= to launch() or set explicit timezone=/locale=." )
Same for JS. No new functions, no monkey-patching, covers all code paths including new_page. The README/docs additions are good as-is.
There's an edge case this doesn't cover: launch(proxy=A, geoip=True) then new_context(proxy=B) where B is a different region. But that's already documented by your README addition, so users have the info they need.
Per maintainer review on CloakHQ#229: replace the monkey-patching approach with a launch-time warning. The misconfiguration moment for CloakHQ#160 is launch() called with geoip=True but no top-level proxy. Per-context proxies set via browser.new_context(proxy=...) don't trigger geoip resolution. Add the warning inline in every public launch entry point (launch, launch_async, launch_persistent_context, launch_persistent_context_async, launch_context, launch_context_async). The higher-level wrappers (launch_context*) resolve geoip eagerly and call the lower-level launch() without forwarding geoip=, so no double-warning fires. Same pattern in js/src/playwright.ts across buildLaunchOptions, launchContext, and launchPersistentContext. Add unit tests (pytest caplog + vitest console.warn spy) covering the fires-when-no-proxy and silent-when-proxy-set cases. README + js/README get a short "geoip warnings" subsection.
9b34944 to
38197a2
Compare
mvanhorn
commented
May 21, 2026
Thanks - rewrote per the suggestion. Pushed 38197a2.
The branch was force-pushed: I reset to current main, dropped the monkey-patching, and reimplemented as a launch-time if geoip and not proxy: warn(...) inline in each public entry point (launch, launch_async, launch_persistent_context*, launch_context*). The launch_context* wrappers resolve geoip eagerly and call the lower-level launch() without forwarding geoip=, so the warning fires exactly once per user-facing call - no double-warning bug. Same pattern in js/src/playwright.ts across buildLaunchOptions, launchContext, and launchPersistentContext.
Covers browser.new_page(proxy=...) and any other post-launch context routing because the warning fires at launch regardless of what happens after. Tests in tests/test_geoip_launch_warning.py (pytest caplog) and js/tests/launch.test.ts (vitest console.warn spy) cover the fires-when-no-proxy and silent-when-proxy-set cases. README + js/README.md get short geoip-warnings subsections.
Let me know if you want anything tightened.
Summary
When
geoip=Trueis set onlaunch_async()but a laternew_context()gets its ownproxy=, the per-context proxy doesn't trigger the geoip resolution path —timezoneandlocalestay set to whatever the launch resolved. This now warns once with a pointer at the fix shape, instead of silently looking detectable.The warning is conservative: it doesn't promise behavior the binary patches don't deliver. The deeper "per-context timezone/locale spoofing" change is binary-side and out of scope for this PR.
What changed
cloakbrowser/browser.py:44— patches sync and asyncBrowser.new_context()to detect the misconfiguration (geoip enabled at Browser level + proxy passed per-context) and emit a one-timewarnings.warn(..., UserWarning).js/src/playwright.ts:47— matching JSbrowser.newContext()warning.README.mdandjs/README.md— "GeoIP + multiple contexts" subsection documenting the limitation and how to avoid it.tests/test_context_geoip_warning.py+js/tests/launch.test.ts— warn-once, quiet-path (proxy on launch), quiet-path (no geoip), quiet-path (no secret).Notes for #160
The thread accepted the maintainer's diagnosis but raised concern that something else might be off. This PR ships the diagnosis as a warning + docs so users hit a clear signal. If the deeper investigation turns up a per-context resolution bug beyond geoip, that's a separate change.
Testing
pytest tests/test_context_geoip_warning.py tests/test_launch_context.py tests/test_proxy.py— 79/79 passedpython3 -m py_compile cloakbrowser/browser.py tests/test_context_geoip_warning.py— cleannpm ciblocked by sandbox network (ENOTFOUND registry.npmjs.org); JS test follows the existing shape, CI will exercise.Refs #160