[reta]

CXF-7986: WebClient.path method overwrites base URL when given absolute URL as input. The UriBuilder is pretty clearly states that path represents a relative URI:

/**
     * Create a new instance representing a relative URI initialized from a URI path.
     *
     * @param path a URI path that will be used to initialize the UriBuilder, may contain URI template parameters.
     * @return a new UriBuilder.
     * @throws IllegalArgumentException if path is {@code null}.
     */
    public static UriBuilder fromPath(final String path) throws IllegalArgumentException {
        return newInstance().path(path);
    }

However, UriBuilderImpl does override the whole URI when path is supplied with absolute URI.

[coheigea]

@reta Claude tells me it causes a TCK regression:

The Jakarta REST TCK test ee.jakarta.tck.ws.rs.api.rs.core.uribuilder.JAXRSClientIT calls UriBuilder.fromPath(...) with an absolute URI 25 times. Examples are fromPath(LOCALHOST), where LOCALHOST = "http://localhost:8080", and replaceMatrixParamTest3 / replaceMatrixTest2, which use fromPath("http://localhost:8080;name=x=;..."). It expects the absolute URI to come back.

Suggested fix:

if (isAbsoluteUriPath(path) && scheme == null && host == null
        && schemeSpecificPart == null && paths.isEmpty()) {
    // existing URI.create / TCK-hack branch, unchanged
}

This still fixes CXF-7986, because a WebClient base always has a scheme and host, so path("http://bar") gets appended. fromPath("http://…") stays TCK-compatible, and the unit-test rewrites, the expected-value changes and the JAXBElementProvider change can be reverted. The three new WebClientTest cases should still pass. I’d also add a UriBuilderImplTest case such as fromUri("http://foo").path("http://bar"), so the fix is covered in rt/frontend/jaxrs itself.

Some other points:

Inconsistency. replacePath(String) (around line 781) still treats http:/https: as a full URI, so path() and replacePath() now disagree.

Appended URL loses a slash. path("http://www.bar.com") now gives …/http:/www.bar.com, because the // is collapsed. That’s harmless, but it shows the input isn’t being encoded faithfully. The new tests only check the host and a startsWith, so they don’t pin this down.

[reta]

@reta Claude tells me it causes a TCK regression:

Thanks @coheigea , I will adapt accrodingly

[coheigea]

Problems that remain

1. buildFromEncoded() and toTemplate() corrupt the host. These are the failing rows above: the host becomes foo.comhttp. The new branch reaches an older bug in buildUriString. fromPath("http://foo.com") sets originalPathEmpty = true, and that flag stops a / being added before a path that doesn’t already start with one. build() adds the / in buildURI; buildFromEncoded() and toTemplate() don’t.
2. The same older bug lets path input change the host, and it is already on main. On both main and this PR, UriBuilder.fromPath("http://foo.com").path(".evil.com").buildFromEncoded() gives http://foo.com.evil.com. Calling build() on the same builder gives the correct http://foo.com/.evil.com. The trigger is limited to builders started with fromPath(absoluteUri); fromUri/WebClient bases don’t set the flag. It’s still path input rewriting the host, which is the class of bug CXF-7986 is about. A likely fix: in buildUriString, skip the / only when originalPathEmpty is set and the path starts with ;, which is what the TCK’s empty-path matrix-parameter case needs. That would also fix item 1. This was a security problem on main before this PR, so you may want to handle it through the private security process rather than in a public PR comment.
3. The new tests are too weak to catch this. testBuildAndSetFullPathWhenUriIsSet and testReplaceFullPathWhenUriIsSet only check startsWith("http://foo.com"), which http://foo.comhttp:/bar.com/ also passes. They should check the exact string or getHost(), and cover buildFromEncoded() too. The WebClient tests have the same weakness: preservesPreviouslyAddedPathParameters only checks a prefix.

[reta]

