[grid] Fix 500 when downloading a file whose name contains spaces

[ashrafiucse]

🔗 Related Issues

Fixes #17955

💥 What does this PR do?

GET /session/{sessionId}/se/files/{fileName} returns 500 whenever the file
name contains a space (reported on 4.46, still present in 4.48):

   java.lang.RuntimeException: Unable to execute request for an existing session:                                                                                                 
   Illegal character in path at index 91: http://x.x.x.x:5555/session/.../se/files/xxx aaa.pdf                                                                                    
   Caused by: java.net.URISyntaxException                                                                                                                                         

The root cause is a decode/re-encode asymmetry between the server and the
HTTP client:

1. The Netty server decodes the path of every incoming request
   (QueryStringDecoder.path() in RequestConverter), so %20 becomes a
   literal space in the HttpRequest URI.
2. When the router proxies the request to the node, JdkHttpMessages#getRawUri
   builds the target URI with URI#create, which rejects characters like a
   literal space, and the whole request fails with a 500.

This PR makes the proxy quote such characters, and stops the node from
decoding the file name a second time.

What changed:

• JdkHttpMessages — characters that are not allowed in a URI are
  percent-encoded when building the URI of an outgoing request. Every
  character that URI accepts is left untouched, so URIs that were valid
  before are sent byte-for-byte exactly as they were.
• LocalNode#extractFileName — no longer calls
  urlDecode(...).replace(' ', '+'). The path of the request has already
  been decoded by the server, so the extra decode turned a real file name
  into name+with+spaces.pdf, which never matched the file on disk.
  The substring after /se/files/ is now used as-is.
• HttpClientTestBase — new round-trip test: a request whose path holds a
  space (and a non-ASCII character) is sent through a real Netty server, and
  the handler must receive the same path decoded back.
• LocalNodeTest — extractsFileNameFromRequestUri now also covers a name
  with spaces; the existing assertions are unchanged.

🔧 Implementation Notes

An alternative would be to keep the raw, undecoded path in
RequestConverter, but every route (UrlTemplate, getSessionId, …)
matches against the decoded value, so that would be a much larger change.
Quoting at the outgoing edge instead is local and provably safe: the
transformation is the identity function for every URI that was already
valid, so only requests that previously failed with URISyntaxException
behave differently.

The deprecated GET /se/files/{name} endpoint is kept working rather than
removed (it was marked for removal in #16844). The POST /se/files endpoint,
which all bindings use, is unaffected.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): GLM (Z.ai)
  • What was generated: Root-cause analysis of the reported issue, the fix
    in JdkHttpMessages and LocalNode#extractFileName, and the two test
    additions. I reviewed the final diff, ran the tests locally
    (//java/test/org/openqa/selenium/remote/http:small-tests,
    //java/test/org/openqa/selenium/grid/node/local:LocalNodeTest)
    and verified the fix end-to-end against a running Grid server.
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

• File names containing a literal ? still cannot be referenced by this
  endpoint (the character separates the query string) — same as before this
  PR.
• A file name containing a literal % is ambiguous after decoding, and is
  likewise out of scope here.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

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 →

[CLAassistant]

All committers have signed the CLA.

[joerg1985]

did not look too deep into this but:

1. % is uri not safe, i guess this is to hide issues with allready encoded URIs.
2. the URLEncoder must know the charset to encode a URI, but this implementation not? suspicious.
3. the JdkHttpMessages is a very low level class, high risk of breaking things.

PS: this is a fix to a deprecated method to download the file:
https://github.com/SeleniumHQ/selenium/blob/trunk/java/src/org/openqa/selenium/grid/node/local/LocalNode.java#L936-L938

      if (req.getMethod().equals(HttpMethod.GET)) {
        // Left here for backward compatibility.
        // Remove this IF in Selenium 4.41, 4.42 or 4.43
        return getDownloadedFile(downloadsDirectory, extractFileName(req));
      }

[ashrafiucse]

Thanks for taking a look, @joerg1985! Some answers, hopefully they address the concerns:

1. On % — that one is deliberate, and it is what keeps the change safe for URIs that are already encoded. The proxy regularly forwards paths holding legitimate %XX sequences, and re-encoding % would corrupt them — guava's own urlFragmentEscaper turns a b%20c.pdf into a%20b%2520c.pdf, for example. So % passes through untouched, which makes the transformation the identity for every URI that was valid before this change; only characters URI.create actually rejects (a literal space, non-ASCII, …) get percent-encoded. The residual ambiguity for a file literally named 50%.pdf is the same as before the PR — such requests already failed — and is called out under "Additional Considerations".

2. On the charset — it is pinned to UTF-8: each disallowed code point is turned into its UTF-8 bytes and percent-encoded per RFC 3986. I avoided URLEncoder on purpose, because it is form encoding (application/x-www-form-urlencoded) and writes + for spaces — exactly the historical bug in extractFileName this PR removes. Guava's escaper cannot be dropped in either, for the %2520 reason above.

3. On the risk in JdkHttpMessages — agreed that it is low level, which is why the quoter is conservative: identity for everything that already worked, and only requests that previously died with URISyntaxException behave differently. The round-trip test in HttpClientTestBase pushes a request with a space and a non-ASCII character through a real Netty server and asserts the handler sees the same decoded path back, for both client implementations. If there is a specific scenario you are worried about, I am happy to add a test for it.

PS. on the deprecated endpoint — fair point. The JdkHttpMessages part is endpoint-agnostic and fixes proxying for any request whose decoded path holds characters URI refuses. The LocalNode hunk only keeps the legacy GET from mangling file names until its scheduled removal; if you would rather drop that hunk and pull the removal forward, that works for me too.