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
left a comment
There was a problem hiding this comment.
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
|
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 Covers Let me know if you want anything tightened. |
38197a2 to
22b83aa
Compare
|
Good call - switched to the launch-time warning approach you suggested and dropped the monkey-patching entirely. When geoip=True but no proxy is passed to launch, it now warns (your exact message) in launch()/launch_async() on the Python side and in buildLaunchOptions() on the JS side, so it covers every path including new_page. Kept the README limitation note. Rebased onto main. Verified: the new warning tests pass (fires when geoip + no proxy, stays quiet when a launch proxy is set, both sync and async). I couldn't run the JS Vitest locally (no js/node_modules here), but the JS change is the same guarded console.warn. |
Replaces the per-context monkey-patching approach with a launch-time warning: when geoip=True but no proxy is passed to launch(), timezone/locale silently default to UTC/en-US. Warning fires in launch()/launch_async() (Python) and buildLaunchOptions() (JS), so it covers every code path including new_page(). Keeps the README note documenting the per-context-proxy limitation.
22b83aa to
aa3315d
Compare
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