feat(connection): add secure WebSocket (WSS) support with configurable connection host#434
Open
IMXEren wants to merge 2 commits into
Open
feat(connection): add secure WebSocket (WSS) support with configurable connection host#434IMXEren wants to merge 2 commits into
IMXEren wants to merge 2 commits into
Conversation
…e connection host Refactor connection address resolution to support `connection_host`, `use_secure`, and typed resolver parameters throughout the browser, tab, and connection stack.
📝 WalkthroughWalkthroughChangesThe CDP connection flow now supports configurable hosts and secure WebSocket schemes. Browser, tab, and OOPIF handlers propagate these settings, while address resolution supports explicit schemes, default ports, IPv6 hosts, and typed resolver parameters. CDP connection configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant ConnectionHandler
participant Resolver
participant CDPServer
Browser->>ConnectionHandler: configure host, port, and secure scheme
ConnectionHandler->>Resolver: request browser WebSocket address
Resolver->>CDPServer: GET /json/version
CDPServer-->>Resolver: return WebSocket debugger URL
Resolver-->>ConnectionHandler: return resolved address
ConnectionHandler-->>Browser: establish CDP connection
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pydoll/browser/chromium/base.py`:
- Around line 1095-1097: Update start() to construct the initial Tab using the
propagated connection_host, connection_port, and use_secure kwargs, matching the
configuration passed to later tabs. Preserve the existing configured endpoint
behavior for subsequent tab creation.
In `@pydoll/connection/types.py`:
- Around line 20-22: Update the resolver contract fields in the connection types
definition so host is required and non-optional, using a string type while
preserving the existing required semantics. Keep port and use_secure unchanged,
matching ConnectionHandler’s localhost normalization invariant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7e2584c-8ae6-4a2d-9491-4ad92d27c44b
📒 Files selected for processing (14)
pydoll/browser/chromium/base.pypydoll/browser/chromium/chrome.pypydoll/browser/chromium/edge.pypydoll/browser/tab.pypydoll/connection/connection_handler.pypydoll/connection/types.pypydoll/interactions/iframe.pypydoll/utils/general.pytests/integration/test_connection_handler.pytests/unit/conftest.pytests/unit/test_browser_commands.pytests/unit/test_browser_wiring.pytests/unit/test_connection_handler_unit.pytests/unit/test_utils.py
Comment on lines
+1095
to
+1097
| kwargs['connection_host'] = self._connection_host | ||
| kwargs['connection_port'] = self._connection_port | ||
| kwargs['use_secure'] = self._use_secure |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use these propagated kwargs for the initial tab as well.
start() still constructs its initial Tab with only connection_port at Line 218. With a custom host or use_secure=True, that tab falls back to localhost/ws, while later tabs use the configured endpoint.
Proposed fix
- tab = Tab(self, target_id=valid_tab_id, connection_port=self._connection_port)
+ tab = Tab(self, **self._get_tab_kwargs(valid_tab_id))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pydoll/browser/chromium/base.py` around lines 1095 - 1097, Update start() to
construct the initial Tab using the propagated connection_host, connection_port,
and use_secure kwargs, matching the configuration passed to later tabs. Preserve
the existing configured endpoint behavior for subsequent tab creation.
Comment on lines
+20
to
+22
| host: Required[Optional[str]] | ||
| port: Required[Optional[int]] | ||
| use_secure: Required[bool] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Make host non-optional in the resolver contract.
get_browser_ws_address interpolates this value directly, so a type-valid host=None produces http(s)://None:.... ConnectionHandler already normalizes missing hosts to localhost; reflect that invariant here.
Proposed fix
- host: Required[Optional[str]]
+ host: Required[str]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| host: Required[Optional[str]] | |
| port: Required[Optional[int]] | |
| use_secure: Required[bool] | |
| host: Required[str] | |
| port: Required[Optional[int]] | |
| use_secure: Required[bool] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pydoll/connection/types.py` around lines 20 - 22, Update the resolver
contract fields in the connection types definition so host is required and
non-optional, using a string type while preserving the existing required
semantics. Keep port and use_secure unchanged, matching ConnectionHandler’s
localhost normalization invariant.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/en/deep-dive/fundamentals/connection-layer.md`:
- Around line 140-146: Align the documented connection contracts with the
implementation: in docs/en/deep-dive/fundamentals/connection-layer.md lines
140-146, mark connection_port and page_id as optional, use None for
browser-level page connections, and document ws_address precedence; in lines
159-161, change _connection_port to Optional[int]; in
docs/en/deep-dive/architecture/browser-domain.md lines 239-245, change
connection_host to Optional[str]; and in
docs/en/deep-dive/architecture/tab-domain.md lines 101-103, use str for the host
and bool for secure transport.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a9a6525-f314-4db5-babf-5a2be06f89e8
📒 Files selected for processing (5)
docs/en/deep-dive/architecture/browser-domain.mddocs/en/deep-dive/architecture/find-elements-mixin.mddocs/en/deep-dive/architecture/tab-domain.mddocs/en/deep-dive/fundamentals/connection-layer.mddocs/en/deep-dive/fundamentals/iframes-and-contexts.md
Comment on lines
+140
to
+146
| | `connection_host` | `str` | Host where the browser's CDP endpoint is listening | | ||
| | `connection_port` | `int` | Port number where the browser's CDP endpoint is listening | | ||
| | `page_id` | `str` | Identifier for the specific page/target (use 'browser' for browser-level connections) | | ||
| | `ws_address_resolver` | `Callable` | Function to resolve the WebSocket URL from the port number | | ||
| | `ws_address_resolver` | `Callable` | Function to resolve the WebSocket URL from the `WSAddressResolverParams` | | ||
| | `ws_connector` | `Callable` | Function to establish the WebSocket connection | | ||
| | `ws_address` | `str` | WebSocket address of browser's CDP endpoint for use with remote connections | | ||
| | `use_secure` | `bool` | Whether to establish a secure connection to the browser's CDP endpoint | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align all documented connection types and semantics with the implementation.
The new documentation currently mixes incorrect required types with the actual optional/string/boolean contract.
docs/en/deep-dive/fundamentals/connection-layer.md#L140-L146: document optionalconnection_port/page_id, useNonefor browser-level connections, and mentionws_addressprecedence.docs/en/deep-dive/fundamentals/connection-layer.md#L159-L161: useOptional[int]for_connection_port.docs/en/deep-dive/architecture/browser-domain.md#L239-L245: changeconnection_hosttoOptional[str].docs/en/deep-dive/architecture/tab-domain.md#L101-L103: usestrfor the host andboolfor secure transport.
📍 Affects 3 files
docs/en/deep-dive/fundamentals/connection-layer.md#L140-L146(this comment)docs/en/deep-dive/fundamentals/connection-layer.md#L159-L161docs/en/deep-dive/architecture/browser-domain.md#L239-L245docs/en/deep-dive/architecture/tab-domain.md#L101-L103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/en/deep-dive/fundamentals/connection-layer.md` around lines 140 - 146,
Align the documented connection contracts with the implementation: in
docs/en/deep-dive/fundamentals/connection-layer.md lines 140-146, mark
connection_port and page_id as optional, use None for browser-level page
connections, and document ws_address precedence; in lines 159-161, change
_connection_port to Optional[int]; in
docs/en/deep-dive/architecture/browser-domain.md lines 239-245, change
connection_host to Optional[str]; and in
docs/en/deep-dive/architecture/tab-domain.md lines 101-103, use str for the host
and bool for secure transport.
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
1 participant
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.
Refactor connection address resolution to support
connection_host,use_secure, and typed resolver parameters throughout the browser, tab, and connection stack.Pull Request
Description
Related Issue(s)
Type of Change
How Has This Been Tested?
# Include code examples if relevantTesting Checklist
Screenshots
Implementation Details
API Changes
connection_hostanduse_secureparam across all browsers,TabandConnectionHandler.WSAddressResolverParamsand now the ws address resolver callback expects it as arg only. Did it to make the params list non-exhaustive as well as more type checked. Note that the host maybe an ipv6 address hence, must be enclosed in[]before making any requests to it.get_browser_ws_addressis updated to support the callback param.IFrameContextResolveris updated to also use host, use_secure and ws_address when creatingbrowser_handler. Earlier even if you connected using.connect(ws_url), it only used the port to create the connection handler. Similar issue inTab._collect_oopif_shadow_roots._validate_ws_addressnow accepts ws address likews://shortallowing users to usewss://production-sfo.browserless.io?token=YOUR_API_TOKEN_HERE. No validation is done except for valid schemes and a non-empty hostname.Additional Info
connection_portwas mentioned in docs.Checklist before requesting a review
poetry run task lintand fixed any issuespoetry run task testand all tests passSummary by CodeRabbit
New Features
wss://) support for Chrome and Edge.Bug Fixes
Tests
Documentation