Skip to content

Commit

Permalink
Merge pull request quarkusio#41927 from bechhansen/fix-off-by-one-pat…
Browse files Browse the repository at this point in the history
…h-issue

Fix off-by-one issue caused by ObservabilityIntegrationRecorder using its own method for getting path without prefix
  • Loading branch information
geoand authored Aug 29, 2024
2 parents 33e1e1f + 04def9c commit 6d7f998
Show file tree
Hide file tree
Showing 7 changed files with 30 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import jakarta.ws.rs.HttpMethod;

import org.jboss.logging.Logger;
import org.jboss.resteasy.reactive.common.util.PathHelper;
import org.jboss.resteasy.reactive.server.core.Deployment;
import org.jboss.resteasy.reactive.server.handlers.ClassRoutingHandler;
import org.jboss.resteasy.reactive.server.mapping.RequestMapper;
Expand Down Expand Up @@ -112,20 +113,7 @@ public static void setTemplatePath(RoutingContext rc, Deployment deployment) {
setUrlPathTemplate(rc, templatePath);
}

private static String getPath(RoutingContext rc) {
return rc.normalizedPath();
}

private static String getPathWithoutPrefix(RoutingContext rc, Deployment deployment) {
String path = getPath(rc);
if (path != null) {
String prefix = deployment.getPrefix();
if (!prefix.isEmpty()) {
if (path.startsWith(prefix)) {
return path.substring(prefix.length());
}
}
}
return path;
return PathHelper.getPathWithoutPrefix(rc.normalizedPath(), deployment.getPrefix());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

/**
* A utility class for handling URI template parameters. As the Java
* regulare expressions package does not handle named groups, this
* regular expressions package does not handle named groups, this
* class attempts to simulate that functionality by using groups.
*
* @author Ryan J. McDonough
Expand Down Expand Up @@ -99,4 +99,19 @@ public static String recoverEnclosedCurlyBraces(String str) {
return str.replace(openCurlyReplacement, '{').replace(closeCurlyReplacement, '}');
}

public static String getPathWithoutPrefix(String path, String prefix) {
if (path != null) {
if (!prefix.isEmpty()) {
// FIXME: can we really have paths that don't start with the prefix if there's a prefix?
if (path.startsWith(prefix)) {
if (path.length() == prefix.length()) {
return "/";
}
return path.substring(prefix.length());
}
}
}
return path;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.jboss.resteasy.reactive.common.NotImplementedYet;
import org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext;
import org.jboss.resteasy.reactive.common.util.Encode;
import org.jboss.resteasy.reactive.common.util.PathHelper;
import org.jboss.resteasy.reactive.common.util.PathSegmentImpl;
import org.jboss.resteasy.reactive.server.SimpleResourceInfo;
import org.jboss.resteasy.reactive.server.core.multipart.FormData;
Expand Down Expand Up @@ -443,20 +444,7 @@ public String getRemaining() {
* Returns the normalised non-decoded path excluding any prefix.
*/
public String getPathWithoutPrefix() {
String path = getPath();
if (path != null) {
String prefix = deployment.getPrefix();
if (!prefix.isEmpty()) {
// FIXME: can we really have paths that don't start with the prefix if there's a prefix?
if (path.startsWith(prefix)) {
if (path.length() == prefix.length()) {
return "/";
}
return path.substring(prefix.length());
}
}
}
return path;
return PathHelper.getPathWithoutPrefix(getPath(), deployment.getPrefix());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.instrumentation.annotations.WithSpan;
import io.smallrye.mutiny.Uni;
import io.vertx.core.impl.NoStackTraceException;

@Path("/reactive")
public class ReactiveResource {
Expand Down Expand Up @@ -82,14 +83,14 @@ public Uni<String> helloPost(String body) {
@Path("blockingException")
@GET
public String blockingException() {
throw new RuntimeException("dummy");
throw new NoStackTraceException("dummy");
}

@Path("reactiveException")
@GET
public Uni<String> reactiveException() {
return Uni.createFrom().item(() -> {
throw new RuntimeException("dummy2");
throw new NoStackTraceException("dummy2");
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@

import org.jboss.resteasy.reactive.RestPath;

@Path("secured")
@Path("{dummy}/secured")
public class SecuredResource {
@GET
@Path("item/{value}")
public String get(@RestPath String value) {
public String get(@RestPath String dummy, @RestPath String value) {
return "Received: " + value;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ quarkus.security.users.embedded.users.stuart=writer
quarkus.security.users.embedded.roles.scott=READER
quarkus.security.users.embedded.roles.stuart=READER,WRITER
quarkus.http.auth.permission.secured.policy=authenticated
quarkus.http.auth.permission.secured.paths=/secured/*
quarkus.http.auth.permission.secured.paths=/foo/secured/*

quarkus.otel.security-events.enabled=true
Original file line number Diff line number Diff line change
Expand Up @@ -248,26 +248,26 @@ void multipleUsingCombine() {

@Test
public void securedInvalidCredential() {
given().auth().preemptive().basic("scott", "reader2").when().get("/secured/item/something")
given().auth().preemptive().basic("scott", "reader2").when().get("/foo/secured/item/something")
.then()
.statusCode(401);

await().atMost(5, SECONDS).until(() -> getSpans().size() == 1);
assertThat(getSpans()).singleElement().satisfies(m -> {
assertThat(m).extractingByKey("name").isEqualTo("GET /secured/item/{value}");
assertThat(m).extractingByKey("name").isEqualTo("GET /{dummy}/secured/item/{value}");
assertEvent(m, SecurityEventUtil.AUTHN_FAILURE_EVENT_NAME);
});
}

@Test
public void securedProperCredentials() {
given().auth().preemptive().basic("scott", "reader").when().get("/secured/item/something")
given().auth().preemptive().basic("scott", "reader").when().get("/foo/secured/item/something")
.then()
.statusCode(200);

await().atMost(5, SECONDS).until(() -> getSpans().size() == 1);
assertThat(getSpans()).singleElement().satisfies(m -> {
assertThat(m).extractingByKey("name").isEqualTo("GET /secured/item/{value}");
assertThat(m).extractingByKey("name").isEqualTo("GET /{dummy}/secured/item/{value}");
assertEvent(m, SecurityEventUtil.AUTHN_SUCCESS_EVENT_NAME, SecurityEventUtil.AUTHZ_SUCCESS_EVENT_NAME);
});
}
Expand Down

0 comments on commit 6d7f998

Please sign in to comment.