[java] Fixed issue with embedded authentication in URLs for JdkHttpClient

[iampopovich]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This pull request includes several changes to the JdkHttpClient class to handle and test the stripping of credentials from URLs. The most significant changes include modifications to the JdkHttpClient class, the addition of new methods and fields, and the introduction of new tests to ensure the correct functionality.

Updates to JdkHttpClient class:

• Added a ClientConfig field and initialized it in the constructor to store configuration settings.
• Introduced logic to strip credentials from the base URI in the getPasswordAuthentication method.
• Added a package-private method getBaseUri for testing purposes to retrieve the base URI from the configuration.
• Enhanced the send method to include the cause in the WebDriverException when interrupted.

New tests in JdkHttpClientTest class:

• Added tests to verify that credentials are stripped from the URL and that other URL components are preserved. [1] [2]

Motivation and Context

Fixes #13803

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Bug fix, Tests

---

Description

• Fixed issue with embedded authentication in URLs for JdkHttpClient.

• Added logic to strip credentials from base URI in JdkHttpClient.

• Introduced new tests to validate credential stripping and URL component preservation.

• Enhanced exception handling in send method to include cause details.

---

Changes walkthrough 📝

	Relevant files
Bug fix	JdkHttpClient.java Implement credential stripping and enhance exception handling java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java Added logic to strip credentials from base URI. Introduced a package-private method getBaseUri for testing. Enhanced exception handling in the send method. Updated ClientConfig initialization to handle credential-free URIs. +27/-1
Tests	JdkHttpClientTest.java Add tests for URL credential stripping and validation        java/test/org/openqa/selenium/remote/http/jdk/JdkHttpClientTest.java Added tests to verify credential stripping from URLs. Validated preservation of other URL components. Tested handling of URLs without credentials. +51/-0

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 13803 - Fully compliant Compliant requirements: Fix implementation mismatch in RemoteWebDriver for URLs with embedded authentication in Java URLs with embedded authentication (e.g., http://admin:myStrongPassword@localhost:4444) should work in Java like in other languages Support for all browsers and RemoteWebDriver

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 Security concerns Credential handling: The code handles sensitive authentication credentials by stripping them from the URL and storing them securely in a PasswordAuthentication object. However, the original URL with credentials is stored in the config field until the credentials are stripped, which could potentially be exposed through logs or error messages. Consider stripping credentials earlier in the process.

⚡ Recommended focus areas for review Error Handling The catch block for URISyntaxException only logs a warning but continues execution. This could lead to authentication issues if the URI stripping fails. } catch (URISyntaxException e) { LOG.log(Level.WARNING, "Could not strip credentials from URI", e); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add null check for password parameter to prevent potential NullPointerException Add null check for password before accessing it in the PasswordAuthentication creation to prevent potential NullPointerException when only username is provided in credentials. java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java [124] -return new PasswordAuthentication(username, password.toCharArray()); +return new PasswordAuthentication(username, (password != null) ? password.toCharArray() : new char[0]); Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion addresses a critical null pointer vulnerability that could occur when credentials contain only a username. This is a valid security concern that could cause runtime crashes.	8
	✅ Preserve all configuration parameters when creating new ClientConfig instance Suggestion Impact: The commit removes the use of ClientConfig.defaultConfig() and modifies the config object directly, preserving the original configuration code diff: - this.config = - ClientConfig.defaultConfig() - .baseUri( - new URI( - config.baseUri().getScheme(), - null, - config.baseUri().getHost(), - config.baseUri().getPort(), - config.baseUri().getPath(), - config.baseUri().getQuery(), - config.baseUri().getFragment())); + config = config.baseUri( + new URI( + config.baseUri().getScheme(), + null, + config.baseUri().getHost(), + config.baseUri().getPort(), + config.baseUri().getPath(), + config.baseUri().getQuery(), + config.baseUri().getFragment())); When creating a new ClientConfig instance, preserve all original config parameters instead of using defaultConfig() to avoid losing custom settings. java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java [131-133] -this.config = - ClientConfig.defaultConfig() - .baseUri( +this.config = new ClientConfig(config) + .baseUri( Apply this suggestion Suggestion importance[1-10]: 7 Why: This is a valid concern as using defaultConfig() could potentially lose custom settings from the original config. The suggestion would help maintain all configuration parameters.	7

[joerg1985]

@VietND96 Please do not merge this, in my mind this will not fix the issue.

[diemol]

@iampopovich have you tested this locally?

[iampopovich]

@diemol sure
I also provided new tests for the bugfix, you can try run it locally
Unfortunately i am away from pc, so i can attach test results later today or tomorrow

[diemol]

Just wondering why @joerg1985 thinks this does not fix the issue.

[joerg1985]

@diemol i will write later this evening details about this and the original issue.

[joerg1985]

This PR does remove the UserInfo from the URI, this is okay, but it will not fix the issue #13803 from my point of view.

I have asked inside the ticket #13803 for details about the setup, but never did get a response. In #13802 you can see a nginx response with 404, so my current assumption is the authentication is done by a reverse proxy not following the RFC for WWW-Authenticate. The Java Authenticator will work only in case the server does send a 401 + challenge. https://datatracker.ietf.org/doc/html/rfc7235#section-4.1

We would need to translate the UserInfo from the URI to a 'Authorization' header and send it blind to the server. This might end in other issues, e.g. we do not know the expected scheme and might send the plain credentials (Basic scheme) instead of a hashed one (Digest scheme) over a unencrypted connection.

The nginx config might be an attemp to hide the service from attackers by forcing the clients to send the 'Authorization' header without a challenge. Some of the selenium language bindings might do so by default and IF it should be supported, this could be implemented in Java too. But this should be done via opt-in and not by default to avoid unexpected issues. And we should know the usecase so we can document this, to avoid more magic lines in the code nobody is aware of the use.

I would propalby hide the service using a UUID prefix in the path, the server has a command line flag to add a prefix...

[VietND96]

@iampopovich, without this fix, for example a simple get started driver = new RemoteWebDriver("http://admin:password@localhost:4444/wd/hub", options);, does it work as of now?

[VietND96]

@iampopovich, without this fix, for example a simple get started driver = new RemoteWebDriver("http://admin:password@localhost:4444/wd/hub", options);, does it work as of now?

Actually, this kind of embedded auth URL works in Python binding. In background, it also extract user:pass from url then Base64 encode it and set to request header Authorization. You can inspect code at

selenium/py/selenium/webdriver/remote/remote_connection.py

Line 419 in 7e194df

auth_header = self._client_config.get_auth_header()

Probably, it is correct with @joerg1985 on the approach that should do.
In JdkHttpClient, how to insert the Authorization to request headers, I think you have to check it.

[VietND96]

@iampopovich, without this fix, for example a simple get started driver = new RemoteWebDriver("http://admin:password@localhost:4444/wd/hub", options);, does it work as of now?

Looks like it works without this change, since I added a test to example site SeleniumHQ/seleniumhq.github.io#2131

[iampopovich]

@VietND96 What should we do next? Clearly, this PR does not fix the bug. I tried implementing authorization similar to the Python bindings client, but it didn’t solve the issue. At the moment, I don’t have enough knowledge to propose an alternative solution.

We can close this pull request for now, and perhaps I’ll revisit the problem later if it’s still relevant.