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

Fix off-by-one issue caused by ObservabilityIntegrationRecorder using its own method for getting path without prefix #41927

Merged
merged 2 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading