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

fix(fs-router) check for routes with / first, then without #2258

Merged
merged 8 commits into from
Mar 27, 2024
Merged

fix(fs-router) check for routes with / first, then without #2258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
39c79d7
Check for routes with / first, then without
cromoteca Mar 25, 2024
766a204
Merge branch 'main' into fix/2244/file-router-the-view-for-the-root-p…
cromoteca Mar 25, 2024
c17a4b0
Merge branch 'main' into fix/2244/file-router-the-view-for-the-root-p…
cromoteca Mar 26, 2024
341848b
Use `endsWith`
cromoteca Mar 26, 2024
b049890
Merge branch 'main' into fix/2244/file-router-the-view-for-the-root-p…
cromoteca Mar 26, 2024
63f55d9
Further simplify checks
cromoteca Mar 26, 2024
bb170c8
Merge branch 'main' into fix/2244/file-router-the-view-for-the-root-p…
taefi Mar 27, 2024
230bdd0
Merge branch 'main' into fix/2244/file-router-the-view-for-the-root-p…
taefi Mar 27, 2024
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
26 changes: 20 additions & 6 deletions packages/java/endpoint/src/main/java/com/vaadin/hilla/route/ClientRouteRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.vaadin.flow.function.DeploymentConfiguration;
import com.vaadin.hilla.route.records.ClientViewConfig;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Component;
Expand All @@ -32,6 +33,8 @@
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Set;
import java.util.Objects;
import java.util.stream.Stream;

/**
* Keeps track of registered client side routes.
Expand Down Expand Up @@ -98,12 +101,23 @@ public void removeRoute(String route) {
public ClientViewConfig getRouteByPath(String path) {
final Set<String> routes = registeredRoutes.keySet();
final AntPathMatcher pathMatcher = new AntPathMatcher();
for (String route : routes) {
if (pathMatcher.match(route, path)) {
return registeredRoutes.get(route);
}
}
return null;
return Stream.of(addTrailingSlash(path), removeTrailingSlash(path))
.map(p -> {
for (String route : routes) {
if (pathMatcher.match(route, p)) {
return registeredRoutes.get(route);
}
}
return null;
}).filter(Objects::nonNull).findFirst().orElse(null);
}
Copy link

Choose a reason for hiding this comment

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

Just bypassing here: Did you consider security here? I remember some CVEs in the recent past which originated from security confusion based on trailing slash..

Additionally there is this recent Spring Framework Change to keep in mind: spring-projects/spring-framework#28552

Copy link
Contributor Author

@cromoteca cromoteca Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This code doesn't actually change which content is rendered. Its purpose is to try to give the same view permission response that you would get on the client where the navigation is handled by the React Routed.

By doing some manual testing, I've seen that when two routes are present which are only different on the trailing slash, React Router always renders the one with the slash (e.g. you cannot see /about if /about/ exists). But it is also forgiving: /about/ will show the view associated to /about if an exact match is not there.

We don't have this information here, since the new File Router only stores the path items (e.g. ["about"]). Also, the root path is stored as "", but it must match "/".

This PR modifies the code that tries to find the route that React Router would render given a path. In that sense, it should not open any additional security breach.


private String addTrailingSlash(String path) {
return path.endsWith("/") ? path : path + '/';
}

private String removeTrailingSlash(String path) {
return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
}

/**
Expand Down
27 changes: 27 additions & 0 deletions packages/java/endpoint/src/test/java/com/vaadin/hilla/route/RouteUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,4 +190,31 @@ public void test_login_required_on_page() {
boolean actual = endpointUtil.isRouteAllowed(request);
Assert.assertFalse(actual);
}

/**
* Verifies that the root route is allowed when login is not required,
* despite the mismatch between "/" and "".
*/
@Test
public void test_login_not_required_on_root() {
MockHttpServletRequest request = new MockHttpServletRequest();
request.setRequestURI("/context/");
request.setContextPath("/context");
request.setUserPrincipal(null);

ClientViewConfig config = new ClientViewConfig();
config.setTitle("Root");
config.setRolesAllowed(null);
config.setLoginRequired(false);
config.setRoute("");
config.setLazy(false);
config.setAutoRegistered(false);
config.setMenu(null);
config.setChildren(null);
config.setRouteParameters(null);
registry.addRoute("", config);

boolean actual = endpointUtil.isRouteAllowed(request);
Assert.assertTrue(actual);
}
}
Loading