Copilot commented on code in PR #879:
URL:
https://github.com/apache/rocketmq-dashboard/pull/879#discussion_r3705427876
##########
server/src/main/java/org/apache/rocketmq/studio/auth/AuthInterceptor.java:
##########
@@ -50,13 +50,27 @@ public boolean preHandle(HttpServletRequest request,
HttpServletResponse respons
}
private boolean isPublicPath(String path) {
+ path = normalizePath(path);
return path.equals("/api/auth/login")
|| path.equals("/api/auth/status")
|| path.startsWith("/api-docs")
|| path.startsWith("/swagger-ui")
|| path.startsWith("/actuator/health");
}
+ private String normalizePath(String path) {
+ if (path == null || path.isBlank()) {
+ return "";
+ }
+ if (path.equals("/")) {
+ return path;
+ }
+ while (path.endsWith("/")) {
+ path = path.substring(0, path.length() - 1);
+ }
+ return path;
+ }
Review Comment:
`normalizePath("///")` currently returns an empty string because the loop
strips all slashes. If the intent is “remove trailing slashes but keep root as
/”, update the loop condition to preserve at least one character (e.g., only
strip while `path.length() > 1`). This also avoids unexpected behavior if a
request path ever includes repeated trailing slashes.
##########
server/src/main/java/org/apache/rocketmq/studio/auth/AuthInterceptor.java:
##########
@@ -50,13 +50,27 @@ public boolean preHandle(HttpServletRequest request,
HttpServletResponse respons
}
private boolean isPublicPath(String path) {
+ path = normalizePath(path);
return path.equals("/api/auth/login")
|| path.equals("/api/auth/status")
|| path.startsWith("/api-docs")
|| path.startsWith("/swagger-ui")
|| path.startsWith("/actuator/health");
}
+ private String normalizePath(String path) {
+ if (path == null || path.isBlank()) {
+ return "";
+ }
+ if (path.equals("/")) {
+ return path;
+ }
+ while (path.endsWith("/")) {
+ path = path.substring(0, path.length() - 1);
+ }
Review Comment:
This loop allocates a new `String` for each trailing slash. Consider
computing the end index once (or trimming in a single `substring`) to avoid
repeated allocations; it keeps behavior the same but is more efficient and
easier to reason about.
##########
server/src/test/java/org/apache/rocketmq/studio/auth/AuthInterceptorTest.java:
##########
@@ -89,6 +89,18 @@ void shouldAllowLoginEndpointWhenLoginIsEnabled() throws
Exception {
assertThat(allowed).isTrue();
}
+ @Test
+ void shouldAllowLoginEndpointWithTrailingSlashWhenLoginIsEnabled() throws
Exception {
+ AuthProperties properties = new AuthProperties();
+ properties.setLoginRequired(true);
+ AuthInterceptor interceptor = new AuthInterceptor(properties, new
AuthService(properties));
+ MockHttpServletRequest request = new MockHttpServletRequest("POST",
"/api/auth/login/");
Review Comment:
Test setup (creating `AuthProperties`, enabling `loginRequired`,
constructing `AuthInterceptor`) is duplicated across multiple tests in this
class. Consider extracting a small helper or using a `@BeforeEach` initializer
to reduce repetition and make future test additions less error-prone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