1. buildFromEncoded() and toTemplate() corrupt the host.

Thanks @coheigea , I think I fixed the buildFromEncoded() but I will double check the toTemplate(), I suspect it still could cause host corruption (I will post an update shortly).

UPD: @coheigea I think we should be all set, thanks!

[coheigea]

There is one remaining gap - the ; case that was kept for the TCK still writes the path straight after the host with no /. @ is a legal path character, so it isn’t encoded.

fromPath("http://foo.com").matrixParam("a", "@evil.com").build()  -> http://foo.com;a=@evil.com   host=evil.com
fromPath("http://foo.com").path(";@evil.com").build()             -> http://foo.com;@evil.com     host=evil.com
fromPath("http://foo.com").path("{x}").build(";@evil.com")        -> host=evil.com
fromPath("http://foo.com").replaceMatrix("a=@evil.com").build()   -> host=evil.com

Possible fix: in the originalPathEmpty && thePath.startsWith(";") branch of buildUriString, encode @ as %40 before appending.

[reta]

There is one remaining gap - the ; case that was kept for the TCK still writes the path straight after the host with no /. @ is a legal path character, so it isn’t encoded.

@coheigea with this one we opened a pandora box, sadly:

• non encoded: URI.create("http://foo.com;@evil.com").getHost() -> "evil.com"
• encoding does not work: URI.create("http://foo.com;%40evil.com").getHost() -> null

But it is not just ;, I found these ones:

• non encoded: URI.create("http://foo.com&@evil.com").getHost() -> "evil.com"`
• encoding does not work: URI.create("http://foo.com&%40evil.com").getHost() -> null
• non encoded: URI.create("http://foo.com+@evil.com").getHost() -> "evil.com"`
• encoding does not work: URI.create("http://foo.com+%40evil.com").getHost() -> null

The only reliable way that I found is to use leading slash in front of path (but that will change the behavior of UriBuilder::fromUri / UriBuilder::fromPath since we will inject slash there):
-> URI.create("http://foo.com/;@evil.com").getHost() -> "foo.com"
-> URI.create("http://foo.com/&@evil.com").getHost() -> "foo.com"
-> URI.create("http://foo.com/+@evil.com").getHost() -> "foo.com"

Alternatively, we could fail with an exception but that could be surprising.

[coheigea]

A few small, non-blocking points on the latest commit:

• Javadoc on needsLeadingSlash: it says "The URI parsing logic seems to be flawed", but java.net.URI follows RFC 3986 here. ;, & and + are valid userinfo characters, so in http://foo.com;@evil.com the userinfo is foo.com; and the host is evil.com. Suggested wording:

  ;, & and + are valid userinfo characters (RFC 3986), so a path glued to the host that contains @ is parsed as userinfo and the text after @ becomes the host. To prevent that, always insert a leading / when the path contains @.

• Double negative in buildURI: !(thePath.startsWith("/") || !needsLeadingSlash(thePath)) reads more easily as !thePath.startsWith("/") && needsLeadingSlash(thePath).
• Test names: the new testUserInfoAnd...DoesNotChangeHost tests check @ in the path or matrix parameters, not userinfo. A name like testAtSignInMatrixDoesNotChangeHost would describe them better.
• Formatting:
  • The comment in testUserInfoAndPathWithMatrix is misindented ( // The URI will treat…).
  • These lines have trailing whitespace: the blank line before the needsLeadingSlash javadoc, its * lines, the blank line before testPathWithUserInfoDoesNotChangeHost, and // Always include the leading slash in order to prevent host overrides .
• PR description: it still argues that a path is always relative. It would help to describe the actual rule instead: an absolute URI passed to path()/replacePath() replaces the URI only when no scheme or host is set yet, and otherwise it's appended as a path.

[reta]

A few small, non-blocking points on the latest commit:

Thanks @coheigea , addressed a majority of them, also included @ encoded as %40 into the check, + test cases.