Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RESTEasy ResponseBuilder.location Inadvertently Decodes Path Segments in Relative URI #33419

Closed
gilday opened this issue May 16, 2023 · 5 comments · Fixed by #33448
Closed

RESTEasy ResponseBuilder.location Inadvertently Decodes Path Segments in Relative URI #33419

gilday opened this issue May 16, 2023 · 5 comments · Fixed by #33448
Labels
area/rest kind/bug Something isn't working
Milestone

Comments

@gilday
Copy link

gilday commented May 16, 2023

Describe the bug

The ResponseBuilder.location(URI) method accepts relative URIs and will build an absolute URI relative to the base URI to put in the Location header. When the relative URI contains URL-encoded path segments, ResponseBuilder.location erroneously decodes those segments when building the new absolute URI.

For example, the given JAX-RS resource method should build an HTTP response with a Location header that has the URI path /items/foo%2Fbar, but it instead returns /items/foo/bar.

public Response create() { 
  var uri = UriBuilder.newInstance().path("{id}").build("foo/bar");
  return ResponseBuilder.status(202).location(uri).build(); 
}

Expected behavior

ResponseBuilder.location creates an absolute URI without changing the semantics of the URI path.

Actual behavior

ResponseBuilder.location creates an absolute URI with a URI path that has different semantics.

How to Reproduce?

quarkus-resteasy-reactive-uri-decode-issue.tar.gz

The attached reproducer includes a test that demonstrates the issue through a failure. The test fails with a message like

2023-05-16 13:29:47,195 INFO  [io.quarkus] (main) quarkus-resteasy-reactive-uri-decode-issue 1.0.0-SNAPSHOT on JVM (powered by Quarkus 3.0.3.Final) started in 0.940s. Listening on: http://localhost:63035
2023-05-16 13:29:47,214 INFO  [io.quarkus] (main) Profile test activated. 
2023-05-16 13:29:47,215 INFO  [io.quarkus] (main) Installed features: [cdi, resteasy-reactive, smallrye-context-propagation, vertx]
HTTP/1.1 201 Created
Location: http://localhost:63035/greeting/en/us
content-length: 0

java.lang.AssertionError: 1 expectation failed.
Expected header "Location" was not a string matching the pattern 'http://localhost:\d+/greeting/en-us', was "http://localhost:63035/greeting/en/us". Headers are:
Location=http://localhost:63035/greeting/en/us
content-length=0

because the URI path should have been /greeting/en%2Fus

Output of uname -a or ver

Darwin pixee-mbp-gilday.localdomain 22.4.0 Darwin Kernel Version 22.4.0: Mon Mar 6 20:59:28 PST 2023; root:xnu-8796.101.5~3/RELEASE_ARM64_T6000 arm64

Output of java -version

openjdk version "17.0.7" 2023-04-18 OpenJDK Runtime Environment Temurin-17.0.7+7 (build 17.0.7+7) OpenJDK 64-Bit Server VM Temurin-17.0.7+7 (build 17.0.7+7, mixed mode)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.0.3.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.8 (4c87b05d9aedce574290d1acc98575ed5eb6cd39) Maven home: /Users/jgilday/.m2/wrapper/dists/apache-maven-3.8.8-bin/67c30f74/apache-maven-3.8.8 Java version: 17.0.7, vendor: Eclipse Adoptium, runtime: /Library/Java/JavaVirtualMachines/temurin-17.jdk/Contents/Home Default locale: en_US, platform encoding: UTF-8 OS name: "mac os x", version: "13.3.1", arch: "aarch64", family: "mac"

Additional information

No response

@gilday gilday added the kind/bug Something isn't working label May 16, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented May 17, 2023

/cc @FroMage (resteasy-reactive), @Sgitario (resteasy-reactive), @stuartwdouglas (resteasy-reactive)

@Sgitario
Copy link
Contributor

@gilday I guess the assertion in your reproducer is wrong:

.header("Location", matchesPattern("http://localhost:\\d+/greeting/en-us"));

Instead, you would expect to be:

.header("Location", endsWith("greeting/en%2Fus"));

@geoand I've checked RESTEasy classic and it returns the raw params localhost:xxx/greeting/en%2Fus. I see this is done in this line. Using .getRawPath would make it, tho I'm not sure what the specs say about it.

@geoand
Copy link
Contributor

geoand commented May 17, 2023

tho I'm not sure what the specs say about it.

You can certainly try that change, but my guess is that the TCK will fail

@gilday
Copy link
Author

gilday commented May 17, 2023

I guess the assertion in your reproducer is wrong:

Yes, I had two test cases in there, and I deleted the wrong one 🤦🏻‍♂️

Sgitario added a commit to Sgitario/quarkus that referenced this issue May 17, 2023
When providing a location, the URI was being decoded, so the value was being altered from what users set. 

Note that these changes are based on what Resteasy already does: https://github.com/resteasy/resteasy/blob/dadddfb699a875c44ba05c0abe176873acbd9aa2/resteasy-core/src/main/java/org/jboss/resteasy/specimpl/ResponseBuilderImpl.java#L187

Fix quarkusio#33419
@Sgitario
Copy link
Contributor

.header("Location", endsWith("greeting/en%2Fus"));

#33448 should include these changes.
The TCK tests seem to be happy with these changes (at least, when I ran them locally!)

@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone May 18, 2023
@gsmet gsmet modified the milestones: 3.2 - main, 3.1.0.Final May 23, 2023
gsmet pushed a commit to gsmet/quarkus that referenced this issue May 23, 2023
When providing a location, the URI was being decoded, so the value was being altered from what users set.

Note that these changes are based on what Resteasy already does: https://github.com/resteasy/resteasy/blob/dadddfb699a875c44ba05c0abe176873acbd9aa2/resteasy-core/src/main/java/org/jboss/resteasy/specimpl/ResponseBuilderImpl.java#L187

Fix quarkusio#33419

(cherry picked from commit ced8b0a)
@gsmet gsmet modified the milestones: 3.1.0.Final, 3.0.4.Final May 23, 2023
gsmet pushed a commit to gsmet/quarkus that referenced this issue May 23, 2023
When providing a location, the URI was being decoded, so the value was being altered from what users set.

Note that these changes are based on what Resteasy already does: https://github.com/resteasy/resteasy/blob/dadddfb699a875c44ba05c0abe176873acbd9aa2/resteasy-core/src/main/java/org/jboss/resteasy/specimpl/ResponseBuilderImpl.java#L187

Fix quarkusio#33419

(cherry picked from commit ced8b0a)
@gsmet gsmet modified the milestones: 3.0.4.Final, 2.16.8.Final Jun 28, 2023
gsmet pushed a commit to gsmet/quarkus that referenced this issue Jun 28, 2023
When providing a location, the URI was being decoded, so the value was being altered from what users set.

Note that these changes are based on what Resteasy already does: https://github.com/resteasy/resteasy/blob/dadddfb699a875c44ba05c0abe176873acbd9aa2/resteasy-core/src/main/java/org/jboss/resteasy/specimpl/ResponseBuilderImpl.java#L187

Fix quarkusio#33419

(cherry picked from commit ced8b0a)
gsmet pushed a commit to gsmet/quarkus that referenced this issue Jun 28, 2023
When providing a location, the URI was being decoded, so the value was being altered from what users set.

Note that these changes are based on what Resteasy already does: https://github.com/resteasy/resteasy/blob/dadddfb699a875c44ba05c0abe176873acbd9aa2/resteasy-core/src/main/java/org/jboss/resteasy/specimpl/ResponseBuilderImpl.java#L187

Fix quarkusio#33419

(cherry picked from commit ced8b0a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants