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.

Sign up for GitHub

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

Jump to bottom

Save pathParamValues encoded and perform decoding when requested #37513

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Save pathParamValues encoded and perform decoding when requested #37513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
41 changes: 18 additions & 23 deletions ...src/main/java/org/jboss/resteasy/reactive/server/core/ResteasyReactiveRequestContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,18 +226,20 @@ public void setMaxPathParams(int maxPathParams) {
}
}

public String getPathParam(int index) {
return doGetPathParam(index, pathParamValues);
public String getPathParam(int index, boolean encoded) {
return doGetPathParam(index, pathParamValues, encoded);
}

private String doGetPathParam(int index, Object pathParamValues) {
private String doGetPathParam(int index, Object pathParamValues, boolean encoded) {
if (pathParamValues instanceof String[]) {
return ((String[]) pathParamValues)[index];
String pathParam = ((String[]) pathParamValues)[index];
return encoded ? pathParam : Encode.decodePath(pathParam);
}
if (index > 1) {
throw new IndexOutOfBoundsException();
}
return (String) pathParamValues;
String pathParam = (String) pathParamValues;
return encoded ? pathParam : Encode.decodePath(pathParam);
}

public ResteasyReactiveRequestContext setPathParamValue(int index, String value) {
Expand Down Expand Up @@ -926,18 +928,11 @@ public String getPathParameter(String name, boolean encoded) {
Integer index = target.getPathParameterIndexes().get(name);
String value;
if (index != null) {
value = getPathParam(index);
} else {
// Check previous resources if the path is not defined in the current target
value = getResourceLocatorPathParam(name);
}

// It's possible to inject a path param that's not defined, return null in this case
if (encoded && value != null) {
return Encode.encodeQueryParam(value);
return getPathParam(index, encoded);
}

return value;
// Check previous resources if the path is not defined in the current target
return getResourceLocatorPathParam(name, encoded);
}

@Override
Expand Down Expand Up @@ -996,8 +991,8 @@ public ResteasyReactiveResourceInfo getResteasyReactiveResourceInfo() {

public abstract Runnable registerTimer(long millis, Runnable task);

public String getResourceLocatorPathParam(String name) {
return getResourceLocatorPathParam(name, (PreviousResource) getProperty(PreviousResource.PROPERTY_KEY));
public String getResourceLocatorPathParam(String name, boolean encoded) {
return getResourceLocatorPathParam(name, (PreviousResource) getProperty(PreviousResource.PROPERTY_KEY), encoded);
}

public FormData getFormData() {
Expand All @@ -1009,7 +1004,7 @@ public ResteasyReactiveRequestContext setFormData(FormData formData) {
return this;
}

private String getResourceLocatorPathParam(String name, PreviousResource previousResource) {
private String getResourceLocatorPathParam(String name, PreviousResource previousResource, boolean encoded) {
if (previousResource == null) {
return null;
}
Expand All @@ -1020,13 +1015,13 @@ private String getResourceLocatorPathParam(String name, PreviousResource previou
for (URITemplate.TemplateComponent component : classPath.components) {
if (component.name != null) {
if (component.name.equals(name)) {
return doGetPathParam(index, previousResource.locatorPathParamValues);
return doGetPathParam(index, previousResource.locatorPathParamValues, encoded);
}
index++;
} else if (component.names != null) {
for (String nm : component.names) {
if (nm.equals(name)) {
return doGetPathParam(index, previousResource.locatorPathParamValues);
return doGetPathParam(index, previousResource.locatorPathParamValues, encoded);
}
}
index++;
Expand All @@ -1036,19 +1031,19 @@ private String getResourceLocatorPathParam(String name, PreviousResource previou
for (URITemplate.TemplateComponent component : previousResource.locatorTarget.getPath().components) {
if (component.name != null) {
if (component.name.equals(name)) {
return doGetPathParam(index, previousResource.locatorPathParamValues);
return doGetPathParam(index, previousResource.locatorPathParamValues, encoded);
}
index++;
} else if (component.names != null) {
for (String nm : component.names) {
if (nm.equals(name)) {
return doGetPathParam(index, previousResource.locatorPathParamValues);
return doGetPathParam(index, previousResource.locatorPathParamValues, encoded);
}
}
index++;
}
}
return getResourceLocatorPathParam(name, previousResource.prev);
return getResourceLocatorPathParam(name, previousResource.prev, encoded);
}

public abstract boolean resumeExternalProcessing();
Expand Down
2 changes: 1 addition & 1 deletion ...g/jboss/resteasy/reactive/server/core/parameters/LocatableResourcePathParamExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public LocatableResourcePathParamExtractor(String name) {

@Override
public Object extractParameter(ResteasyReactiveRequestContext context) {
return context.getResourceLocatorPathParam(name);
return context.getResourceLocatorPathParam(name, false);
}

}
14 changes: 8 additions & 6 deletions .../src/main/java/org/jboss/resteasy/reactive/server/core/parameters/PathParamExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package org.jboss.resteasy.reactive.server.core.parameters;

import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;

import org.jboss.resteasy.reactive.common.util.Encode;
import org.jboss.resteasy.reactive.server.core.ResteasyReactiveRequestContext;
Expand All @@ -19,14 +21,14 @@ public PathParamExtractor(int index, boolean encoded, boolean single) {

@Override
public Object extractParameter(ResteasyReactiveRequestContext context) {
String pathParam = context.getPathParam(index);
if (encoded) {
pathParam = Encode.encodeQueryParam(pathParam);
}
String pathParam = context.getPathParam(index, true);
if (single) {
return pathParam;
return encoded ? pathParam : Encode.decodePath(pathParam);
} else {
return List.of(pathParam.split("/"));
return encoded
? List.of(pathParam.split("/"))
: Arrays.stream(pathParam.split("/")).map(Encode::decodePath)
.collect(Collectors.toList());
}
}
}
2 changes: 1 addition & 1 deletion ...ve/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/jaxrs/UriInfoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public MultivaluedMap<String, String> getPathParameters(boolean decode) {
RuntimeResource target = currentRequest.getTarget();
if (target != null) { // a target can be null if this happens in a filter that runs before the target is set
for (Entry<String, Integer> pathParam : target.getPathParameterIndexes().entrySet()) {
pathParams.add(pathParam.getKey(), currentRequest.getPathParam(pathParam.getValue()));
pathParams.add(pathParam.getKey(), currentRequest.getPathParam(pathParam.getValue(), false));
}
}
}
Expand Down
4 changes: 1 addition & 3 deletions ...erver/runtime/src/main/java/org/jboss/resteasy/reactive/server/mapping/RequestMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
import java.util.Map;
import java.util.regex.Matcher;

import org.jboss.resteasy.reactive.common.util.URIDecoder;

public class RequestMapper<T> {

private static final String[] EMPTY_STRING_ARRAY = new String[0];
Expand Down Expand Up @@ -111,7 +109,7 @@ private RequestMatch<T> mapFromPathMatcher(String path, PathMatcher.PathMatch<Ar
while (matchPos < pathLength && path.charAt(matchPos) != '/') {
matchPos++;
}
params[paramCount++] = URIDecoder.decodeURIComponent(path.substring(start, matchPos), false);
params[paramCount++] = path.substring(start, matchPos);
}
}
if (!matched) {
Expand Down
4 changes: 4 additions & 0 deletions .../src/test/java/org/jboss/resteasy/reactive/server/vertx/test/matching/RegexMatchTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ public void testLiteralInRegex() {
.then()
.statusCode(200)
.body(equalTo("plain:abb/foo/alongpathtotriggerbug"));
RestAssured.get("/regex/first space/foo/second space")
.then()
.statusCode(200)
.body(equalTo("plain:first space/foo/second space"));
Comment on lines +44 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only new test we need?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 For the new issue yes. Paths were not decoded when using a custom regex like the one in RegexResource. This test failed without the fix.

Working on this tcks folder tests failed initially. I had not changed all the calls, that's why I changed the signature, to be sure I was not missing anything. So I hope those tests are testing the common functionality. I can add more tests if you see something missing...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, let's see what CI says

RestAssured.get("/regex/abb/literal/ddc")
.then()
.statusCode(200)
Expand Down
Loading