fix: Perform additional security checks on overrideServerUrl API#2408
fix: Perform additional security checks on overrideServerUrl API#2408mykola-mokhnach merged 2 commits intomasterfrom
Conversation
mykola-mokhnach
requested review from
SrinivasanTarget,
saikrishna321 and
valfirst
as code owners
There was a problem hiding this comment.
Pull request overview
This PR hardens the direct-connect/overrideServerUrl path by validating server-provided override URLs to prevent unsafe redirection targets (e.g., loopback and link-local addresses), reducing SSRF-style risk during session creation.
Changes:
- Added
DirectConnectUrlSafetyhelper to validate override URL hosts against disallowed address classes. - Enforced the safety validation in
AppiumCommandExecutor.overrideServerUrl. - Expanded unit test coverage for safe vs. unsafe override targets (including new dedicated safety tests).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java |
Calls the new safety validator before overriding the executor’s base URL; updates Javadoc to describe restrictions. |
src/main/java/io/appium/java_client/internal/DirectConnectUrlSafety.java |
New validation utility that resolves the host and rejects loopback/link-local/unspecified/multicast destinations. |
src/test/java/io/appium/java_client/remote/AppiumCommandExecutorTest.java |
Updates the override URL test to use IP literals and adds a loopback rejection assertion. |
src/test/java/io/appium/java_client/internal/DirectConnectUrlSafetyTest.java |
New unit tests for the safety validator covering several allowed/blocked scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param serverUrl A url to override. The host must resolve only to routable addresses; loopback, | ||
| * link-local (including metadata service ranges), wildcard, and multicast targets | ||
| * are rejected. |
There was a problem hiding this comment.
The Javadoc says the host must resolve only to “routable addresses”, but the actual safety check only rejects loopback/link-local/unspecified/multicast. Consider rewording this to match the implemented policy (or expand the validation if “routable” is the intended requirement), so callers aren’t misled about what is blocked/allowed.
| * @param serverUrl A url to override. The host must resolve only to routable addresses; loopback, | |
| * link-local (including metadata service ranges), wildcard, and multicast targets | |
| * are rejected. | |
| * @param serverUrl A url to override. The host must not resolve to loopback, link-local | |
| * (including metadata service ranges), wildcard/unspecified, or multicast | |
| * addresses. |
| private static boolean isDisallowed(InetAddress address) { | ||
| return address.isLoopbackAddress() | ||
| || address.isLinkLocalAddress() | ||
| || address.isAnyLocalAddress() | ||
| || address.isMulticastAddress(); |
There was a problem hiding this comment.
isDisallowed rejects unspecified (any-local) and multicast addresses, but the new tests only cover loopback and link-local cases. Add unit tests that assert rejection for 0.0.0.0 / [::] (unspecified) and a multicast literal (e.g., 224.0.0.1) to keep this security-sensitive logic fully covered.
Change list
Ensures additional safety on server provided urls
Types of changes