Conversation
There was a problem hiding this comment.
Pull request overview
Adds validation around Appium/Selenium “direct connect” capability values so the Ruby client avoids switching its HTTP endpoint to unsafe/unusable hosts (e.g., loopback), and updates unit tests accordingly.
Changes:
- Implemented
DirectConnections#valid?with DNS/IP-based host checks and fallback behavior when invalid. - Added/updated unit tests to cover valid direct-connect endpoints and loopback fallback behavior.
- Updated direct-connect documentation comment in the driver.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/appium_lib_core/driver.rb |
Adds direct-connect validation (DNS/IP checks) and only updates the HTTP endpoint when valid; otherwise logs and keeps the original URL. |
test/unit/driver_test.rb |
Updates direct-connect test fixtures away from loopback hosts and adds a new test asserting fallback when loopback is provided. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def resolve_addresses(host) | ||
| normalized_host = host.to_s.delete_prefix('[').delete_suffix(']') | ||
| Socket.getaddrinfo(normalized_host, nil).map { |entry| entry[3] }.uniq | ||
| rescue SocketError => e | ||
| error_message = "Failed to resolve host '#{host}' for direct connect: #{e.message}" | ||
| ::Appium::Logger.warn(error_message) | ||
| [] | ||
| end |
There was a problem hiding this comment.
valid? calls Socket.getaddrinfo during start_driver, which can introduce blocking DNS resolution into session creation and may fail in restricted/offline environments even when the direct-connect URL would otherwise be usable. To reduce operational risk, consider short-circuiting resolution when host is already an IP literal, and/or making hostname resolution optional or bounded (e.g., a configurable timeout / non-blocking resolver) so start_driver cannot hang on DNS.
| appActivity: 'io.appium.android.apis.ApiDemos', | ||
| someCapability: 'some_capability', | ||
| 'appium:directConnectProtocol' => 'http', | ||
| 'appium:directConnectHost' => 'localhost', | ||
| 'appium:directConnectProtocol' => 'https', | ||
| 'appium:directConnectHost' => 'appium.io', | ||
| 'appium:directConnectPort' => '8888', | ||
| 'appium:directConnectPath' => '/wd/hub' | ||
| } |
There was a problem hiding this comment.
This test now depends on external DNS resolution for appium.io because the implementation validates direct-connect hosts using Socket.getaddrinfo. In CI/offline environments, DNS failures would cause d_c.valid? to return false and the driver to fall back to the original URL, making this test flaky. Prefer using an IP literal (e.g., a TEST-NET address like 203.0.113.10) or stubbing Socket.getaddrinfo in the test.
appium/java-client#2408 for this client