Skip to content

[java] remove let's guess URI encoding - #17995

Merged
diemol merged 5 commits into
trunkfrom
URI-encoding
Sep 9, 2026
Merged

[java] remove let's guess URI encoding#17995
diemol merged 5 commits into
trunkfrom
URI-encoding

Conversation

@joerg1985

Copy link
Copy Markdown
Member

🔗 Related Issues

Fixes #17994, sorry for the rude text in the issue.

💥 What does this PR do?

Remove the just added encoding step for URIs.

🔧 Implementation Notes

We should not hide incorrect calls to HttpClient, so i removed this again.

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s):
    • What was generated:
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

  • Bug fix (backwards compatible)

@qodo-code-review

Copy link
Copy Markdown
Contributor

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@selenium-ci selenium-ci added the C-java Java Bindings label Sep 7, 2026
@titusfortner titusfortner added this to the 4.49 milestone Sep 8, 2026
@qodo-code-review

qodo-code-review Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Users cannot proxy encoded paths 🐞 Bug ≡ Correctness
Description
getRawUri no longer quotes the decoded value from HttpRequest.getUri() before passing the
assembled URL to URI.create. When Netty converts a valid path such as /session/id/foo%20bar to
/session/id/foo bar, every session request forwarded through the default JDK client can fail
before reaching its node.
Code

java/src/org/openqa/selenium/remote/http/jdk/JdkHttpMessages.java[141]

-      String path = quoteIllegalCharacters(uri);
Evidence
The Netty converter builds Selenium's request from QueryStringDecoder.path(), whose documented
contract is to return the decoded path. Grid routes all /session/ traffic through HandleSession,
which creates a default JDK client and passes the decoded URI unchanged through
ReverseProxyHandler; the changed implementation then calls URI.create without restoring the
required escaping. A valid %20 therefore becomes an illegal literal space and raises
IllegalArgumentException before the upstream request is sent.

java/src/org/openqa/selenium/netty/server/RequestConverter.java[158-166]
java/src/org/openqa/selenium/grid/router/Router.java[62-68]
java/src/org/openqa/selenium/grid/router/HandleSession.java[227-232]
java/src/org/openqa/selenium/grid/router/HandleSession.java[305-324]
java/src/org/openqa/selenium/grid/web/ReverseProxyHandler.java[59-73]
java/src/org/openqa/selenium/remote/http/HttpClient.java[101-109]
java/src/org/openqa/selenium/remote/http/jdk/JdkHttpMessages.java[123-146]
🌐 Netty documents path() as returning the decoded path and rawPath() as returning the raw path.

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Valid percent-encoded request paths are decoded at the Netty boundary and subsequently rejected by the JDK HTTP client because the URI-quoting step was removed.

## Issue Context
Avoid guessing how arbitrary client-provided strings should be encoded, but preserve the original raw path when forwarding an already-valid inbound request. Add coverage proving that encoded spaces and non-ASCII path components survive Grid reverse proxying without double encoding.

## Fix Focus Areas
- java/src/org/openqa/selenium/netty/server/RequestConverter.java[158-166]
- java/src/org/openqa/selenium/grid/web/ReverseProxyHandler.java[59-73]
- java/src/org/openqa/selenium/remote/http/jdk/JdkHttpMessages.java[123-146]
- java/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java[320-323]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Web pages:
  +2 more
Review mode: ⚖️ Balanced: This runtime HTTP URI behavior change removes encoding logic and its tests, creating compatibility and request-construction risk despite the localized two-file scope.

Grey Divider

Tip of the day
💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread java/src/org/openqa/selenium/remote/http/jdk/JdkHttpMessages.java
@diemol

diemol commented Sep 9, 2026

Copy link
Copy Markdown
Member

Yes, I wanted to do this. I merged the initial PR, then I noticed we did not need the endpoint, so I removed it, but I forgot to remove this.

@diemol
diemol merged commit db9eac2 into trunk Sep 9, 2026
12 of 13 checks passed
@qodo-code-review

qodo-code-review Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

No code changes since the last review — review skipped

Qodo Logo

@diemol
diemol deleted the URI-encoding branch September 9, 2026 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: HttpClient breaks URIs due to let's guess what part of the URI needs encoding

4 participants